From b93c7f109917975d6b4e73f123e09f320247c23c Mon Sep 17 00:00:00 2001 From: Flavian Desverne Date: Thu, 18 Jan 2024 17:05:35 +0100 Subject: [PATCH] fix one2m & m2m ordering (using limit hack) --- .../cockroach_datamodel_connector.rs | 3 +-- .../postgres_datamodel_connector.rs | 3 +-- .../src/datamodel_connector/capabilities.rs | 1 - .../src/query_builder/select.rs | 22 +++++++++++++++++-- .../core/src/query_graph_builder/read/many.rs | 1 - .../core/src/query_graph_builder/read/one.rs | 1 - .../src/query_graph_builder/read/utils.rs | 11 +--------- 7 files changed, 23 insertions(+), 19 deletions(-) diff --git a/psl/psl-core/src/builtin_connectors/cockroach_datamodel_connector.rs b/psl/psl-core/src/builtin_connectors/cockroach_datamodel_connector.rs index 84968b15a6f0..03b312ba3574 100644 --- a/psl/psl-core/src/builtin_connectors/cockroach_datamodel_connector.rs +++ b/psl/psl-core/src/builtin_connectors/cockroach_datamodel_connector.rs @@ -62,8 +62,7 @@ const CAPABILITIES: ConnectorCapabilities = enumflags2::make_bitflags!(Connector RowIn | DeleteReturning | SupportsFiltersOnRelationsWithoutJoins | - LateralJoin | - LateralJoinM2MOrdering + LateralJoin }); const SCALAR_TYPE_DEFAULTS: &[(ScalarType, CockroachType)] = &[ diff --git a/psl/psl-core/src/builtin_connectors/postgres_datamodel_connector.rs b/psl/psl-core/src/builtin_connectors/postgres_datamodel_connector.rs index 7fd56b2075fc..4da85fa89861 100644 --- a/psl/psl-core/src/builtin_connectors/postgres_datamodel_connector.rs +++ b/psl/psl-core/src/builtin_connectors/postgres_datamodel_connector.rs @@ -70,8 +70,7 @@ const CAPABILITIES: ConnectorCapabilities = enumflags2::make_bitflags!(Connector DistinctOn | DeleteReturning | SupportsFiltersOnRelationsWithoutJoins | - LateralJoin | - LateralJoinM2MOrdering + LateralJoin }); pub struct PostgresDatamodelConnector; diff --git a/psl/psl-core/src/datamodel_connector/capabilities.rs b/psl/psl-core/src/datamodel_connector/capabilities.rs index 0094c0d164a6..8f53920c8edd 100644 --- a/psl/psl-core/src/datamodel_connector/capabilities.rs +++ b/psl/psl-core/src/datamodel_connector/capabilities.rs @@ -108,7 +108,6 @@ capabilities!( DeleteReturning, // Connector supports deleting records and returning them in one operation. SupportsFiltersOnRelationsWithoutJoins, // Connector supports rendering filters on relation fields without joins. LateralJoin, // Connector supports lateral joins to resolve relations. - LateralJoinM2MOrdering // Connector supports ordering M2M relations with lateral joins. ); /// Contains all capabilities that the connector is able to serve. diff --git a/query-engine/connectors/sql-query-connector/src/query_builder/select.rs b/query-engine/connectors/sql-query-connector/src/query_builder/select.rs index 5e229bc10108..42ad5480e637 100644 --- a/query-engine/connectors/sql-query-connector/src/query_builder/select.rs +++ b/query-engine/connectors/sql-query-connector/src/query_builder/select.rs @@ -1,3 +1,4 @@ +use psl::datamodel_connector::Flavour; use std::borrow::Cow; use tracing::Span; @@ -133,6 +134,12 @@ impl SelectBuilder { let inner = inner.with_columns(inner_selection.into()).comment("inner select"); + let take = match is_mysql(rs) { + // On MySQL, using LIMIT makes the ordering of the JSON_AGG working. Beware, this is undocumented behavior. + true => rs.args.take_abs().or(Some(i64::MAX)), + false => rs.args.take_abs(), + }; + let middle = Select::from_table(Table::from(inner).alias(inner_alias.to_table_string())) // SELECT . .column(Column::from((inner_alias.to_table_string(), JSON_AGG_IDENT))) @@ -141,7 +148,7 @@ impl SelectBuilder { // WHERE ... .with_filters(rs.args.filter.clone(), Some(inner_alias), ctx) // LIMIT $1 OFFSET $2 - .with_pagination(rs.args.take_abs(), rs.args.skip) + .with_pagination(take, rs.args.skip) .comment("middle select"); // SELECT COALESCE(JSON_AGG(), '[]') AS FROM ( ) as @@ -181,6 +188,11 @@ impl SelectBuilder { .lateral(); let child_table = rf.as_table(ctx).alias(m2m_table_alias.to_table_string()); + let take = match is_mysql(rs) { + // On MySQL, using LIMIT makes the ordering of the JSON_AGG working. Beware, this is undocumented behavior. + true => rs.args.take_abs().or(Some(i64::MAX)), + false => rs.args.take_abs(), + }; let inner = Select::from_table(child_table) .value(Column::from((m2m_join_alias.to_table_string(), JSON_AGG_IDENT))) @@ -188,7 +200,7 @@ impl SelectBuilder { .and_where(join_conditions) // adds join condition to the child table .with_ordering(&rs.args, Some(m2m_join_alias.to_table_string()), ctx) // adds ordering stmts .with_filters(rs.args.filter.clone(), Some(m2m_join_alias), ctx) // adds query filters // TODO: avoid clone filter - .with_pagination(rs.args.take_abs(), rs.args.skip) + .with_pagination(take, rs.args.skip) .comment("inner"); // adds pagination let outer = Select::from_table(Table::from(inner).alias(outer_alias.to_table_string())) @@ -415,3 +427,9 @@ fn json_agg() -> Function<'static> { fn empty_json_array() -> serde_json::Value { serde_json::Value::Array(Vec::new()) } + +// TODO: Hack to get around the fact that we don't have a way to know if we're on mysql or not +// TODO: Remove this once we have a proper way to know the connector type +fn is_mysql(rs: &RelationSelection) -> bool { + rs.args.model().dm.schema.connector.flavour() == Flavour::Mysql +} diff --git a/query-engine/core/src/query_graph_builder/read/many.rs b/query-engine/core/src/query_graph_builder/read/many.rs index b6e27f8cea9b..3a462588f957 100644 --- a/query-engine/core/src/query_graph_builder/read/many.rs +++ b/query-engine/core/src/query_graph_builder/read/many.rs @@ -45,7 +45,6 @@ fn find_many_with_options( args.cursor.as_ref(), args.distinct.as_ref(), &nested, - &selected_fields, &aggregation_selections, query_schema, ); diff --git a/query-engine/core/src/query_graph_builder/read/one.rs b/query-engine/core/src/query_graph_builder/read/one.rs index 06c890db410c..97b2e13e3f93 100644 --- a/query-engine/core/src/query_graph_builder/read/one.rs +++ b/query-engine/core/src/query_graph_builder/read/one.rs @@ -58,7 +58,6 @@ fn find_unique_with_options( None, None, &nested, - &selected_fields, &aggregation_selections, query_schema, ); diff --git a/query-engine/core/src/query_graph_builder/read/utils.rs b/query-engine/core/src/query_graph_builder/read/utils.rs index 4f3ee2a2ca48..e59c4dab0b3d 100644 --- a/query-engine/core/src/query_graph_builder/read/utils.rs +++ b/query-engine/core/src/query_graph_builder/read/utils.rs @@ -247,18 +247,9 @@ pub(crate) fn get_relation_load_strategy( cursor: Option<&SelectionResult>, distinct: Option<&FieldSelection>, nested_queries: &[ReadQuery], - field_selection: &FieldSelection, aggregation_selections: &[RelAggregationSelection], query_schema: &QuerySchema, ) -> RelationLoadStrategy { - // MySQL does not support M2M ordering with lateral joins. - // We fallback to the query-based strategy when a m2m relation with ordering is present. - let has_ordered_m2m = field_selection - .relations() - .any(|rf| rf.field.relation().is_many_to_many() && !rf.args.order_by.is_empty()); - let can_order_m2m = query_schema.has_capability(ConnectorCapability::LateralJoinM2MOrdering); - let supports_m2m_ordering = !has_ordered_m2m || can_order_m2m; - if query_schema.has_feature(PreviewFeature::RelationJoins) && query_schema.has_capability(ConnectorCapability::LateralJoin) && cursor.is_none() @@ -268,7 +259,7 @@ pub(crate) fn get_relation_load_strategy( ReadQuery::RelatedRecordsQuery(q) => q.has_cursor() || q.has_distinct() || q.has_aggregation_selections(), _ => false, }) - && supports_m2m_ordering + // && supports_m2m_ordering && requested_strategy != Some(RelationLoadStrategy::Query) { RelationLoadStrategy::Join