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 reportAfter 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.
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.
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.
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.
| (ScalarType::Boolean, MongoDbType::Bool), | ||
| (ScalarType::String, MongoDbType::String), | ||
| (ScalarType::DateTime, MongoDbType::Timestamp), | ||
| (ScalarType::DateTime, MongoDbType::Date), |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
| // 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.
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(), | ||
| })? | ||
| } |
| 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.
MySQL-specific coercion for tinyint (IIRC)
| } | ||
|
|
||
| pub(crate) fn build( | ||
| pub(crate) trait JoinSelectBuilder { |
There was a problem hiding this comment.
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.
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(), | ||
| }; |
aqrln
left a comment
There was a problem hiding this comment.
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
query-engine/connectors/sql-query-connector/src/query_builder/select/mod.rs
Outdated
Show resolved
Hide resolved
laplab
left a comment
There was a problem hiding this comment.
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
UserandPostmodel:1-1
{ findManyPost { id title user { id name } } }1-m
{ findManyUser { id name posts(orderBy: { id: asc }) { id title } } }m-n
{ findManyUser { id name posts(orderBy: { id: asc }) { id title } } }Note: When fetching to-many relations, if any ordering is present, we add a limit of
i64::MAXwhich inexplicably forces MySQL to order the aggregated rows.