-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(joins): support MySQL 8.0 #4639
Conversation
85f4380
to
29525e7
Compare
WASM Size
|
CodSpeed Performance ReportMerging #4639 will not alter performanceComparing Summary
|
✅ WASM query-engine performance won't change substantially (0.996x)Full benchmark report
After changes in a58db5f |
e7af6ad
to
8bf285c
Compare
( | ||
Some(TypeFamily::Text(_)), | ||
Some("LONGBLOB") | Some("BLOB") | Some("MEDIUMBLOB") | Some("SMALLBLOB") | Some("TINYBLOB") | ||
| Some("VARBINARY") | Some("BINARY") | Some("BIT"), | ||
) => { | ||
self.write("to_base64")?; | ||
self.surround_with("(", ")", |s| s.visit_expression(expr))?; | ||
|
||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We convert binary columns to base64
to ease coercion in the Query Engine
(_, Some("FLOAT")) => { | ||
self.write("CONVERT")?; | ||
self.surround_with("(", ")", |s| { | ||
s.visit_expression(expr)?; | ||
s.write(", ")?; | ||
s.write("CHAR") | ||
})?; | ||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We convert floats to string to avoid losing precision
ScalarFieldId::InCompositeType(id) => self.dm.walk(id).scalar_type(), | ||
}; | ||
|
||
let nt = psl_nt.or_else(|| scalar_type.and_then(|st| connector.default_native_type_for_scalar_type(&st)))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To enable applying special conversion in quaint for specific native types, I updated the ScalarField::native_type
function to return the default native type in case none are specified. Previously, this function would only return something if a native type was explicitly specified.
@@ -39,7 +39,7 @@ static DEFAULT_MAPPING: Lazy<HashMap<ScalarType, MongoDbType>> = Lazy::new(|| { | |||
(ScalarType::Float, MongoDbType::Double), | |||
(ScalarType::Boolean, MongoDbType::Bool), | |||
(ScalarType::String, MongoDbType::String), | |||
(ScalarType::DateTime, MongoDbType::Timestamp), | |||
(ScalarType::DateTime, MongoDbType::Date), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change mentioned above 👆 has rippled in unintended ways. Notably, it surfaced an incorrect default native type for the DateTime
prisma type. This is why I've changed this.
fn default_native_type_for_scalar_type(&self, _scalar_type: &ScalarType) -> Option<NativeTypeInstance> { | ||
None | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, now that this method is called for all connectors when constructing quaint columns, the function's signature was changed to better signal when a scalar type has no default native type.
let actual = logs | ||
.iter() | ||
.any(|l| l.contains("LEFT JOIN LATERAL") || (l.contains("JSON_ARRAYAGG") && l.contains("JSON_OBJECT"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're not using JOINs to resolve relations on MySQL, this had to be changed with a loose and worse heuristic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine. Should we maybe now rename the function to something else? Same problem as #4639 (comment) I guess.
let exclusions = exclude | ||
.iter() | ||
.filter_map(|c| ConnectorVersion::try_from(*c).ok()) | ||
.map(|c| ConnectorVersion::try_from(*c).unwrap()) | ||
.collect::<Vec<_>>(); | ||
|
||
let inclusions = only | ||
.iter() | ||
.filter_map(|c| ConnectorVersion::try_from(*c).ok()) | ||
.map(|c| ConnectorVersion::try_from(*c).unwrap()) | ||
.collect::<Vec<_>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change to make sure that when a version is specified for the only
/ exclude
attributes in the test suite and this version is incorrectly specified, it errors instead of silently ignoring it. Got bitten by it multiple times when working on this PR.
if version.is_mysql() && !matches!(version, ConnectorVersion::MySql(Some(MySqlVersion::V8))) { | ||
excluded_features.push(format!(r#""{}""#, PreviewFeature::RelationJoins)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid having to exclude hundreds of tests for MySQL 5.6 & 5.7, we simply do not render the relationJoins
preview feature for now when rendering the datamodel of each tests. This ensure the relationLoadStrategy: join
is never used.
@@ -152,7 +152,7 @@ impl IntoBson for (&MongoDbType, PrismaValue) { | |||
|
|||
// Double | |||
(MongoDbType::Double, PrismaValue::Int(i)) => Bson::Double(i as f64), | |||
(MongoDbType::Double, PrismaValue::Float(f)) => Bson::Double(f.to_f64().convert(expl::MONGO_DOUBLE)?), | |||
(MongoDbType::Double, PrismaValue::Float(f)) => bigdecimal_to_bson_double(f)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side-effect of ScalarField::native_type
always returning a native type now. This is just a bug-fix that had not been caught before.
(MongoDbType::Json, PrismaValue::Json(json)) => { | ||
let val: Value = serde_json::from_str(&json)?; | ||
|
||
Bson::try_from(val).map_err(|_| MongoError::ConversionError { | ||
from: "Stringified JSON".to_owned(), | ||
to: "Mongo BSON (extJSON)".to_owned(), | ||
})? | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
TypeIdentifier::Boolean => { | ||
let err = | ||
|| build_conversion_error(sf, &format!("Number({n})"), &format!("{:?}", sf.type_identifier())); | ||
let i = n.as_i64().ok_or_else(err)?; | ||
|
||
match i { | ||
0 => Ok(PrismaValue::Boolean(false)), | ||
1 => Ok(PrismaValue::Boolean(true)), | ||
_ => Err(err()), | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MySQL-specific coercion for tinyint (IIRC)
|
||
pub(crate) fn build( | ||
pub(crate) trait JoinSelectBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To deal with the differences of PG & MySQL (lateral join vs correlated sub-queries), I've went for a trait design with a bunch of unimplemented methods that handle the few differences that we have between both connectors.
It has grown a bit too much to my taste already with the addition of _count
support, but I can't think of a cleaner way to avoid huge code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the downside, it made the code harder to follow.
let middle_take = match connector_flavour(&rs.args) { | ||
// On MySQL, using LIMIT makes the ordering of the JSON_AGG working. Beware, this is undocumented behavior. | ||
// Note: Ideally, this should live in the MySQL select builder, but it's currently the only implementation difference | ||
// between MySQL and Postgres, so we keep it here for now to avoid code duplication. | ||
Flavour::Mysql if !rs.args.order_by.is_empty() => rs.args.take_abs().or(Some(i64::MAX)), | ||
_ => rs.args.take_abs(), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🫠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial review, I'll finish reading tomorrow (haven't gotten to the most important part yet)
query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/data_types/native/mysql.rs
Outdated
Show resolved
Hide resolved
query-engine/connectors/sql-query-connector/src/database/operations/coerce.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
query-engine/connectors/sql-query-connector/src/query_builder/select/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick call with Flavian to go over the key parts. LGTM, great work!
Overview
closes https://github.com/prisma/team-orm/issues/455
Adds support for joins on MySQL 8.0, through correlated sub-queries. Also works with PlanetScale. Does not work with MySQL 5.6, MySQL 5.7 and MariaDB.
Here are examples for 1-1, 1-m and m-n relations between a
User
andPost
model:1-1
1-m
m-n
Note: When fetching to-many relations, if any ordering is present, we add a limit of
i64::MAX
which inexplicably forces MySQL to order the aggregated rows.