Only apply non-left-associative nesting to deferred join constraints - #2445
Only apply non-left-associative nesting to deferred join constraints#2445revitalkr wants to merge 3 commits into
Conversation
bf6c5e8 to
99aaa65
Compare
LucaCappelletti94
left a comment
There was a problem hiding this comment.
Since this PR changes the behavior of the method, I believe that the doc on Dialect::supports_left_associative_joins_without_parens should be updated. It still promises nesting for every dialect returning false, and still calls MySQL and Postgres left-associative.
I suggest to tackle in a subsequent PR that peek_parens_less_nested_join omits CROSS and NATURAL, so SELECT * FROM a JOIN b CROSS JOIN c ON a.id = c.id fails to parse although PostgreSQL accepts it.
| table_with_joins: Box::new(TableWithJoins { relation, joins }), | ||
| alias: None, | ||
| }; | ||
| let mut inner_joins = self.parse_joins()?; |
There was a problem hiding this comment.
Please refactor dropping the recursion, otherwise parse time goes superlinear (median of seven runs, release):
| joins | main |
this branch |
|---|---|---|
| 100 | 0.97 ms | 4.63 ms |
| 200 | 1.66 ms | 14.0 ms |
| 400 | 3.17 ms | 46.1 ms |
| 700 | 5.54 ms | 98.4 ms |
| let last = inner_joins.pop().expect("inner_joins is non-empty"); | ||
| let outer_constraint = if natural { | ||
| JoinConstraint::Natural | ||
| } else { | ||
| JoinConstraint::None | ||
| }; | ||
|
|
||
| joins.push(Join { | ||
| relation, | ||
| global, | ||
| join_operator: join_operator_type(outer_constraint), | ||
| }); | ||
| joins.extend(inner_joins); | ||
| joins.push(last); |
There was a problem hiding this comment.
pop then extend then push(last) rebuilds the same vector, and last pins a whole Join in every recursion frame.
| let last = inner_joins.pop().expect("inner_joins is non-empty"); | |
| let outer_constraint = if natural { | |
| JoinConstraint::Natural | |
| } else { | |
| JoinConstraint::None | |
| }; | |
| joins.push(Join { | |
| relation, | |
| global, | |
| join_operator: join_operator_type(outer_constraint), | |
| }); | |
| joins.extend(inner_joins); | |
| joins.push(last); | |
| let outer_constraint = if natural { | |
| JoinConstraint::Natural | |
| } else { | |
| JoinConstraint::None | |
| }; | |
| joins.push(Join { | |
| relation, | |
| global, | |
| join_operator: join_operator_type(outer_constraint), | |
| }); | |
| joins.extend(inner_joins); |
| fn parse_left_join_chain_with_and_without_left_associativity() { | ||
| let query = "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id"; | ||
|
|
||
| let generic_ast = Parser::parse_sql(&GenericDialect {}, query) | ||
| .unwrap() | ||
| .into_iter() | ||
| .next() | ||
| .unwrap(); | ||
| let generic_canonical = generic_ast.to_string(); | ||
| println!("Generic AST:\n{generic_ast:#?}"); | ||
| println!("Generic canonical:\n{generic_canonical}"); | ||
|
|
||
| assert_eq!( | ||
| generic_canonical, | ||
| "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id" | ||
| ); | ||
|
|
||
| let Statement::Query(generic_query) = &generic_ast else { | ||
| unreachable!() | ||
| }; | ||
| let SetExpr::Select(generic_select) = generic_query.body.as_ref() else { | ||
| unreachable!() | ||
| }; | ||
| let generic_from = only(&generic_select.from); | ||
| assert_eq!(generic_from.joins.len(), 2); | ||
|
|
||
| let snowflake_ast = Parser::parse_sql(&SnowflakeDialect {}, query) | ||
| .unwrap() | ||
| .into_iter() | ||
| .next() | ||
| .unwrap(); | ||
| let snowflake_canonical = snowflake_ast.to_string(); | ||
| println!("Snowflake AST:\n{snowflake_ast:#?}"); | ||
| println!("Snowflake canonical:\n{snowflake_canonical}"); | ||
|
|
||
| assert_eq!( | ||
| snowflake_canonical, | ||
| "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id" | ||
| ); | ||
|
|
||
| let Statement::Query(snowflake_query) = &snowflake_ast else { | ||
| unreachable!() | ||
| }; | ||
| let SetExpr::Select(snowflake_select) = snowflake_query.body.as_ref() else { | ||
| unreachable!() | ||
| }; | ||
| let snowflake_from = only(&snowflake_select.from); | ||
| assert_eq!(snowflake_from.joins.len(), 2); | ||
| } |
There was a problem hiding this comment.
| fn parse_left_join_chain_with_and_without_left_associativity() { | |
| let query = "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id"; | |
| let generic_ast = Parser::parse_sql(&GenericDialect {}, query) | |
| .unwrap() | |
| .into_iter() | |
| .next() | |
| .unwrap(); | |
| let generic_canonical = generic_ast.to_string(); | |
| println!("Generic AST:\n{generic_ast:#?}"); | |
| println!("Generic canonical:\n{generic_canonical}"); | |
| assert_eq!( | |
| generic_canonical, | |
| "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id" | |
| ); | |
| let Statement::Query(generic_query) = &generic_ast else { | |
| unreachable!() | |
| }; | |
| let SetExpr::Select(generic_select) = generic_query.body.as_ref() else { | |
| unreachable!() | |
| }; | |
| let generic_from = only(&generic_select.from); | |
| assert_eq!(generic_from.joins.len(), 2); | |
| let snowflake_ast = Parser::parse_sql(&SnowflakeDialect {}, query) | |
| .unwrap() | |
| .into_iter() | |
| .next() | |
| .unwrap(); | |
| let snowflake_canonical = snowflake_ast.to_string(); | |
| println!("Snowflake AST:\n{snowflake_ast:#?}"); | |
| println!("Snowflake canonical:\n{snowflake_canonical}"); | |
| assert_eq!( | |
| snowflake_canonical, | |
| "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id" | |
| ); | |
| let Statement::Query(snowflake_query) = &snowflake_ast else { | |
| unreachable!() | |
| }; | |
| let SetExpr::Select(snowflake_select) = snowflake_query.body.as_ref() else { | |
| unreachable!() | |
| }; | |
| let snowflake_from = only(&snowflake_select.from); | |
| assert_eq!(snowflake_from.joins.len(), 2); | |
| } | |
| fn parse_join_chain_without_deferred_constraint() { | |
| let select = all_dialects().verified_only_select( | |
| "SELECT * FROM orders AS o JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id", | |
| ); | |
| let from = only(&select.from); | |
| assert_eq!(from.joins.len(), 2); | |
| assert!(matches!( | |
| from.joins[0].join_operator, | |
| JoinOperator::Join(JoinConstraint::None) | |
| )); | |
| assert!(matches!( | |
| from.joins[1].join_operator, | |
| JoinOperator::Left(JoinConstraint::On(_)) | |
| )); | |
| } |
| fn parse_left_join_chain_with_and_without_left_associativity_cross_join() { | ||
| let query = "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o CROSS JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id"; | ||
|
|
||
| let generic_ast = Parser::parse_sql(&GenericDialect {}, query) | ||
| .unwrap() | ||
| .into_iter() | ||
| .next() | ||
| .unwrap(); | ||
| let generic_canonical = generic_ast.to_string(); | ||
| println!("Generic AST:\n{generic_ast:#?}"); | ||
| println!("Generic canonical:\n{generic_canonical}"); | ||
|
|
||
| assert_eq!( | ||
| generic_canonical, | ||
| "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o CROSS JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id" | ||
| ); | ||
|
|
||
| let Statement::Query(generic_query) = &generic_ast else { | ||
| unreachable!() | ||
| }; | ||
| let SetExpr::Select(generic_select) = generic_query.body.as_ref() else { | ||
| unreachable!() | ||
| }; | ||
| let generic_from = only(&generic_select.from); | ||
| assert_eq!(generic_from.joins.len(), 2); | ||
|
|
||
| let snowflake_ast = Parser::parse_sql(&SnowflakeDialect {}, query) | ||
| .unwrap() | ||
| .into_iter() | ||
| .next() | ||
| .unwrap(); | ||
| let snowflake_canonical = snowflake_ast.to_string(); | ||
| println!("Snowflake AST:\n{snowflake_ast:#?}"); | ||
| println!("Snowflake canonical:\n{snowflake_canonical}"); | ||
|
|
||
| assert_eq!( | ||
| snowflake_canonical, | ||
| "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o CROSS JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id" | ||
| ); | ||
|
|
||
| let Statement::Query(snowflake_query) = &snowflake_ast else { | ||
| unreachable!() | ||
| }; | ||
| let SetExpr::Select(snowflake_select) = snowflake_query.body.as_ref() else { | ||
| unreachable!() | ||
| }; | ||
| let snowflake_from = only(&snowflake_select.from); | ||
| assert_eq!(snowflake_from.joins.len(), 2); | ||
| } |
There was a problem hiding this comment.
| fn parse_left_join_chain_with_and_without_left_associativity_cross_join() { | |
| let query = "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o CROSS JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id"; | |
| let generic_ast = Parser::parse_sql(&GenericDialect {}, query) | |
| .unwrap() | |
| .into_iter() | |
| .next() | |
| .unwrap(); | |
| let generic_canonical = generic_ast.to_string(); | |
| println!("Generic AST:\n{generic_ast:#?}"); | |
| println!("Generic canonical:\n{generic_canonical}"); | |
| assert_eq!( | |
| generic_canonical, | |
| "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o CROSS JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id" | |
| ); | |
| let Statement::Query(generic_query) = &generic_ast else { | |
| unreachable!() | |
| }; | |
| let SetExpr::Select(generic_select) = generic_query.body.as_ref() else { | |
| unreachable!() | |
| }; | |
| let generic_from = only(&generic_select.from); | |
| assert_eq!(generic_from.joins.len(), 2); | |
| let snowflake_ast = Parser::parse_sql(&SnowflakeDialect {}, query) | |
| .unwrap() | |
| .into_iter() | |
| .next() | |
| .unwrap(); | |
| let snowflake_canonical = snowflake_ast.to_string(); | |
| println!("Snowflake AST:\n{snowflake_ast:#?}"); | |
| println!("Snowflake canonical:\n{snowflake_canonical}"); | |
| assert_eq!( | |
| snowflake_canonical, | |
| "SELECT 'ORIGINAL' AS src, o.order_id, c.customer_id, p.product_id FROM orders AS o CROSS JOIN customers AS c LEFT JOIN products AS p ON p.order_id = o.order_id ORDER BY o.order_id, c.customer_id, p.product_id" | |
| ); | |
| let Statement::Query(snowflake_query) = &snowflake_ast else { | |
| unreachable!() | |
| }; | |
| let SetExpr::Select(snowflake_select) = snowflake_query.body.as_ref() else { | |
| unreachable!() | |
| }; | |
| let snowflake_from = only(&snowflake_select.from); | |
| assert_eq!(snowflake_from.joins.len(), 2); | |
| } | |
| #[test] | |
| fn parse_long_join_chain_without_deferred_constraint() { | |
| let mut sql = String::from("SELECT * FROM t0"); | |
| for index in 1..=200 { | |
| sql.push_str(&format!(" JOIN t{index}")); | |
| } | |
| non_left_associative_dialects().verified_stmt(&sql); | |
| } |
Summary
For dialects with
supports_left_associative_joins_without_parens = false, nested joins are currently created even when no deferred join constraint exists.Problem
The non-left-associative join handling was introduced to support deferred join constraints such as:
A JOIN B JOIN C ON X ON Y
However, the same logic is also applied to queries such as:
A JOIN B LEFT JOIN C ON X
even though there is no deferred ON or USING constraint to attach.
This produces a nested join structure where a flat join chain is expected.
Solution
Only apply the nesting logic when a deferred join constraint (ON or USING) remains to be attached.
Queries without deferred constraints continue to use the normal flat join structure.
Test
Adds coverage for:
SELECT 'ORIGINAL' AS src,
o.order_id,
c.customer_id,
p.product_id
FROM orders AS o
JOIN customers AS c
LEFT JOIN products AS p
ON p.order_id = o.order_id
and verifies that both Generic and Snowflake dialects produce a flat join chain (joins.len() == 2).
Validation - Snowflake script
The following query executes successfully in Snowflake:
SELECT 'ORIGINAL' AS src,
o.order_id,
c.customer_id,
p.product_id
FROM orders AS o
JOIN customers AS c
LEFT JOIN products AS p
ON p.order_id = o.order_id;
and matches the flat interpretation:
(orders AS o JOIN customers AS c)
LEFT JOIN products AS p
ON p.order_id = o.order_id
while the nested interpretation generated by the current non-left-associative handling:
orders AS o
JOIN (
customers AS c
LEFT JOIN products AS p
ON p.order_id = o.order_id
)
is rejected by Snowflake.