From 76a412ac2a1d6138b07cce86cebe5b5e108c17e8 Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Sun, 8 Sep 2024 19:36:49 +0200 Subject: [PATCH 01/11] Implement Offset and Limit Dsl on CombinationClause --- .../src/query_builder/combination_clause.rs | 169 ++++++++++++++++-- diesel_tests/tests/combination.rs | 58 ++++++ 2 files changed, 212 insertions(+), 15 deletions(-) diff --git a/diesel/src/query_builder/combination_clause.rs b/diesel/src/query_builder/combination_clause.rs index ba7908e60df2..7cbfd65ec4e2 100644 --- a/diesel/src/query_builder/combination_clause.rs +++ b/diesel/src/query_builder/combination_clause.rs @@ -1,22 +1,46 @@ //! Combine queries using a combinator like `UNION`, `INTERSECT` or `EXPECT` //! with or without `ALL` rule for duplicates +//! +//! Within this module, types commonly use the following abbreviations: +//! +//! L: Limit Clause +//! Of: Offset Clause +//! LOf: Limit Offset Clause use crate::backend::{Backend, DieselReserveSpecialization}; +use crate::dsl::AsExprOf; use crate::expression::subselect::ValidSubselect; +use crate::expression::IntoSql; use crate::expression::NonAggregate; +use crate::expression::*; +use crate::query_builder::from_clause::AsQuerySource; use crate::query_builder::insert_statement::InsertFromSelect; +use crate::query_builder::limit_clause::{LimitClause, NoLimitClause}; +use crate::query_builder::limit_offset_clause::LimitOffsetClause; +use crate::query_builder::offset_clause::{NoOffsetClause, OffsetClause}; use crate::query_builder::{AsQuery, AstPass, Query, QueryFragment, QueryId, SelectQuery}; -use crate::{CombineDsl, Insertable, QueryResult, RunQueryDsl, Table}; +use crate::query_dsl::methods::*; +use crate::query_source::*; +use crate::sql_types::BigInt; +use crate::{CombineDsl, Insertable, QueryResult, QueryDsl, RunQueryDsl, Table}; #[derive(Debug, Copy, Clone, QueryId)] #[must_use = "Queries are only executed when calling `load`, `get_result` or similar."] /// Combine queries using a combinator like `UNION`, `INTERSECT` or `EXPECT` /// with or without `ALL` rule for duplicates -pub struct CombinationClause { +pub struct CombinationClause< + Combinator, + Rule, + Source, + Rhs, + LimitOffset = LimitOffsetClause, +> { combinator: Combinator, duplicate_rule: Rule, source: ParenthesisWrapper, rhs: ParenthesisWrapper, + /// The combined limit/offset clause of the query + limit_offset: LimitOffset, } impl CombinationClause { @@ -32,11 +56,57 @@ impl CombinationClause Query for CombinationClause +impl + CombinationClause +{ + pub(crate) fn new_full( + combinator: Combinator, + duplicate_rule: Rule, + source: Source, + rhs: Rhs, + limit_offset: LOf, + ) -> Self { + CombinationClause { + combinator, + duplicate_rule, + source: ParenthesisWrapper(source), + rhs: ParenthesisWrapper(rhs), + limit_offset, + } + } +} + +impl QueryDsl + for CombinationClause {} + +impl QuerySource + for CombinationClause +where + Combinator: AsQuerySource, + ::DefaultSelection: SelectableExpression, +{ + type FromClause = ::FromClause; + type DefaultSelection = ::DefaultSelection; + + fn from_clause(&self) -> ::FromClause { + self.combinator.as_query_source().from_clause() + } + + fn default_selection(&self) -> Self::DefaultSelection { + self.combinator.as_query_source().default_selection() + } +} + +impl Query + for CombinationClause where Source: Query, Rhs: Query, @@ -44,7 +114,8 @@ where type SqlType = Source::SqlType; } -impl SelectQuery for CombinationClause +impl SelectQuery + for CombinationClause where Source: SelectQuery, Rhs: SelectQuery, @@ -52,21 +123,21 @@ where type SqlType = Source::SqlType; } -impl ValidSubselect - for CombinationClause +impl ValidSubselect + for CombinationClause where Source: ValidSubselect, Rhs: ValidSubselect, { } -impl RunQueryDsl - for CombinationClause +impl RunQueryDsl + for CombinationClause { } -impl Insertable - for CombinationClause +impl Insertable + for CombinationClause where T: Table, T::AllColumns: NonAggregate, @@ -79,8 +150,8 @@ where } } -impl CombineDsl - for CombinationClause +impl CombineDsl + for CombinationClause where Self: Query, { @@ -129,20 +200,88 @@ where } } -impl QueryFragment - for CombinationClause +impl QueryFragment + for CombinationClause where Combinator: QueryFragment, Rule: QueryFragment, ParenthesisWrapper: QueryFragment, ParenthesisWrapper: QueryFragment, + LOf: QueryFragment, DB: Backend + SupportsCombinationClause + DieselReserveSpecialization, { fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> { self.source.walk_ast(out.reborrow())?; self.combinator.walk_ast(out.reborrow())?; self.duplicate_rule.walk_ast(out.reborrow())?; - self.rhs.walk_ast(out) + self.rhs.walk_ast(out.reborrow())?; + self.limit_offset.walk_ast(out) + } +} + +#[doc(hidden)] +type Limit = AsExprOf; + +impl LimitDsl + for CombinationClause> +where + Self: SelectQuery, + CombinationClause, Of>>: + SelectQuery, +{ + type Output = CombinationClause< + Combinator, + Rule, + Source, + Rhs, + LimitOffsetClause, Of>, + >; + + fn limit(self, limit: i64) -> Self::Output { + let limit_clause = LimitClause(limit.into_sql::()); + CombinationClause::new_full( + self.combinator, + self.duplicate_rule, + self.source.0, + self.rhs.0, + LimitOffsetClause { + limit_clause, + offset_clause: self.limit_offset.offset_clause, + }, + ) + } +} + +#[doc(hidden)] +type Offset = Limit; + +impl OffsetDsl + for CombinationClause> +where + Self: SelectQuery, + CombinationClause>>: + SelectQuery, +{ + type Output = CombinationClause< + Combinator, + Rule, + Source, + Rhs, + LimitOffsetClause>, + >; + + fn offset(self, offset: i64) -> Self::Output { + let offset_clause = OffsetClause(offset.into_sql::()); + CombinationClause::new_full( + self.combinator, + self.duplicate_rule, + self.source.0, + self.rhs.0, + LimitOffsetClause { + limit_clause: self.limit_offset.limit_clause, + offset_clause, + }, + ) } } diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs index 368d9810403d..1e677bc8ce9e 100644 --- a/diesel_tests/tests/combination.rs +++ b/diesel_tests/tests/combination.rs @@ -117,6 +117,64 @@ fn except() { assert_eq!(expected_data, data); } +#[test] +fn union_with_limit() { + use crate::schema::users::dsl::*; + + let conn = &mut connection(); + let data = vec![ + NewUser::new("Sean", None), + NewUser::new("Tess", None), + NewUser::new("Jim", None), + ]; + insert_into(users).values(&data).execute(conn).unwrap(); + let data = users.order(id).load::(conn).unwrap(); + let sean = &data[0]; + let tess = &data[1]; + let _jim = &data[2]; + + let expected_data = vec![ + User::new(sean.id, "Sean"), + User::new(tess.id, "Tess"), + ]; + let data: Vec<_> = users + .filter(id.le(tess.id)) + .union(users.filter(id.ge(tess.id))) + .limit(2) + .load(conn) + .unwrap(); + assert_eq!(expected_data, data); +} + +#[test] +fn union_with_offset() { + use crate::schema::users::dsl::*; + + let conn = &mut connection(); + let data = vec![ + NewUser::new("Sean", None), + NewUser::new("Tess", None), + NewUser::new("Jim", None), + ]; + insert_into(users).values(&data).execute(conn).unwrap(); + let data = users.order(id).load::(conn).unwrap(); + let _sean = &data[0]; + let tess = &data[1]; + let jim = &data[2]; + + let expected_data = vec![ + User::new(tess.id, "Tess"), + User::new(jim.id, "Jim"), + ]; + let data: Vec<_> = users + .filter(id.le(tess.id)) + .union(users.filter(id.ge(tess.id))) + .offset(1) + .load(conn) + .unwrap(); + assert_eq!(expected_data, data); +} + #[test] fn union_with_order() { let conn = &mut connection(); From 9503575513492f8b70efc4969f38c93d19100257 Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Sun, 8 Sep 2024 19:48:44 +0200 Subject: [PATCH 02/11] Fix PositionalOrderDsl for CombinationClause --- diesel/src/query_dsl/positional_order_dsl.rs | 4 ++-- diesel_tests/tests/combination.rs | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/diesel/src/query_dsl/positional_order_dsl.rs b/diesel/src/query_dsl/positional_order_dsl.rs index 0b20fcab5bb6..a748ecf71c4a 100644 --- a/diesel/src/query_dsl/positional_order_dsl.rs +++ b/diesel/src/query_dsl/positional_order_dsl.rs @@ -26,8 +26,8 @@ pub struct PositionalOrderClause { expr: Expr, } -impl PositionalOrderDsl - for CombinationClause +impl PositionalOrderDsl + for CombinationClause { } diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs index 1e677bc8ce9e..a89223182def 100644 --- a/diesel_tests/tests/combination.rs +++ b/diesel_tests/tests/combination.rs @@ -131,16 +131,17 @@ fn union_with_limit() { let data = users.order(id).load::(conn).unwrap(); let sean = &data[0]; let tess = &data[1]; - let _jim = &data[2]; + let jim = &data[2]; let expected_data = vec![ + User::new(jim.id, "Jim"), User::new(sean.id, "Sean"), - User::new(tess.id, "Tess"), ]; let data: Vec<_> = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) .limit(2) + .positional_order_by(2) // name is the second column .load(conn) .unwrap(); assert_eq!(expected_data, data); From eeaf8c1253bef23a72163e41920d55c8711cf08f Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Sun, 8 Sep 2024 20:04:17 +0200 Subject: [PATCH 03/11] Remove left-over from attempting to implement order --- .../src/query_builder/combination_clause.rs | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/diesel/src/query_builder/combination_clause.rs b/diesel/src/query_builder/combination_clause.rs index 7cbfd65ec4e2..914ee11b05d3 100644 --- a/diesel/src/query_builder/combination_clause.rs +++ b/diesel/src/query_builder/combination_clause.rs @@ -12,15 +12,12 @@ use crate::dsl::AsExprOf; use crate::expression::subselect::ValidSubselect; use crate::expression::IntoSql; use crate::expression::NonAggregate; -use crate::expression::*; -use crate::query_builder::from_clause::AsQuerySource; use crate::query_builder::insert_statement::InsertFromSelect; use crate::query_builder::limit_clause::{LimitClause, NoLimitClause}; use crate::query_builder::limit_offset_clause::LimitOffsetClause; use crate::query_builder::offset_clause::{NoOffsetClause, OffsetClause}; use crate::query_builder::{AsQuery, AstPass, Query, QueryFragment, QueryId, SelectQuery}; use crate::query_dsl::methods::*; -use crate::query_source::*; use crate::sql_types::BigInt; use crate::{CombineDsl, Insertable, QueryResult, QueryDsl, RunQueryDsl, Table}; @@ -87,24 +84,6 @@ impl impl QueryDsl for CombinationClause {} -impl QuerySource - for CombinationClause -where - Combinator: AsQuerySource, - ::DefaultSelection: SelectableExpression, -{ - type FromClause = ::FromClause; - type DefaultSelection = ::DefaultSelection; - - fn from_clause(&self) -> ::FromClause { - self.combinator.as_query_source().from_clause() - } - - fn default_selection(&self) -> Self::DefaultSelection { - self.combinator.as_query_source().default_selection() - } -} - impl Query for CombinationClause where From cfc14c127948804675ec1ec74b6cc6c66b2fddd6 Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Sun, 8 Sep 2024 20:04:48 +0200 Subject: [PATCH 04/11] Revert implementing position order for limited/offsetted combination clause --- diesel/src/query_dsl/positional_order_dsl.rs | 4 ++-- diesel_tests/tests/combination.rs | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/diesel/src/query_dsl/positional_order_dsl.rs b/diesel/src/query_dsl/positional_order_dsl.rs index a748ecf71c4a..0b20fcab5bb6 100644 --- a/diesel/src/query_dsl/positional_order_dsl.rs +++ b/diesel/src/query_dsl/positional_order_dsl.rs @@ -26,8 +26,8 @@ pub struct PositionalOrderClause { expr: Expr, } -impl PositionalOrderDsl - for CombinationClause +impl PositionalOrderDsl + for CombinationClause { } diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs index a89223182def..05fcc75c42a4 100644 --- a/diesel_tests/tests/combination.rs +++ b/diesel_tests/tests/combination.rs @@ -134,14 +134,13 @@ fn union_with_limit() { let jim = &data[2]; let expected_data = vec![ - User::new(jim.id, "Jim"), User::new(sean.id, "Sean"), + User::new(tess.id, "Tess"), ]; let data: Vec<_> = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) .limit(2) - .positional_order_by(2) // name is the second column .load(conn) .unwrap(); assert_eq!(expected_data, data); From 29635c8564fbbbdc6c8f79a4e6e2a1c7d20c1f7b Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Sun, 8 Sep 2024 20:20:14 +0200 Subject: [PATCH 05/11] Tidy --- .../src/query_builder/combination_clause.rs | 19 +++++++------------ diesel_tests/tests/combination.rs | 12 +++--------- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/diesel/src/query_builder/combination_clause.rs b/diesel/src/query_builder/combination_clause.rs index 914ee11b05d3..62a58966d7e6 100644 --- a/diesel/src/query_builder/combination_clause.rs +++ b/diesel/src/query_builder/combination_clause.rs @@ -19,7 +19,7 @@ use crate::query_builder::offset_clause::{NoOffsetClause, OffsetClause}; use crate::query_builder::{AsQuery, AstPass, Query, QueryFragment, QueryId, SelectQuery}; use crate::query_dsl::methods::*; use crate::sql_types::BigInt; -use crate::{CombineDsl, Insertable, QueryResult, QueryDsl, RunQueryDsl, Table}; +use crate::{CombineDsl, Insertable, QueryDsl, QueryResult, RunQueryDsl, Table}; #[derive(Debug, Copy, Clone, QueryId)] #[must_use = "Queries are only executed when calling `load`, `get_result` or similar."] @@ -61,9 +61,7 @@ impl CombinationClause - CombinationClause -{ +impl CombinationClause { pub(crate) fn new_full( combinator: Combinator, duplicate_rule: Rule, @@ -82,7 +80,9 @@ impl } impl QueryDsl - for CombinationClause {} + for CombinationClause +{ +} impl Query for CombinationClause @@ -208,13 +208,8 @@ where CombinationClause, Of>>: SelectQuery, { - type Output = CombinationClause< - Combinator, - Rule, - Source, - Rhs, - LimitOffsetClause, Of>, - >; + type Output = + CombinationClause, Of>>; fn limit(self, limit: i64) -> Self::Output { let limit_clause = LimitClause(limit.into_sql::()); diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs index 05fcc75c42a4..eb27e30c69d5 100644 --- a/diesel_tests/tests/combination.rs +++ b/diesel_tests/tests/combination.rs @@ -131,12 +131,9 @@ fn union_with_limit() { let data = users.order(id).load::(conn).unwrap(); let sean = &data[0]; let tess = &data[1]; - let jim = &data[2]; + let _jim = &data[2]; - let expected_data = vec![ - User::new(sean.id, "Sean"), - User::new(tess.id, "Tess"), - ]; + let expected_data = vec![User::new(sean.id, "Sean"), User::new(tess.id, "Tess")]; let data: Vec<_> = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) @@ -162,10 +159,7 @@ fn union_with_offset() { let tess = &data[1]; let jim = &data[2]; - let expected_data = vec![ - User::new(tess.id, "Tess"), - User::new(jim.id, "Jim"), - ]; + let expected_data = vec![User::new(tess.id, "Tess"), User::new(jim.id, "Jim")]; let data: Vec<_> = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) From 35e86279573ad67b9be333ffce046afca086676e Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Sun, 8 Sep 2024 20:32:29 +0200 Subject: [PATCH 06/11] Fix union_with_offset test for mysql requiring limit --- diesel_tests/tests/combination.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs index eb27e30c69d5..55bcd17d06c9 100644 --- a/diesel_tests/tests/combination.rs +++ b/diesel_tests/tests/combination.rs @@ -163,6 +163,7 @@ fn union_with_offset() { let data: Vec<_> = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) + .limit(3) .offset(1) .load(conn) .unwrap(); From c3ce0cccea3dfe5a8249c446b6fe771f2d7a5aec Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Mon, 9 Sep 2024 08:36:57 +0200 Subject: [PATCH 07/11] Fix compile test stderr output --- .../tests/fail/exists_can_only_take_subselects.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/diesel_compile_tests/tests/fail/exists_can_only_take_subselects.stderr b/diesel_compile_tests/tests/fail/exists_can_only_take_subselects.stderr index ad7ed57b409a..b559972f8f4e 100644 --- a/diesel_compile_tests/tests/fail/exists_can_only_take_subselects.stderr +++ b/diesel_compile_tests/tests/fail/exists_can_only_take_subselects.stderr @@ -7,7 +7,7 @@ error[E0277]: the trait bound `bool: SelectQuery` is not satisfied = help: the following other types implement trait `SelectQuery`: BoxedSelectStatement<'a, ST, QS, DB, GB> SelectStatement - diesel::query_builder::combination_clause::CombinationClause + diesel::query_builder::combination_clause::CombinationClause = note: required for `diesel::expression::subselect::Subselect` to implement `diesel::Expression` = note: 1 redundant requirement hidden = note: required for `Exists` to implement `diesel::Expression` @@ -22,7 +22,7 @@ error[E0277]: the trait bound `users::columns::id: SelectQuery` is not satisfied = help: the following other types implement trait `SelectQuery`: BoxedSelectStatement<'a, ST, QS, DB, GB> SelectStatement - diesel::query_builder::combination_clause::CombinationClause + diesel::query_builder::combination_clause::CombinationClause = note: required for `diesel::expression::subselect::Subselect` to implement `diesel::Expression` = note: 1 redundant requirement hidden = note: required for `Exists` to implement `diesel::Expression` From 81f0e98f7279e060239330f71437432778043f40 Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Mon, 9 Sep 2024 09:15:06 +0200 Subject: [PATCH 08/11] Manually sort combination result data for comparison --- diesel_tests/tests/combination.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs index 55bcd17d06c9..6e693d302eca 100644 --- a/diesel_tests/tests/combination.rs +++ b/diesel_tests/tests/combination.rs @@ -134,12 +134,13 @@ fn union_with_limit() { let _jim = &data[2]; let expected_data = vec![User::new(sean.id, "Sean"), User::new(tess.id, "Tess")]; - let data: Vec<_> = users + let mut data: Vec<_> = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) .limit(2) .load(conn) .unwrap(); + data.sort_by_key(|u: &User| u.id); assert_eq!(expected_data, data); } @@ -160,13 +161,14 @@ fn union_with_offset() { let jim = &data[2]; let expected_data = vec![User::new(tess.id, "Tess"), User::new(jim.id, "Jim")]; - let data: Vec<_> = users + let mut data: Vec<_> = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) .limit(3) .offset(1) .load(conn) .unwrap(); + data.sort_by_key(|u: &User| u.id); assert_eq!(expected_data, data); } From f96d29d4bf68fbc4916f7281bd2a09ee65e6ca7f Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Mon, 9 Sep 2024 09:26:00 +0200 Subject: [PATCH 09/11] Stop expecting data from unordered combination clause --- diesel_tests/tests/combination.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs index 6e693d302eca..7fc78eaea0f7 100644 --- a/diesel_tests/tests/combination.rs +++ b/diesel_tests/tests/combination.rs @@ -129,19 +129,21 @@ fn union_with_limit() { ]; insert_into(users).values(&data).execute(conn).unwrap(); let data = users.order(id).load::(conn).unwrap(); - let sean = &data[0]; + let _sean = &data[0]; let tess = &data[1]; let _jim = &data[2]; - let expected_data = vec![User::new(sean.id, "Sean"), User::new(tess.id, "Tess")]; - let mut data: Vec<_> = users + let data: Vec = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) .limit(2) .load(conn) .unwrap(); - data.sort_by_key(|u: &User| u.id); - assert_eq!(expected_data, data); + + // Cannot determine any `expected_data` to match against because the database might return + // different rows + // The positional_order_by fix is not applicable, because ORDER should appear before LIMIT + assert_eq!(2, data.len()); } #[test] @@ -158,18 +160,20 @@ fn union_with_offset() { let data = users.order(id).load::(conn).unwrap(); let _sean = &data[0]; let tess = &data[1]; - let jim = &data[2]; + let _jim = &data[2]; - let expected_data = vec![User::new(tess.id, "Tess"), User::new(jim.id, "Jim")]; - let mut data: Vec<_> = users + let data: Vec = users .filter(id.le(tess.id)) .union(users.filter(id.ge(tess.id))) .limit(3) .offset(1) .load(conn) .unwrap(); - data.sort_by_key(|u: &User| u.id); - assert_eq!(expected_data, data); + + // Cannot determine any `expected_data` to match against because the database might return + // different rows + // The positional_order_by fix is not applicable, because ORDER should appear before LIMIT + assert_eq!(2, data.len()); } #[test] From 0a2dd9c7a9d78a9f59f4a754e93251eca477e024 Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Mon, 9 Sep 2024 13:15:05 +0200 Subject: [PATCH 10/11] Construct CombinationClause directly --- .../src/query_builder/combination_clause.rs | 46 ++++++------------- 1 file changed, 14 insertions(+), 32 deletions(-) diff --git a/diesel/src/query_builder/combination_clause.rs b/diesel/src/query_builder/combination_clause.rs index 62a58966d7e6..d3aed8b6fa0b 100644 --- a/diesel/src/query_builder/combination_clause.rs +++ b/diesel/src/query_builder/combination_clause.rs @@ -61,24 +61,6 @@ impl CombinationClause CombinationClause { - pub(crate) fn new_full( - combinator: Combinator, - duplicate_rule: Rule, - source: Source, - rhs: Rhs, - limit_offset: LOf, - ) -> Self { - CombinationClause { - combinator, - duplicate_rule, - source: ParenthesisWrapper(source), - rhs: ParenthesisWrapper(rhs), - limit_offset, - } - } -} - impl QueryDsl for CombinationClause { @@ -213,16 +195,16 @@ where fn limit(self, limit: i64) -> Self::Output { let limit_clause = LimitClause(limit.into_sql::()); - CombinationClause::new_full( - self.combinator, - self.duplicate_rule, - self.source.0, - self.rhs.0, - LimitOffsetClause { + CombinationClause { + combinator: self.combinator, + duplicate_rule: self.duplicate_rule, + source: self.source, + rhs: self.rhs, + limit_offset: LimitOffsetClause { limit_clause, offset_clause: self.limit_offset.offset_clause, }, - ) + } } } @@ -246,16 +228,16 @@ where fn offset(self, offset: i64) -> Self::Output { let offset_clause = OffsetClause(offset.into_sql::()); - CombinationClause::new_full( - self.combinator, - self.duplicate_rule, - self.source.0, - self.rhs.0, - LimitOffsetClause { + CombinationClause { + combinator: self.combinator, + duplicate_rule: self.duplicate_rule, + source: self.source, + rhs: self.rhs, + limit_offset: LimitOffsetClause { limit_clause: self.limit_offset.limit_clause, offset_clause, }, - ) + } } } From b156c92f45a1282082a4732b50e0ffc094207a29 Mon Sep 17 00:00:00 2001 From: Geoffroy Jaffa Planquart Date: Mon, 9 Sep 2024 13:35:55 +0200 Subject: [PATCH 11/11] Add changelog and better comment subpar test --- CHANGELOG.md | 1 + diesel_tests/tests/combination.rs | 20 ++++++++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffd696ce1526..86f3e4537285 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Increasing the minimal supported Rust version will always be coupled at least wi ### Added +* Added `limit()` and `offset()` DSL to combination clauses such as `UNION` * Fixed `#[derive(Identifiable)]` ignoring attribute `#[diesel(serialize_as)]` on primary keys * Added embedded struct support for `AsChangeset` via `#[diesel(embed)]` * Support for libsqlite3-sys 0.30.0 diff --git a/diesel_tests/tests/combination.rs b/diesel_tests/tests/combination.rs index 7fc78eaea0f7..8b912af89581 100644 --- a/diesel_tests/tests/combination.rs +++ b/diesel_tests/tests/combination.rs @@ -140,10 +140,25 @@ fn union_with_limit() { .load(conn) .unwrap(); + assert_eq!(2, data.len()); + // Cannot determine any `expected_data` to match against because the database might return // different rows - // The positional_order_by fix is not applicable, because ORDER should appear before LIMIT - assert_eq!(2, data.len()); + // + // The following code works with sqlite but fails with postgres: + // ```rust + // let expected_data = vec![User::new(sean.id, "Sean"), User::new(tess.id, "Tess")]; + // assert_eq!(expected_data, data); + // ``` + // + // thread 'combination::union_with_limit' panicked at diesel_tests/tests/combination.rs:144:5: + // assertion `left == right` failed + // left: [User { id: 236, name: "Sean", hair_color: None }, User { id: 237, name: "Tess", hair_color: None }] + // right: [User { id: 237, name: "Tess", hair_color: None }, User { id: 236, name: "Sean", hair_color: None }] + // + // Using positional_order_by to get predictable result is not possible because the ORDER + // fragment it produce would need to be placed in the middle of the fragment generated by the + // combination clause to be valid (and ORDER should be before LIMIT in an SQL query). } #[test] @@ -173,6 +188,7 @@ fn union_with_offset() { // Cannot determine any `expected_data` to match against because the database might return // different rows // The positional_order_by fix is not applicable, because ORDER should appear before LIMIT + // cf. union_with_limit for a lengthier explanation assert_eq!(2, data.len()); }