From 57bd41f8e6076b7d86d1899b177ede9645cb384d Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Mon, 11 Sep 2023 14:39:11 +0200 Subject: [PATCH 01/20] add distinct db capability --- psl/builtin-connectors/src/postgres_datamodel_connector.rs | 3 ++- psl/psl-core/src/datamodel_connector/capabilities.rs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/psl/builtin-connectors/src/postgres_datamodel_connector.rs b/psl/builtin-connectors/src/postgres_datamodel_connector.rs index 8fac79165c58..52568dd8fab3 100644 --- a/psl/builtin-connectors/src/postgres_datamodel_connector.rs +++ b/psl/builtin-connectors/src/postgres_datamodel_connector.rs @@ -65,7 +65,8 @@ const CAPABILITIES: ConnectorCapabilities = enumflags2::make_bitflags!(Connector NativeUpsert | InsertReturning | UpdateReturning | - RowIn + RowIn | + Distinct }); pub struct PostgresDatamodelConnector; diff --git a/psl/psl-core/src/datamodel_connector/capabilities.rs b/psl/psl-core/src/datamodel_connector/capabilities.rs index 1b3f557e6285..5ae70e933574 100644 --- a/psl/psl-core/src/datamodel_connector/capabilities.rs +++ b/psl/psl-core/src/datamodel_connector/capabilities.rs @@ -103,7 +103,8 @@ capabilities!( NativeUpsert, InsertReturning, UpdateReturning, - RowIn, // Connector supports (a, b) IN (c, d) expression. + RowIn, // Connector supports (a, b) IN (c, d) expression. + Distinct // Connector supports DB-level distinct (e.g. postgres) ); /// Contains all capabilities that the connector is able to serve. From 16ec52639aaba5d12e7bc8e00f189cfb029c034e Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Mon, 11 Sep 2023 14:57:46 +0200 Subject: [PATCH 02/20] wip distinct typing --- quaint/src/ast.rs | 2 +- quaint/src/ast/select.rs | 15 +++++++++++++-- quaint/src/visitor.rs | 12 +++++++++--- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/quaint/src/ast.rs b/quaint/src/ast.rs index dc634423014a..b2b96fc16654 100644 --- a/quaint/src/ast.rs +++ b/quaint/src/ast.rs @@ -49,7 +49,7 @@ pub use ordering::{IntoOrderDefinition, Order, OrderDefinition, Orderable, Order pub use over::*; pub use query::{Query, SelectQuery}; pub use row::Row; -pub use select::Select; +pub use select::{DistinctType, Select}; pub use table::*; pub use union::Union; pub use update::*; diff --git a/quaint/src/ast/select.rs b/quaint/src/ast/select.rs index 96d50ba645c5..0c798d5227a0 100644 --- a/quaint/src/ast/select.rs +++ b/quaint/src/ast/select.rs @@ -4,7 +4,7 @@ use std::borrow::Cow; /// A builder for a `SELECT` statement. #[derive(Debug, PartialEq, Clone, Default)] pub struct Select<'a> { - pub(crate) distinct: bool, + pub(crate) distinct: Option>, pub(crate) tables: Vec>, pub(crate) columns: Vec>, pub(crate) conditions: Option>, @@ -18,6 +18,12 @@ pub struct Select<'a> { pub(crate) comment: Option>, } +#[derive(Debug, PartialEq, Clone)] +pub enum DistinctType<'a> { + Default, + OnClause(Vec>), +} + impl<'a> From> for Expression<'a> { fn from(sel: Select<'a>) -> Expression<'a> { Expression { @@ -236,7 +242,12 @@ impl<'a> Select<'a> { /// # } /// ``` pub fn distinct(mut self) -> Self { - self.distinct = true; + self.distinct = Some(DistinctType::Default); + self + } + + pub fn distinct_on(mut self, columns: Vec>) -> Self { + self.distinct = Some(DistinctType::OnClause(columns)); self } diff --git a/quaint/src/visitor.rs b/quaint/src/visitor.rs index c205b49dd279..18dee697722f 100644 --- a/quaint/src/visitor.rs +++ b/quaint/src/visitor.rs @@ -234,9 +234,15 @@ pub trait Visitor<'a> { self.write("SELECT ")?; - if select.distinct { - self.write("DISTINCT ")?; - } + if let Some(distinct) = select.distinct { + match distinct { + DistinctType::Default => self.write("DISTINCT ")?, + DistinctType::OnClause(columns) => { + self.write("DISTINCT ON ")?; + self.surround_with("(", ") ", |ref mut s| s.visit_columns(columns))?; + } + } + }; if !select.tables.is_empty() { if select.columns.is_empty() { From 4f2e7eadb75e16432aba3f617c7bf6946852a5a7 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Mon, 11 Sep 2023 16:12:58 +0200 Subject: [PATCH 03/20] distinct_on rendering in quaint --- quaint/src/visitor/postgres.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/quaint/src/visitor/postgres.rs b/quaint/src/visitor/postgres.rs index 648b3f0dc1ec..6305136df9a7 100644 --- a/quaint/src/visitor/postgres.rs +++ b/quaint/src/visitor/postgres.rs @@ -785,6 +785,18 @@ mod tests { assert_eq!(expected_sql, sql); } + #[test] + fn test_distinct_on() { + let expected_sql = "SELECT DISTINCT ON (\"bar\") \"bar\" FROM \"test\""; + let query = Select::from_table("test") + .column(Column::new("bar")) + .distinct_on(vec![Expression::from(Column::from("bar"))]); + + let (sql, _) = Postgres::build(query).unwrap(); + + assert_eq!(expected_sql, sql); + } + #[test] fn test_distinct_with_subquery() { let expected_sql = "SELECT DISTINCT (SELECT $1 FROM \"test2\"), \"bar\" FROM \"test\""; From deee44dc40037b619d0a3c306764dc320caec9cc Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Mon, 11 Sep 2023 16:19:15 +0200 Subject: [PATCH 04/20] rendering --- quaint/src/visitor/postgres.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/quaint/src/visitor/postgres.rs b/quaint/src/visitor/postgres.rs index 6305136df9a7..749b752709d4 100644 --- a/quaint/src/visitor/postgres.rs +++ b/quaint/src/visitor/postgres.rs @@ -787,10 +787,11 @@ mod tests { #[test] fn test_distinct_on() { - let expected_sql = "SELECT DISTINCT ON (\"bar\") \"bar\" FROM \"test\""; - let query = Select::from_table("test") - .column(Column::new("bar")) - .distinct_on(vec![Expression::from(Column::from("bar"))]); + let expected_sql = "SELECT DISTINCT ON (\"bar\", \"foo\") \"bar\" FROM \"test\""; + let query = Select::from_table("test").column(Column::new("bar")).distinct_on(vec![ + Expression::from(Column::from("bar")), + Expression::from(Column::from("foo")), + ]); let (sql, _) = Postgres::build(query).unwrap(); From 4c5e39a49f81bf410600b8af11a4f1e2c1b1b517 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Tue, 12 Sep 2023 17:15:30 +0200 Subject: [PATCH 05/20] Breaking tests --- .../src/query_builder/read.rs | 11 +++++++ .../interpreter/query_interpreters/read.rs | 31 ++++++++++++++----- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/query-engine/connectors/sql-query-connector/src/query_builder/read.rs b/query-engine/connectors/sql-query-connector/src/query_builder/read.rs index 3aa91288ea90..b3fbb548b64c 100644 --- a/query-engine/connectors/sql-query-connector/src/query_builder/read.rs +++ b/query-engine/connectors/sql-query-connector/src/query_builder/read.rs @@ -106,6 +106,17 @@ impl SelectDefinition for QueryArguments { .iter() .fold(select_ast, |acc, o| acc.order_by(o.order_definition.clone())); + let select_ast = if let Some(distinct) = self.distinct { + let distinct_fields = ModelProjection::from(distinct) + .as_columns(ctx) + .map(Expression::from) + .collect_vec(); + + select_ast.distinct_on(distinct_fields) + } else { + select_ast + }; + match limit { Some(limit) => (select_ast.limit(limit as usize), aggregation_joins.columns), None => (select_ast, aggregation_joins.columns), diff --git a/query-engine/core/src/interpreter/query_interpreters/read.rs b/query-engine/core/src/interpreter/query_interpreters/read.rs index 464ac6651677..3698e22368da 100644 --- a/query-engine/core/src/interpreter/query_interpreters/read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/read.rs @@ -1,8 +1,7 @@ -use super::*; +use super::{inmemory_record_processor::InMemoryRecordProcessor, *}; use crate::{interpreter::InterpretationResult, query_ast::*, result_ast::*}; use connector::{self, error::ConnectorError, ConnectionLike, RelAggregationRow, RelAggregationSelection}; use futures::future::{BoxFuture, FutureExt}; -use inmemory_record_processor::InMemoryRecordProcessor; use query_structure::ManyRecords; use std::collections::HashMap; use user_facing_errors::KnownError; @@ -86,14 +85,30 @@ fn read_one( /// -> Unstable cursors can't reliably be fetched by the underlying datasource, so we need to process part of it in-memory. fn read_many( tx: &mut dyn ConnectionLike, - mut query: ManyRecordsQuery, + query: ManyRecordsQuery, trace_id: Option, ) -> BoxFuture<'_, InterpretationResult> { - let processor = if query.args.requires_inmemory_processing() { - Some(InMemoryRecordProcessor::new_from_query_args(&mut query.args)) - } else { - None - }; + // let req_inmem_proc = query.args.requires_inmemory_processing(); + + // let processor = if req_inmem_proc { + // let inm_builder = InMemoryRecordProcessorBuilder::new( + // query.args.model.clone(), + // query.args.order_by.clone(), + // query.args.ignore_skip.clone(), + // query.args.ignore_take.clone(), + // ); + + // let inmemory_record_processor = match query.args.distinct { + // Some(ref fs) => inm_builder.distinct(fs.clone()).build(), + // None => inm_builder.build(), + // }; + + // Some(inmemory_record_processor) + // } else { + // None + // }; + + let processor: Option = None; let fut = async move { let scalars = tx From 67cbbbce161dd1cf538dda49a3901fa370e9baab Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Tue, 12 Sep 2023 17:23:28 +0200 Subject: [PATCH 06/20] Test Highlight: queries::distinct::distinct::with_duplicates --- .../query-engine-tests/tests/queries/distinct.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs index 55ab265d22eb..f40c495cbdc3 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs @@ -57,7 +57,7 @@ mod distinct { assert_query!( runner, - "query { findManyUser(distinct: [first_name, last_name]) { id, first_name, last_name } }", + "query { findManyUser(distinct: [first_name, last_name], orderBy: {id: asc}) { id, first_name, last_name } }", r#"{"data":{"findManyUser":[{"id":1,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# ); From 143988ecf589e61bd7ab7e7ee510a28198ffd85d Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Wed, 13 Sep 2023 18:26:41 +0200 Subject: [PATCH 07/20] Add builder for inmemproc Add capability for db distinct in: - read_many - m2m - one2m --- .../inmemory_record_processor.rs | 52 +++++++++++++++++- .../query_interpreters/nested_read.rs | 53 +++++++++++++++++-- .../interpreter/query_interpreters/read.rs | 52 ++++++++++-------- .../query-structure/src/query_arguments.rs | 14 ++++- 4 files changed, 144 insertions(+), 27 deletions(-) diff --git a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs index f4c0465412e2..b2a605ef5872 100644 --- a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs +++ b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs @@ -79,7 +79,7 @@ impl InMemoryRecordProcessor { records.records.first().map(|x| x.parent_id.is_some()).unwrap_or(false) } - fn apply_distinct(&self, mut records: ManyRecords) -> ManyRecords { + pub(crate) fn apply_distinct(&self, mut records: ManyRecords) -> ManyRecords { let field_names = &records.field_names; let distinct_selection = if let Some(ref distinct) = self.distinct { @@ -188,3 +188,53 @@ impl InMemoryRecordProcessor { self.take.or(self.skip).is_some() || self.cursor.is_some() } } + +pub struct InMemoryRecordProcessorBuilder { + args: QueryArguments, +} + +impl InMemoryRecordProcessorBuilder { + pub fn new(model: Model, order_by: Vec, ignore_skip: bool, ignore_take: bool) -> Self { + Self { + args: QueryArguments { + model, + cursor: None, + take: None, + skip: None, + filter: None, + order_by, + distinct: None, + ignore_skip, + ignore_take, + }, + } + } + pub fn _cursor(mut self, cursor: SelectionResult) -> Self { + self.args.cursor = Some(cursor); + self + } + + pub fn _take(mut self, take: i64) -> Self { + self.args.take = Some(take); + self + } + + pub fn _skip(mut self, skip: i64) -> Self { + self.args.skip = Some(skip); + self + } + + pub fn _filter(mut self, filter: Filter) -> Self { + self.args.filter = Some(filter); + self + } + + pub fn distinct(mut self, distinct: FieldSelection) -> Self { + self.args.distinct = Some(distinct); + self + } + + pub fn build(self) -> InMemoryRecordProcessor { + InMemoryRecordProcessor { args: self.args } + } +} diff --git a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs index fa4dc7c6e529..4c39369a0ee9 100644 --- a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs @@ -1,4 +1,4 @@ -use super::{inmemory_record_processor::InMemoryRecordProcessor, read}; +use super::{inmemory_record_processor::InMemoryRecordProcessorBuilder, read}; use crate::{interpreter::InterpretationResult, query_ast::*}; use connector::{self, ConnectionLike, RelAggregationRow, RelAggregationSelection}; use query_structure::*; @@ -10,7 +10,25 @@ pub(crate) async fn m2m( parent_result: Option<&ManyRecords>, trace_id: Option, ) -> InterpretationResult<(ManyRecords, Option>)> { - let processor = InMemoryRecordProcessor::new_from_query_args(&mut query.args); + let inm_builder = InMemoryRecordProcessorBuilder::new( + query.args.model.clone(), + query.args.order_by.clone(), + query.args.ignore_skip, + query.args.ignore_take, + ); + + let processor = match query.args.distinct { + Some(ref fs) => { + if query.args.requires_inmemory_distinct() { + inm_builder.distinct(fs.clone()) + } else { + inm_builder + } + } + None => inm_builder, + } + .build(); + let parent_field = &query.parent_field; let child_link_id = parent_field.related_field().linking_fields(); @@ -135,7 +153,7 @@ pub async fn one2m( parent_field: &RelationFieldRef, parent_selections: Option>, parent_result: Option<&ManyRecords>, - mut query_args: QueryArguments, + query_args: QueryArguments, selected_fields: &FieldSelection, aggr_selections: Vec, trace_id: Option, @@ -189,10 +207,31 @@ pub async fn one2m( // If we're fetching related records from a single parent, then we can apply normal pagination instead of in-memory processing. // However, we can't just apply a LIMIT/OFFSET for multiple parents as we need N related records PER parent. // We could use ROW_NUMBER() but it requires further refactoring so we're still using in-memory processing for now. + + let req_inmem_distinct = query_args.requires_inmemory_distinct(); + let processor = if uniq_selections.len() == 1 && !query_args.requires_inmemory_processing() { None } else { - Some(InMemoryRecordProcessor::new_from_query_args(&mut query_args)) + let inm_builder = InMemoryRecordProcessorBuilder::new( + query_args.model.clone(), + query_args.order_by.clone(), + query_args.ignore_skip, + query_args.ignore_take, + ); + + let inm_builder = match query_args.distinct { + Some(ref fs) => { + if req_inmem_distinct { + inm_builder.distinct(fs.clone()) + } else { + inm_builder + } + } + None => inm_builder, + }; + + Some(inm_builder.build()) }; let mut scalars = { @@ -262,6 +301,12 @@ pub async fn one2m( } let scalars = if let Some(processor) = processor { + let scalars = if req_inmem_distinct { + processor.apply_distinct(scalars) + } else { + scalars + }; + processor.apply(scalars) } else { scalars diff --git a/query-engine/core/src/interpreter/query_interpreters/read.rs b/query-engine/core/src/interpreter/query_interpreters/read.rs index 3698e22368da..c653e11418ee 100644 --- a/query-engine/core/src/interpreter/query_interpreters/read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/read.rs @@ -1,4 +1,4 @@ -use super::{inmemory_record_processor::InMemoryRecordProcessor, *}; +use super::{inmemory_record_processor::InMemoryRecordProcessorBuilder, *}; use crate::{interpreter::InterpretationResult, query_ast::*, result_ast::*}; use connector::{self, error::ConnectorError, ConnectionLike, RelAggregationRow, RelAggregationSelection}; use futures::future::{BoxFuture, FutureExt}; @@ -88,27 +88,31 @@ fn read_many( query: ManyRecordsQuery, trace_id: Option, ) -> BoxFuture<'_, InterpretationResult> { - // let req_inmem_proc = query.args.requires_inmemory_processing(); - - // let processor = if req_inmem_proc { - // let inm_builder = InMemoryRecordProcessorBuilder::new( - // query.args.model.clone(), - // query.args.order_by.clone(), - // query.args.ignore_skip.clone(), - // query.args.ignore_take.clone(), - // ); - - // let inmemory_record_processor = match query.args.distinct { - // Some(ref fs) => inm_builder.distinct(fs.clone()).build(), - // None => inm_builder.build(), - // }; - - // Some(inmemory_record_processor) - // } else { - // None - // }; + let req_inmem_distinct = query.args.requires_inmemory_distinct(); + + let processor = if query.args.requires_inmemory_processing() { + let inm_builder = InMemoryRecordProcessorBuilder::new( + query.args.model.clone(), + query.args.order_by.clone(), + query.args.ignore_skip, + query.args.ignore_take, + ); + + let inm_builder = match query.args.distinct { + Some(ref fs) => { + if req_inmem_distinct { + inm_builder.distinct(fs.clone()) + } else { + inm_builder + } + } + None => inm_builder, + }; - let processor: Option = None; + Some(inm_builder.build()) + } else { + None + }; let fut = async move { let scalars = tx @@ -122,6 +126,12 @@ fn read_many( .await?; let scalars = if let Some(p) = processor { + let scalars = if req_inmem_distinct { + p.apply_distinct(scalars) + } else { + scalars + }; + p.apply(scalars) } else { scalars diff --git a/query-engine/query-structure/src/query_arguments.rs b/query-engine/query-structure/src/query_arguments.rs index f9c222d80dbe..83b432ac91dd 100644 --- a/query-engine/query-structure/src/query_arguments.rs +++ b/query-engine/query-structure/src/query_arguments.rs @@ -1,3 +1,5 @@ +use psl::datamodel_connector::ConnectorCapability; + use crate::*; /// `QueryArguments` define various constraints queried data should fulfill: @@ -70,7 +72,17 @@ impl QueryArguments { /// retrieved by the connector or if it requires the query engine to fetch a raw set /// of records and perform certain operations itself, in-memory. pub fn requires_inmemory_processing(&self) -> bool { - self.distinct.is_some() || self.contains_unstable_cursor() || self.contains_null_cursor() + self.contains_unstable_cursor() || self.contains_null_cursor() + } + + pub fn requires_inmemory_distinct(&self) -> bool { + self.distinct.is_some() + && !self + .model() + .dm + .schema + .connector + .has_capability(ConnectorCapability::Distinct) } /// An unstable cursor is a cursor that is used in conjunction with an unstable (non-unique) combination of orderBys. From 40a991736ff6f89acecf4723caba8c1a9c552544 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Wed, 13 Sep 2023 18:27:07 +0200 Subject: [PATCH 08/20] Update tests (wip) --- .../tests/queries/distinct.rs | 155 ++++++++++++------ 1 file changed, 103 insertions(+), 52 deletions(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs index f40c495cbdc3..aff7845eee90 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs @@ -3,14 +3,19 @@ use query_engine_tests::*; #[test_suite(schema(schemas::user_posts))] mod distinct { use indoc::indoc; - use query_engine_tests::assert_query; + use query_engine_tests::run_query; #[connector_test] async fn empty_database(runner: Runner) -> TestResult<()> { - assert_query!( - runner, - "query { findManyUser(distinct: [first_name, last_name]) { id, first_name, last_name } }", - r#"{"data":{"findManyUser":[]}}"# + insta::assert_snapshot!( + run_query!( + &runner, + indoc!("{ + findManyUser(distinct: [first_name, last_name]) + { id, first_name, last_name } + }") + ), + @r###"{"data":{"findManyUser":[]}}"### ); Ok(()) @@ -22,10 +27,16 @@ mod distinct { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user(&runner, r#"{ id: 2, first_name: "Doe", last_name: "Joe", email: "2" }"#).await?; - assert_query!( - runner, - "query { findManyUser(distinct: [first_name, last_name]) { id } }", - r#"{"data":{"findManyUser":[{"id":1},{"id":2}]}}"# + insta::assert_snapshot!( + run_query!( + &runner, + indoc!("{ + findManyUser(distinct: [first_name, last_name]) + { id } + }") + ), + // r#"{"data":{"findManyUser":[{"id":1},{"id":2}]}}"# + @r###"{"data":{"findManyUser":[{"id":2},{"id":1}]}}"### ); Ok(()) @@ -36,10 +47,15 @@ mod distinct { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user(&runner, r#"{ id: 2, first_name: "Joe", last_name: "Doe", email: "2" }"#).await?; - assert_query!( - runner, - "query { findManyUser(distinct: first_name) { id } }", - r#"{"data":{"findManyUser":[{"id":1}]}}"# + insta::assert_snapshot!( + run_query!( + &runner, + indoc!("{ + findManyUser(distinct: first_name) + { id } + }") + ), + @r###"{"data":{"findManyUser":[{"id":1}]}}"### ); Ok(()) @@ -55,10 +71,14 @@ mod distinct { .await?; test_user(&runner, r#"{ id: 3, first_name: "Joe", last_name: "Doe", email: "3" }"#).await?; - assert_query!( - runner, - "query { findManyUser(distinct: [first_name, last_name], orderBy: {id: asc}) { id, first_name, last_name } }", - r#"{"data":{"findManyUser":[{"id":1,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# + insta::assert_snapshot!(run_query!( + &runner, + indoc!("{ + findManyUser(distinct: [first_name, last_name]) + { id, first_name, last_name } + }") + ), + @r###"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"},{"id":1,"first_name":"Joe","last_name":"Doe"}]}}"### ); Ok(()) @@ -74,10 +94,17 @@ mod distinct { .await?; test_user(&runner, r#"{ id: 3, first_name: "Joe", last_name: "Doe", email: "3" }"#).await?; - assert_query!( - runner, - "query { findManyUser(skip: 1, distinct: [first_name, last_name]) { id, first_name, last_name } }", - r#"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# + insta::assert_snapshot!( + run_query!( + &runner, + indoc!("{ + findManyUser(skip: 1, distinct: [first_name, last_name]) + { id, first_name, last_name } + }") + ), + // r#"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# + // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions + @r###""### ); Ok(()) @@ -93,10 +120,18 @@ mod distinct { .await?; test_user(&runner, r#"{ id: 3, first_name: "Joe", last_name: "Doe", email: "3" }"#).await?; - assert_query!( - runner, - "query { findManyUser(orderBy: { first_name: asc }, skip: 1, distinct: [first_name, last_name]) { first_name, last_name } }", - r#"{"data":{"findManyUser":[{"first_name":"Joe","last_name":"Doe"}]}}"# + insta::assert_snapshot!( + run_query!( + &runner, + indoc!("{ + findManyUser( + orderBy: { first_name: asc }, + skip: 1, + distinct: [first_name, last_name]) + { first_name, last_name } + }") + ), + @r###"{"data":{"findManyUser":[{"first_name":"Joe","last_name":"Doe"}]}}"### ); Ok(()) @@ -112,17 +147,25 @@ mod distinct { .await?; test_user(&runner, r#"{ id: 3, first_name: "Joe", last_name: "Doe", email: "3" }"#).await?; - assert_query!( - runner, - "query { findManyUser(orderBy: { id: desc }, distinct: [first_name, last_name]) { id, first_name, last_name } }", - r#"{"data":{"findManyUser":[{"id":3,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# + insta::assert_snapshot!(run_query!( + &runner, + indoc!("{ + findManyUser( + orderBy: { id: desc }, + distinct: [first_name, last_name]) + { id, first_name, last_name } + }") + ), + // r#"{"data":{"findManyUser":[{"id":3,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# + // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions + @r###""### ); Ok(()) } /// Mut return only distinct records for top record, and only for those the distinct relation records. - #[connector_test] + // #[connector_test] async fn nested_distinct(runner: Runner) -> TestResult<()> { nested_dataset(&runner).await?; @@ -131,17 +174,20 @@ mod distinct { // 3 => [] // 4 => ["1"] // 5 => ["2", "3"] - assert_query!( - runner, - indoc! {"{ - findManyUser(distinct: [first_name, last_name]) { - id - posts(distinct: [title], orderBy: { id: asc }) { - title - } - } - }"}, - r#"{"data":{"findManyUser":[{"id":1,"posts":[{"title":"3"},{"title":"1"},{"title":"2"}]},{"id":3,"posts":[]},{"id":4,"posts":[{"title":"1"}]},{"id":5,"posts":[{"title":"2"},{"title":"3"}]}]}}"# + insta::assert_snapshot!(run_query!( + &runner, + indoc!("{ + findManyUser(distinct: [first_name, last_name]) + { + id + posts(distinct: [title], orderBy: { id: asc }) { + title + } + }}") + ), + // {"data":{"findManyUser":[{"id":1,"posts":[{"title":"3"},{"title":"1"},{"title":"2"}]},{"id":3,"posts":[]},{"id":4,"posts":[{"title":"1"}]},{"id":5,"posts":[{"title":"2"},{"title":"3"}]}]}} + // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions + @r###""### ); Ok(()) @@ -157,17 +203,22 @@ mod distinct { // 4 => ["1"] // 3 => [] // 2 => ["2", "1"] - assert_query!( - runner, - indoc! {"{ - findManyUser(distinct: [first_name, last_name], orderBy: { id: desc }) { - id - posts(distinct: [title], orderBy: { id: desc }) { - title - } - } - }"}, - r#"{"data":{"findManyUser":[{"id":5,"posts":[{"title":"2"},{"title":"3"}]},{"id":4,"posts":[{"title":"1"}]},{"id":3,"posts":[]},{"id":2,"posts":[{"title":"2"},{"title":"1"}]}]}}"# + insta::assert_snapshot!(run_query!( + &runner, + indoc! {"{ + findManyUser( + distinct: [first_name, last_name], + orderBy: { id: desc } + ) + { + id + posts(distinct: [title], orderBy: { id: desc }) { title } + } + }"} + ), + // {"data":{"findManyUser":[{"id":5,"posts":[{"title":"2"},{"title":"3"}]},{"id":4,"posts":[{"title":"1"}]},{"id":3,"posts":[]},{"id":2,"posts":[{"title":"2"},{"title":"1"}]}]}} + // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions + @r###""### ); Ok(()) From 611cc4ed1dcbf8e6e71b621cb20b8bfd57a12ecb Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Wed, 29 Nov 2023 01:40:12 +0100 Subject: [PATCH 09/20] Fixed - inmemory distinct will actually be used Added - capability for distinct inmem when orderby --- .../tests/queries/distinct.rs | 12 +++--- .../inmemory_record_processor.rs | 4 +- .../query_interpreters/nested_read.rs | 39 +++++++++---------- .../interpreter/query_interpreters/read.rs | 20 +++++----- .../query-structure/src/query_arguments.rs | 24 ++++++++---- 5 files changed, 53 insertions(+), 46 deletions(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs index aff7845eee90..bc52fb7cfdf6 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs @@ -85,7 +85,7 @@ mod distinct { } #[connector_test] - async fn with_skip(runner: Runner) -> TestResult<()> { + async fn with_skip_basic(runner: Runner) -> TestResult<()> { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user( &runner, @@ -104,7 +104,7 @@ mod distinct { ), // r#"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions - @r###""### + @r###"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"},{"id":3,"first_name":"Joe","last_name":"Doe"}]}}"### ); Ok(()) @@ -158,14 +158,14 @@ mod distinct { ), // r#"{"data":{"findManyUser":[{"id":3,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions - @r###""### + @r###"{"data":{"findManyUser":[{"id":3,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"### ); Ok(()) } /// Mut return only distinct records for top record, and only for those the distinct relation records. - // #[connector_test] + #[connector_test] async fn nested_distinct(runner: Runner) -> TestResult<()> { nested_dataset(&runner).await?; @@ -187,7 +187,7 @@ mod distinct { ), // {"data":{"findManyUser":[{"id":1,"posts":[{"title":"3"},{"title":"1"},{"title":"2"}]},{"id":3,"posts":[]},{"id":4,"posts":[{"title":"1"}]},{"id":5,"posts":[{"title":"2"},{"title":"3"}]}]}} // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions - @r###""### + @r###"{"data":{"findManyUser":[{"id":1,"posts":[{"title":"3"},{"title":"1"},{"title":"2"}]},{"id":4,"posts":[{"title":"1"}]},{"id":3,"posts":[]},{"id":5,"posts":[{"title":"2"},{"title":"3"}]}]}}"### ); Ok(()) @@ -218,7 +218,7 @@ mod distinct { ), // {"data":{"findManyUser":[{"id":5,"posts":[{"title":"2"},{"title":"3"}]},{"id":4,"posts":[{"title":"1"}]},{"id":3,"posts":[]},{"id":2,"posts":[{"title":"2"},{"title":"1"}]}]}} // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions - @r###""### + @r###"{"data":{"findManyUser":[{"id":5,"posts":[{"title":"2"},{"title":"3"}]},{"id":4,"posts":[{"title":"1"}]},{"id":3,"posts":[]},{"id":2,"posts":[{"title":"2"},{"title":"1"}]}]}}"### ); Ok(()) diff --git a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs index b2a605ef5872..b78e172a6c0e 100644 --- a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs +++ b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs @@ -229,8 +229,8 @@ impl InMemoryRecordProcessorBuilder { self } - pub fn distinct(mut self, distinct: FieldSelection) -> Self { - self.args.distinct = Some(distinct); + pub fn distinct(mut self, distinct: Option) -> Self { + self.args.distinct = distinct.clone(); self } diff --git a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs index 4c39369a0ee9..3a83595a7696 100644 --- a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs @@ -17,17 +17,16 @@ pub(crate) async fn m2m( query.args.ignore_take, ); - let processor = match query.args.distinct { - Some(ref fs) => { - if query.args.requires_inmemory_distinct() { - inm_builder.distinct(fs.clone()) - } else { - inm_builder - } - } - None => inm_builder, - } - .build(); + let inm_builder = if query.args.requires_inmemory_distinct() { + let inm_builder = inm_builder.distinct(query.args.distinct.clone()); + query.args.distinct = None; + + inm_builder + } else { + inm_builder + }; + + let processor = inm_builder.build(); let parent_field = &query.parent_field; let child_link_id = parent_field.related_field().linking_fields(); @@ -153,7 +152,7 @@ pub async fn one2m( parent_field: &RelationFieldRef, parent_selections: Option>, parent_result: Option<&ManyRecords>, - query_args: QueryArguments, + mut query_args: QueryArguments, selected_fields: &FieldSelection, aggr_selections: Vec, trace_id: Option, @@ -220,15 +219,13 @@ pub async fn one2m( query_args.ignore_take, ); - let inm_builder = match query_args.distinct { - Some(ref fs) => { - if req_inmem_distinct { - inm_builder.distinct(fs.clone()) - } else { - inm_builder - } - } - None => inm_builder, + let inm_builder = if req_inmem_distinct { + let inm_builder = inm_builder.distinct(query_args.distinct); + query_args.distinct = None; + + inm_builder + } else { + inm_builder }; Some(inm_builder.build()) diff --git a/query-engine/core/src/interpreter/query_interpreters/read.rs b/query-engine/core/src/interpreter/query_interpreters/read.rs index c653e11418ee..49cc9754c649 100644 --- a/query-engine/core/src/interpreter/query_interpreters/read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/read.rs @@ -85,7 +85,7 @@ fn read_one( /// -> Unstable cursors can't reliably be fetched by the underlying datasource, so we need to process part of it in-memory. fn read_many( tx: &mut dyn ConnectionLike, - query: ManyRecordsQuery, + mut query: ManyRecordsQuery, trace_id: Option, ) -> BoxFuture<'_, InterpretationResult> { let req_inmem_distinct = query.args.requires_inmemory_distinct(); @@ -98,15 +98,15 @@ fn read_many( query.args.ignore_take, ); - let inm_builder = match query.args.distinct { - Some(ref fs) => { - if req_inmem_distinct { - inm_builder.distinct(fs.clone()) - } else { - inm_builder - } - } - None => inm_builder, + dbg!(req_inmem_distinct); + let inm_builder = if req_inmem_distinct { + let inm_builder = inm_builder.distinct(query.args.distinct); + query.args.distinct = None; + dbg!(&query.args.distinct); + + inm_builder + } else { + inm_builder }; Some(inm_builder.build()) diff --git a/query-engine/query-structure/src/query_arguments.rs b/query-engine/query-structure/src/query_arguments.rs index 83b432ac91dd..70551d6c726f 100644 --- a/query-engine/query-structure/src/query_arguments.rs +++ b/query-engine/query-structure/src/query_arguments.rs @@ -72,17 +72,27 @@ impl QueryArguments { /// retrieved by the connector or if it requires the query engine to fetch a raw set /// of records and perform certain operations itself, in-memory. pub fn requires_inmemory_processing(&self) -> bool { - self.contains_unstable_cursor() || self.contains_null_cursor() + self.contains_unstable_cursor() || self.contains_null_cursor() || self.requires_inmemory_distinct() } pub fn requires_inmemory_distinct(&self) -> bool { + self.has_distinct() && (!self.has_distinct_capability() || self.has_orderby()) + } + + fn has_distinct_capability(&self) -> bool { + self.model() + .dm + .schema + .connector + .has_capability(ConnectorCapability::Distinct) + } + + fn has_distinct(&self) -> bool { self.distinct.is_some() - && !self - .model() - .dm - .schema - .connector - .has_capability(ConnectorCapability::Distinct) + } + + fn has_orderby(&self) -> bool { + !self.order_by.is_empty() } /// An unstable cursor is a cursor that is used in conjunction with an unstable (non-unique) combination of orderBys. From 2e7c863d9df42c3f0e264d9af5f0a4d224d2da27 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Wed, 29 Nov 2023 01:57:14 +0100 Subject: [PATCH 10/20] Clean up distinct builder --- .../query_interpreters/inmemory_record_processor.rs | 3 ++- .../src/interpreter/query_interpreters/nested_read.rs | 10 ++-------- .../core/src/interpreter/query_interpreters/read.rs | 6 +----- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs index b78e172a6c0e..8e3f9aae9066 100644 --- a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs +++ b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs @@ -229,8 +229,9 @@ impl InMemoryRecordProcessorBuilder { self } - pub fn distinct(mut self, distinct: Option) -> Self { + pub fn distinct(mut self, distinct: &mut Option) -> Self { self.args.distinct = distinct.clone(); + *distinct = None; self } diff --git a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs index 3a83595a7696..45c7b52a431d 100644 --- a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs @@ -18,10 +18,7 @@ pub(crate) async fn m2m( ); let inm_builder = if query.args.requires_inmemory_distinct() { - let inm_builder = inm_builder.distinct(query.args.distinct.clone()); - query.args.distinct = None; - - inm_builder + inm_builder.distinct(&mut query.args.distinct.clone()) } else { inm_builder }; @@ -220,10 +217,7 @@ pub async fn one2m( ); let inm_builder = if req_inmem_distinct { - let inm_builder = inm_builder.distinct(query_args.distinct); - query_args.distinct = None; - - inm_builder + inm_builder.distinct(&mut query_args.distinct) } else { inm_builder }; diff --git a/query-engine/core/src/interpreter/query_interpreters/read.rs b/query-engine/core/src/interpreter/query_interpreters/read.rs index 49cc9754c649..ac7525ffb5c0 100644 --- a/query-engine/core/src/interpreter/query_interpreters/read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/read.rs @@ -100,11 +100,7 @@ fn read_many( dbg!(req_inmem_distinct); let inm_builder = if req_inmem_distinct { - let inm_builder = inm_builder.distinct(query.args.distinct); - query.args.distinct = None; - dbg!(&query.args.distinct); - - inm_builder + inm_builder.distinct(&mut query.args.distinct) } else { inm_builder }; From 90b35f367d285ae41ba081f6b3f56ecea7afb193 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Thu, 30 Nov 2023 01:09:53 +0100 Subject: [PATCH 11/20] move apply_distinct back into apply --- .../query_interpreters/inmemory_record_processor.rs | 9 +++++++-- .../src/interpreter/query_interpreters/nested_read.rs | 6 ------ .../core/src/interpreter/query_interpreters/read.rs | 6 ------ 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs index 8e3f9aae9066..43e10abc66a5 100644 --- a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs +++ b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs @@ -50,7 +50,12 @@ impl InMemoryRecordProcessor { records }; - let records = self.apply_distinct(records); + let records = if self.requires_inmemory_distinct() { + self.apply_distinct(records) + } else { + records + }; + let mut records = self.apply_pagination(records); if self.needs_reversed_order() { @@ -79,7 +84,7 @@ impl InMemoryRecordProcessor { records.records.first().map(|x| x.parent_id.is_some()).unwrap_or(false) } - pub(crate) fn apply_distinct(&self, mut records: ManyRecords) -> ManyRecords { + fn apply_distinct(&self, mut records: ManyRecords) -> ManyRecords { let field_names = &records.field_names; let distinct_selection = if let Some(ref distinct) = self.distinct { diff --git a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs index 45c7b52a431d..752204d0ba93 100644 --- a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs @@ -292,12 +292,6 @@ pub async fn one2m( } let scalars = if let Some(processor) = processor { - let scalars = if req_inmem_distinct { - processor.apply_distinct(scalars) - } else { - scalars - }; - processor.apply(scalars) } else { scalars diff --git a/query-engine/core/src/interpreter/query_interpreters/read.rs b/query-engine/core/src/interpreter/query_interpreters/read.rs index ac7525ffb5c0..55d72c9f8f68 100644 --- a/query-engine/core/src/interpreter/query_interpreters/read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/read.rs @@ -122,12 +122,6 @@ fn read_many( .await?; let scalars = if let Some(p) = processor { - let scalars = if req_inmem_distinct { - p.apply_distinct(scalars) - } else { - scalars - }; - p.apply(scalars) } else { scalars From 8401c124edfd923a98a93d9395580d7bf78d0fef Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Thu, 30 Nov 2023 14:39:18 +0100 Subject: [PATCH 12/20] Fixed skip bug using builder --- .../tests/queries/distinct.rs | 11 +------ .../inmemory_record_processor.rs | 32 ++++++++++++------- .../query_interpreters/nested_read.rs | 29 +++++++++-------- .../interpreter/query_interpreters/read.rs | 17 +++++----- 4 files changed, 46 insertions(+), 43 deletions(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs index bc52fb7cfdf6..2fbb769521f5 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs @@ -35,7 +35,6 @@ mod distinct { { id } }") ), - // r#"{"data":{"findManyUser":[{"id":1},{"id":2}]}}"# @r###"{"data":{"findManyUser":[{"id":2},{"id":1}]}}"### ); @@ -102,9 +101,7 @@ mod distinct { { id, first_name, last_name } }") ), - // r#"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# - // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions - @r###"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"},{"id":3,"first_name":"Joe","last_name":"Doe"}]}}"### + @r###"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"### ); Ok(()) @@ -156,8 +153,6 @@ mod distinct { { id, first_name, last_name } }") ), - // r#"{"data":{"findManyUser":[{"id":3,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"# - // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions @r###"{"data":{"findManyUser":[{"id":3,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"### ); @@ -185,8 +180,6 @@ mod distinct { } }}") ), - // {"data":{"findManyUser":[{"id":1,"posts":[{"title":"3"},{"title":"1"},{"title":"2"}]},{"id":3,"posts":[]},{"id":4,"posts":[{"title":"1"}]},{"id":5,"posts":[{"title":"2"},{"title":"3"}]}]}} - // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions @r###"{"data":{"findManyUser":[{"id":1,"posts":[{"title":"3"},{"title":"1"},{"title":"2"}]},{"id":4,"posts":[{"title":"1"}]},{"id":3,"posts":[]},{"id":5,"posts":[{"title":"2"},{"title":"3"}]}]}}"### ); @@ -216,8 +209,6 @@ mod distinct { } }"} ), - // {"data":{"findManyUser":[{"id":5,"posts":[{"title":"2"},{"title":"3"}]},{"id":4,"posts":[{"title":"1"}]},{"id":3,"posts":[]},{"id":2,"posts":[{"title":"2"},{"title":"1"}]}]}} - // ! SELECT DISTINCT ON expressions must match initial ORDER BY expressions @r###"{"data":{"findManyUser":[{"id":5,"posts":[{"title":"2"},{"title":"3"}]},{"id":4,"posts":[{"title":"1"}]},{"id":3,"posts":[]},{"id":2,"posts":[{"title":"2"},{"title":"1"}]}]}}"### ); diff --git a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs index 43e10abc66a5..75b59b4f426c 100644 --- a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs +++ b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs @@ -199,7 +199,7 @@ pub struct InMemoryRecordProcessorBuilder { } impl InMemoryRecordProcessorBuilder { - pub fn new(model: Model, order_by: Vec, ignore_skip: bool, ignore_take: bool) -> Self { + pub fn new(model: Model) -> Self { Self { args: QueryArguments { model, @@ -207,30 +207,38 @@ impl InMemoryRecordProcessorBuilder { take: None, skip: None, filter: None, - order_by, + order_by: Default::default(), distinct: None, - ignore_skip, - ignore_take, + ignore_skip: false, + ignore_take: false, }, } } - pub fn _cursor(mut self, cursor: SelectionResult) -> Self { - self.args.cursor = Some(cursor); + + pub fn cursor(mut self, cursor: &mut Option) -> Self { + self.args.cursor = cursor.clone(); + self + } + + pub fn take(mut self, args: &mut QueryArguments) -> Self { + self.args.take = args.take; + args.ignore_take = true; self } - pub fn _take(mut self, take: i64) -> Self { - self.args.take = Some(take); + pub fn skip(mut self, args: &mut QueryArguments) -> Self { + self.args.skip = args.skip; + args.ignore_skip = true; self } - pub fn _skip(mut self, skip: i64) -> Self { - self.args.skip = Some(skip); + pub fn filter(mut self, filter: Option) -> Self { + self.args.filter = filter; self } - pub fn _filter(mut self, filter: Filter) -> Self { - self.args.filter = Some(filter); + pub fn order_by(mut self, order_by: Vec) -> Self { + self.args.order_by = order_by; self } diff --git a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs index 752204d0ba93..8dd11956c8ad 100644 --- a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs @@ -10,12 +10,7 @@ pub(crate) async fn m2m( parent_result: Option<&ManyRecords>, trace_id: Option, ) -> InterpretationResult<(ManyRecords, Option>)> { - let inm_builder = InMemoryRecordProcessorBuilder::new( - query.args.model.clone(), - query.args.order_by.clone(), - query.args.ignore_skip, - query.args.ignore_take, - ); + let inm_builder = InMemoryRecordProcessorBuilder::new(query.args.model.clone()); let inm_builder = if query.args.requires_inmemory_distinct() { inm_builder.distinct(&mut query.args.distinct.clone()) @@ -23,7 +18,13 @@ pub(crate) async fn m2m( inm_builder }; - let processor = inm_builder.build(); + let processor = inm_builder + .cursor(&mut query.args.cursor) + .take(&mut query.args) + .skip(&mut query.args) + .filter(query.args.filter.clone()) + .order_by(query.args.order_by.clone()) + .build(); let parent_field = &query.parent_field; let child_link_id = parent_field.related_field().linking_fields(); @@ -209,12 +210,7 @@ pub async fn one2m( let processor = if uniq_selections.len() == 1 && !query_args.requires_inmemory_processing() { None } else { - let inm_builder = InMemoryRecordProcessorBuilder::new( - query_args.model.clone(), - query_args.order_by.clone(), - query_args.ignore_skip, - query_args.ignore_take, - ); + let inm_builder = InMemoryRecordProcessorBuilder::new(query_args.model.clone()); let inm_builder = if req_inmem_distinct { inm_builder.distinct(&mut query_args.distinct) @@ -222,6 +218,13 @@ pub async fn one2m( inm_builder }; + let inm_builder = inm_builder + .cursor(&mut query_args.cursor) + .take(&mut query_args) + .skip(&mut query_args) + .filter(query_args.filter.clone()) + .order_by(query_args.order_by.clone()); + Some(inm_builder.build()) }; diff --git a/query-engine/core/src/interpreter/query_interpreters/read.rs b/query-engine/core/src/interpreter/query_interpreters/read.rs index 55d72c9f8f68..c30fab5df7c1 100644 --- a/query-engine/core/src/interpreter/query_interpreters/read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/read.rs @@ -91,20 +91,21 @@ fn read_many( let req_inmem_distinct = query.args.requires_inmemory_distinct(); let processor = if query.args.requires_inmemory_processing() { - let inm_builder = InMemoryRecordProcessorBuilder::new( - query.args.model.clone(), - query.args.order_by.clone(), - query.args.ignore_skip, - query.args.ignore_take, - ); - - dbg!(req_inmem_distinct); + let inm_builder = InMemoryRecordProcessorBuilder::new(query.args.model.clone()); + let inm_builder = if req_inmem_distinct { inm_builder.distinct(&mut query.args.distinct) } else { inm_builder }; + let inm_builder = inm_builder + .cursor(&mut query.args.cursor) + .take(&mut query.args) + .skip(&mut query.args) + .filter(query.args.filter.clone()) + .order_by(query.args.order_by.clone()); + Some(inm_builder.build()) } else { None From c836bcc9002ed588e5d1b684c875c392ce950346 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Thu, 30 Nov 2023 14:40:29 +0100 Subject: [PATCH 13/20] rename distinct capability distincton --- psl/builtin-connectors/src/postgres_datamodel_connector.rs | 2 +- psl/psl-core/src/datamodel_connector/capabilities.rs | 4 ++-- query-engine/query-structure/src/query_arguments.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/psl/builtin-connectors/src/postgres_datamodel_connector.rs b/psl/builtin-connectors/src/postgres_datamodel_connector.rs index 52568dd8fab3..a1bb73f8bd13 100644 --- a/psl/builtin-connectors/src/postgres_datamodel_connector.rs +++ b/psl/builtin-connectors/src/postgres_datamodel_connector.rs @@ -66,7 +66,7 @@ const CAPABILITIES: ConnectorCapabilities = enumflags2::make_bitflags!(Connector InsertReturning | UpdateReturning | RowIn | - Distinct + DistinctOn }); pub struct PostgresDatamodelConnector; diff --git a/psl/psl-core/src/datamodel_connector/capabilities.rs b/psl/psl-core/src/datamodel_connector/capabilities.rs index 5ae70e933574..3c0c1bbe47ed 100644 --- a/psl/psl-core/src/datamodel_connector/capabilities.rs +++ b/psl/psl-core/src/datamodel_connector/capabilities.rs @@ -103,8 +103,8 @@ capabilities!( NativeUpsert, InsertReturning, UpdateReturning, - RowIn, // Connector supports (a, b) IN (c, d) expression. - Distinct // Connector supports DB-level distinct (e.g. postgres) + RowIn, // Connector supports (a, b) IN (c, d) expression. + DistinctOn // Connector supports DB-level distinct (e.g. postgres) ); /// Contains all capabilities that the connector is able to serve. diff --git a/query-engine/query-structure/src/query_arguments.rs b/query-engine/query-structure/src/query_arguments.rs index 70551d6c726f..4e62c232f7b0 100644 --- a/query-engine/query-structure/src/query_arguments.rs +++ b/query-engine/query-structure/src/query_arguments.rs @@ -84,7 +84,7 @@ impl QueryArguments { .dm .schema .connector - .has_capability(ConnectorCapability::Distinct) + .has_capability(ConnectorCapability::DistinctOn) } fn has_distinct(&self) -> bool { From 0223800674cf2c532b1a073213d0a206379a330c Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Thu, 30 Nov 2023 17:13:22 +0100 Subject: [PATCH 14/20] revert back to using inm_processor directly --- .../inmemory_record_processor.rs | 68 ++----------------- .../query_interpreters/nested_read.rs | 37 +--------- .../interpreter/query_interpreters/read.rs | 21 +----- 3 files changed, 12 insertions(+), 114 deletions(-) diff --git a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs index 75b59b4f426c..110972e77f75 100644 --- a/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs +++ b/query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs @@ -20,9 +20,14 @@ impl InMemoryRecordProcessor { /// Creates a new processor from the given query args. /// The original args will be modified to prevent db level processing. pub(crate) fn new_from_query_args(args: &mut QueryArguments) -> Self { - let processor = Self { args: args.clone() }; + let mut processor = Self { args: args.clone() }; + + if !args.requires_inmemory_distinct() { + processor.args.distinct = None; + } else { + args.distinct = None; + } - args.distinct = None; args.ignore_take = true; args.ignore_skip = true; @@ -193,62 +198,3 @@ impl InMemoryRecordProcessor { self.take.or(self.skip).is_some() || self.cursor.is_some() } } - -pub struct InMemoryRecordProcessorBuilder { - args: QueryArguments, -} - -impl InMemoryRecordProcessorBuilder { - pub fn new(model: Model) -> Self { - Self { - args: QueryArguments { - model, - cursor: None, - take: None, - skip: None, - filter: None, - order_by: Default::default(), - distinct: None, - ignore_skip: false, - ignore_take: false, - }, - } - } - - pub fn cursor(mut self, cursor: &mut Option) -> Self { - self.args.cursor = cursor.clone(); - self - } - - pub fn take(mut self, args: &mut QueryArguments) -> Self { - self.args.take = args.take; - args.ignore_take = true; - self - } - - pub fn skip(mut self, args: &mut QueryArguments) -> Self { - self.args.skip = args.skip; - args.ignore_skip = true; - self - } - - pub fn filter(mut self, filter: Option) -> Self { - self.args.filter = filter; - self - } - - pub fn order_by(mut self, order_by: Vec) -> Self { - self.args.order_by = order_by; - self - } - - pub fn distinct(mut self, distinct: &mut Option) -> Self { - self.args.distinct = distinct.clone(); - *distinct = None; - self - } - - pub fn build(self) -> InMemoryRecordProcessor { - InMemoryRecordProcessor { args: self.args } - } -} diff --git a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs index 8dd11956c8ad..7e47b7f545a4 100644 --- a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs @@ -1,4 +1,4 @@ -use super::{inmemory_record_processor::InMemoryRecordProcessorBuilder, read}; +use super::{inmemory_record_processor::InMemoryRecordProcessor, read}; use crate::{interpreter::InterpretationResult, query_ast::*}; use connector::{self, ConnectionLike, RelAggregationRow, RelAggregationSelection}; use query_structure::*; @@ -10,21 +10,7 @@ pub(crate) async fn m2m( parent_result: Option<&ManyRecords>, trace_id: Option, ) -> InterpretationResult<(ManyRecords, Option>)> { - let inm_builder = InMemoryRecordProcessorBuilder::new(query.args.model.clone()); - - let inm_builder = if query.args.requires_inmemory_distinct() { - inm_builder.distinct(&mut query.args.distinct.clone()) - } else { - inm_builder - }; - - let processor = inm_builder - .cursor(&mut query.args.cursor) - .take(&mut query.args) - .skip(&mut query.args) - .filter(query.args.filter.clone()) - .order_by(query.args.order_by.clone()) - .build(); + let processor = InMemoryRecordProcessor::new_from_query_args(&mut query.args); let parent_field = &query.parent_field; let child_link_id = parent_field.related_field().linking_fields(); @@ -205,27 +191,10 @@ pub async fn one2m( // However, we can't just apply a LIMIT/OFFSET for multiple parents as we need N related records PER parent. // We could use ROW_NUMBER() but it requires further refactoring so we're still using in-memory processing for now. - let req_inmem_distinct = query_args.requires_inmemory_distinct(); - let processor = if uniq_selections.len() == 1 && !query_args.requires_inmemory_processing() { None } else { - let inm_builder = InMemoryRecordProcessorBuilder::new(query_args.model.clone()); - - let inm_builder = if req_inmem_distinct { - inm_builder.distinct(&mut query_args.distinct) - } else { - inm_builder - }; - - let inm_builder = inm_builder - .cursor(&mut query_args.cursor) - .take(&mut query_args) - .skip(&mut query_args) - .filter(query_args.filter.clone()) - .order_by(query_args.order_by.clone()); - - Some(inm_builder.build()) + Some(InMemoryRecordProcessor::new_from_query_args(&mut query_args)) }; let mut scalars = { diff --git a/query-engine/core/src/interpreter/query_interpreters/read.rs b/query-engine/core/src/interpreter/query_interpreters/read.rs index c30fab5df7c1..cac53ef5900a 100644 --- a/query-engine/core/src/interpreter/query_interpreters/read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/read.rs @@ -1,4 +1,4 @@ -use super::{inmemory_record_processor::InMemoryRecordProcessorBuilder, *}; +use super::{inmemory_record_processor::InMemoryRecordProcessor, *}; use crate::{interpreter::InterpretationResult, query_ast::*, result_ast::*}; use connector::{self, error::ConnectorError, ConnectionLike, RelAggregationRow, RelAggregationSelection}; use futures::future::{BoxFuture, FutureExt}; @@ -88,25 +88,8 @@ fn read_many( mut query: ManyRecordsQuery, trace_id: Option, ) -> BoxFuture<'_, InterpretationResult> { - let req_inmem_distinct = query.args.requires_inmemory_distinct(); - let processor = if query.args.requires_inmemory_processing() { - let inm_builder = InMemoryRecordProcessorBuilder::new(query.args.model.clone()); - - let inm_builder = if req_inmem_distinct { - inm_builder.distinct(&mut query.args.distinct) - } else { - inm_builder - }; - - let inm_builder = inm_builder - .cursor(&mut query.args.cursor) - .take(&mut query.args) - .skip(&mut query.args) - .filter(query.args.filter.clone()) - .order_by(query.args.order_by.clone()); - - Some(inm_builder.build()) + Some(InMemoryRecordProcessor::new_from_query_args(&mut query.args)) } else { None }; From 64b8841abf287b881223edf919354eae89338ce7 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Thu, 30 Nov 2023 17:47:39 +0100 Subject: [PATCH 15/20] Updated tests to: - track current in-mem snapshots - track in-db pg snapshots --- .../tests/queries/distinct.rs | 95 +++++++++++++++++-- 1 file changed, 88 insertions(+), 7 deletions(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs index 2fbb769521f5..fec9ac8ba8e3 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs @@ -1,5 +1,17 @@ use query_engine_tests::*; +/// `distinct on` queries involving `orderBy` +/// are currently forced through in-memory handling as otherwise, +/// they fail with the following error message. This does not affect _all_ +/// cases of `distinct on` in conjunction with `orderBy`. +/// +/// ```sql +/// SELECT DISTINCT ON expressions must match initial ORDER BY expressions +/// ``` +/// +/// `distinct on` queries _not_ involving `orderBy` return differently ordered +/// result sets, hence we need to duplicate certain tests to track snapshots +/// for both the in-db pg results, and the in-mem result sets. #[test_suite(schema(schemas::user_posts))] mod distinct { use indoc::indoc; @@ -22,8 +34,8 @@ mod distinct { } /// Regression test for not selecting the fields the distinct is performed on: https://github.com/prisma/prisma/issues/5969 - #[connector_test] - async fn no_panic(runner: Runner) -> TestResult<()> { + #[connector_test(exclude(CockroachDb, MongoDb, SqlServer, MySQL, Sqlite))] + async fn no_panic_pg(runner: Runner) -> TestResult<()> { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user(&runner, r#"{ id: 2, first_name: "Doe", last_name: "Joe", email: "2" }"#).await?; @@ -41,6 +53,26 @@ mod distinct { Ok(()) } + /// Regression test for not selecting the fields the distinct is performed on: https://github.com/prisma/prisma/issues/5969 + #[connector_test(exclude(Postgres))] + async fn no_panic_mem(runner: Runner) -> TestResult<()> { + test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; + test_user(&runner, r#"{ id: 2, first_name: "Doe", last_name: "Joe", email: "2" }"#).await?; + + insta::assert_snapshot!( + run_query!( + &runner, + indoc!("{ + findManyUser(distinct: [first_name, last_name]) + { id } + }") + ), + @r###"{"data":{"findManyUser":[{"id":1},{"id":2}]}}"### + ); + + Ok(()) + } + #[connector_test] async fn shorthand_works(runner: Runner) -> TestResult<()> { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; @@ -60,8 +92,31 @@ mod distinct { Ok(()) } - #[connector_test] - async fn with_duplicates(runner: Runner) -> TestResult<()> { + #[connector_test(exclude(Postgres))] + async fn with_duplicates_pg(runner: Runner) -> TestResult<()> { + test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; + test_user( + &runner, + r#"{ id: 2, first_name: "Hans", last_name: "Wurst", email: "2" }"#, + ) + .await?; + test_user(&runner, r#"{ id: 3, first_name: "Joe", last_name: "Doe", email: "3" }"#).await?; + + insta::assert_snapshot!(run_query!( + &runner, + indoc!("{ + findManyUser(distinct: [first_name, last_name]) + { id, first_name, last_name } + }") + ), + @r###"{"data":{"findManyUser":[{"id":1,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"### + ); + + Ok(()) + } + + #[connector_test(exclude(CockroachDb, MongoDb, SqlServer, MySQL, Sqlite))] + async fn with_duplicates_mem(runner: Runner) -> TestResult<()> { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user( &runner, @@ -160,14 +215,14 @@ mod distinct { } /// Mut return only distinct records for top record, and only for those the distinct relation records. - #[connector_test] - async fn nested_distinct(runner: Runner) -> TestResult<()> { + #[connector_test(exclude(CockroachDb, MongoDb, SqlServer, MySQL, Sqlite))] + async fn nested_distinct_pg(runner: Runner) -> TestResult<()> { nested_dataset(&runner).await?; // Returns Users 1, 3, 4, 5 top // 1 => ["3", "1", "2"] - // 3 => [] // 4 => ["1"] + // 3 => [] // 5 => ["2", "3"] insta::assert_snapshot!(run_query!( &runner, @@ -186,6 +241,32 @@ mod distinct { Ok(()) } + #[connector_test(exclude(Postgres))] + async fn nested_distinct_mem(runner: Runner) -> TestResult<()> { + nested_dataset(&runner).await?; + + // Returns Users 1, 3, 4, 5 top + // 1 => ["3", "1", "2"] + // 3 => [] + // 4 => ["1"] + // 5 => ["2", "3"] + insta::assert_snapshot!(run_query!( + &runner, + indoc!("{ + findManyUser(distinct: [first_name, last_name]) + { + id + posts(distinct: [title], orderBy: { id: asc }) { + title + } + }}") + ), + @r###"{"data":{"findManyUser":[{"id":1,"posts":[{"title":"3"},{"title":"1"},{"title":"2"}]},{"id":3,"posts":[]},{"id":4,"posts":[{"title":"1"}]},{"id":5,"posts":[{"title":"2"},{"title":"3"}]}]}}"### + ); + + Ok(()) + } + /// Mut return only distinct records for top record, and only for those the distinct relation records. Both orderings reversed. #[connector_test] async fn nested_distinct_reversed(runner: Runner) -> TestResult<()> { From 66f919ad4e63666a7161dc3cdcc0115ea164aebb Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Thu, 30 Nov 2023 17:55:03 +0100 Subject: [PATCH 16/20] remove nl --- .../core/src/interpreter/query_interpreters/nested_read.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs index 7e47b7f545a4..8573e2853c45 100644 --- a/query-engine/core/src/interpreter/query_interpreters/nested_read.rs +++ b/query-engine/core/src/interpreter/query_interpreters/nested_read.rs @@ -190,7 +190,6 @@ pub async fn one2m( // If we're fetching related records from a single parent, then we can apply normal pagination instead of in-memory processing. // However, we can't just apply a LIMIT/OFFSET for multiple parents as we need N related records PER parent. // We could use ROW_NUMBER() but it requires further refactoring so we're still using in-memory processing for now. - let processor = if uniq_selections.len() == 1 && !query_args.requires_inmemory_processing() { None } else { From 439eeb0fe4905b46897932febb939463e9fab761 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Fri, 1 Dec 2023 15:34:08 +0100 Subject: [PATCH 17/20] Add `DistinctOn` preview feature --- psl/psl-core/src/common/preview_features.rs | 2 ++ .../tests/queries/distinct.rs | 18 +++++++++--------- .../query-structure/src/query_arguments.rs | 14 ++++++++++++-- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/psl/psl-core/src/common/preview_features.rs b/psl/psl-core/src/common/preview_features.rs index 544bf5b99164..ca9da322933d 100644 --- a/psl/psl-core/src/common/preview_features.rs +++ b/psl/psl-core/src/common/preview_features.rs @@ -43,6 +43,7 @@ features!( ConnectOrCreate, CreateMany, DataProxy, + DistinctOn, Deno, Distinct, DriverAdapters, @@ -82,6 +83,7 @@ features!( pub const ALL_PREVIEW_FEATURES: FeatureMap = FeatureMap { active: enumflags2::make_bitflags!(PreviewFeature::{ Deno + | DistinctOn | DriverAdapters | FullTextIndex | FullTextSearch diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs index fec9ac8ba8e3..d1002323f0e3 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs @@ -34,7 +34,7 @@ mod distinct { } /// Regression test for not selecting the fields the distinct is performed on: https://github.com/prisma/prisma/issues/5969 - #[connector_test(exclude(CockroachDb, MongoDb, SqlServer, MySQL, Sqlite))] + #[connector_test(only(Postgres))] async fn no_panic_pg(runner: Runner) -> TestResult<()> { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user(&runner, r#"{ id: 2, first_name: "Doe", last_name: "Joe", email: "2" }"#).await?; @@ -55,7 +55,7 @@ mod distinct { /// Regression test for not selecting the fields the distinct is performed on: https://github.com/prisma/prisma/issues/5969 #[connector_test(exclude(Postgres))] - async fn no_panic_mem(runner: Runner) -> TestResult<()> { + async fn no_panic_other(runner: Runner) -> TestResult<()> { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user(&runner, r#"{ id: 2, first_name: "Doe", last_name: "Joe", email: "2" }"#).await?; @@ -92,7 +92,7 @@ mod distinct { Ok(()) } - #[connector_test(exclude(Postgres))] + #[connector_test(only(Postgres))] async fn with_duplicates_pg(runner: Runner) -> TestResult<()> { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user( @@ -109,14 +109,14 @@ mod distinct { { id, first_name, last_name } }") ), - @r###"{"data":{"findManyUser":[{"id":1,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"### + @r###"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"},{"id":1,"first_name":"Joe","last_name":"Doe"}]}}"### ); Ok(()) } - #[connector_test(exclude(CockroachDb, MongoDb, SqlServer, MySQL, Sqlite))] - async fn with_duplicates_mem(runner: Runner) -> TestResult<()> { + #[connector_test(exclude(Postgres))] + async fn with_duplicates_other(runner: Runner) -> TestResult<()> { test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?; test_user( &runner, @@ -132,7 +132,7 @@ mod distinct { { id, first_name, last_name } }") ), - @r###"{"data":{"findManyUser":[{"id":2,"first_name":"Hans","last_name":"Wurst"},{"id":1,"first_name":"Joe","last_name":"Doe"}]}}"### + @r###"{"data":{"findManyUser":[{"id":1,"first_name":"Joe","last_name":"Doe"},{"id":2,"first_name":"Hans","last_name":"Wurst"}]}}"### ); Ok(()) @@ -215,7 +215,7 @@ mod distinct { } /// Mut return only distinct records for top record, and only for those the distinct relation records. - #[connector_test(exclude(CockroachDb, MongoDb, SqlServer, MySQL, Sqlite))] + #[connector_test(only(Postgres))] async fn nested_distinct_pg(runner: Runner) -> TestResult<()> { nested_dataset(&runner).await?; @@ -242,7 +242,7 @@ mod distinct { } #[connector_test(exclude(Postgres))] - async fn nested_distinct_mem(runner: Runner) -> TestResult<()> { + async fn nested_distinct_other(runner: Runner) -> TestResult<()> { nested_dataset(&runner).await?; // Returns Users 1, 3, 4, 5 top diff --git a/query-engine/query-structure/src/query_arguments.rs b/query-engine/query-structure/src/query_arguments.rs index 4e62c232f7b0..9553cc089e83 100644 --- a/query-engine/query-structure/src/query_arguments.rs +++ b/query-engine/query-structure/src/query_arguments.rs @@ -1,4 +1,4 @@ -use psl::datamodel_connector::ConnectorCapability; +use psl::{datamodel_connector::ConnectorCapability, PreviewFeature}; use crate::*; @@ -76,7 +76,17 @@ impl QueryArguments { } pub fn requires_inmemory_distinct(&self) -> bool { - self.has_distinct() && (!self.has_distinct_capability() || self.has_orderby()) + self.has_distinct() + && ((!self.has_distinct_capability() && !self.has_distincton_preview()) || self.has_orderby()) + } + + fn has_distincton_preview(&self) -> bool { + self.model() + .dm + .schema + .configuration + .preview_features() + .contains(PreviewFeature::DistinctOn) } fn has_distinct_capability(&self) -> bool { From 14c206b1a3d6f3ae647886cbd778cc6a34cfc412 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Fri, 1 Dec 2023 18:54:01 +0100 Subject: [PATCH 18/20] fixed req-in-mem-distinct logic --- query-engine/query-structure/src/query_arguments.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/query-engine/query-structure/src/query_arguments.rs b/query-engine/query-structure/src/query_arguments.rs index 9553cc089e83..699f3c2879d7 100644 --- a/query-engine/query-structure/src/query_arguments.rs +++ b/query-engine/query-structure/src/query_arguments.rs @@ -76,8 +76,7 @@ impl QueryArguments { } pub fn requires_inmemory_distinct(&self) -> bool { - self.has_distinct() - && ((!self.has_distinct_capability() && !self.has_distincton_preview()) || self.has_orderby()) + !self.has_distincton_preview() || !self.has_distinct_capability() || self.has_orderby() } fn has_distincton_preview(&self) -> bool { @@ -97,10 +96,6 @@ impl QueryArguments { .has_capability(ConnectorCapability::DistinctOn) } - fn has_distinct(&self) -> bool { - self.distinct.is_some() - } - fn has_orderby(&self) -> bool { !self.order_by.is_empty() } From 20a8420c8f21a95addb1f14ad2b391e60e505593 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Mon, 4 Dec 2023 12:00:54 +0100 Subject: [PATCH 19/20] Fix aggregation tests --- .../query-structure/src/query_arguments.rs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/query-engine/query-structure/src/query_arguments.rs b/query-engine/query-structure/src/query_arguments.rs index 699f3c2879d7..16eaa189dd88 100644 --- a/query-engine/query-structure/src/query_arguments.rs +++ b/query-engine/query-structure/src/query_arguments.rs @@ -76,28 +76,26 @@ impl QueryArguments { } pub fn requires_inmemory_distinct(&self) -> bool { - !self.has_distincton_preview() || !self.has_distinct_capability() || self.has_orderby() + self.distinct.is_some() && !self.can_distinct_in_db() } - fn has_distincton_preview(&self) -> bool { - self.model() + fn can_distinct_in_db(&self) -> bool { + let has_distinct_feature = self + .model() .dm .schema .configuration .preview_features() - .contains(PreviewFeature::DistinctOn) - } + .contains(PreviewFeature::DistinctOn); - fn has_distinct_capability(&self) -> bool { - self.model() + let connector_can_distinct_in_db = self + .model() .dm .schema .connector - .has_capability(ConnectorCapability::DistinctOn) - } + .has_capability(ConnectorCapability::DistinctOn); - fn has_orderby(&self) -> bool { - !self.order_by.is_empty() + has_distinct_feature && connector_can_distinct_in_db && self.order_by.is_empty() } /// An unstable cursor is a cursor that is used in conjunction with an unstable (non-unique) combination of orderBys. From a9481128cbf7689b5d2f5f50296c321afae20056 Mon Sep 17 00:00:00 2001 From: Sophie Atkins Date: Mon, 4 Dec 2023 12:10:33 +0100 Subject: [PATCH 20/20] Update preview feature test --- psl/psl/tests/config/generators.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/psl/psl/tests/config/generators.rs b/psl/psl/tests/config/generators.rs index f10a9bda3eae..2f0708b13dc9 100644 --- a/psl/psl/tests/config/generators.rs +++ b/psl/psl/tests/config/generators.rs @@ -258,7 +258,7 @@ fn nice_error_for_unknown_generator_preview_feature() { .unwrap_err(); let expectation = expect![[r#" - error: The preview feature "foo" is not known. Expected one of: deno, driverAdapters, fullTextIndex, fullTextSearch, metrics, multiSchema, postgresqlExtensions, tracing, views + error: The preview feature "foo" is not known. Expected one of: distinctOn, deno, driverAdapters, fullTextIndex, fullTextSearch, metrics, multiSchema, postgresqlExtensions, tracing, views --> schema.prisma:3  |   2 |  provider = "prisma-client-js"