LucaCappelletti94 commented on code in PR #2445:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2445#discussion_r3896048549
##########
src/parser/mod.rs:
##########
@@ -16423,14 +16423,42 @@ impl<'a> Parser<'a> {
if !self
.dialect
.supports_left_associative_joins_without_parens()
- && !natural
&& self.peek_parens_less_nested_join()
{
- let joins = self.parse_joins()?;
- relation = TableFactor::NestedJoin {
- table_with_joins: Box::new(TableWithJoins { relation,
joins }),
- alias: None,
- };
+ let mut inner_joins = self.parse_joins()?;
Review 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 |
##########
src/parser/mod.rs:
##########
@@ -16423,14 +16423,42 @@ impl<'a> Parser<'a> {
if !self
.dialect
.supports_left_associative_joins_without_parens()
- && !natural
&& self.peek_parens_less_nested_join()
{
- let joins = self.parse_joins()?;
- relation = TableFactor::NestedJoin {
- table_with_joins: Box::new(TableWithJoins { relation,
joins }),
- alias: None,
- };
+ let mut inner_joins = self.parse_joins()?;
+ let has_deferred_constraint = matches!(
+ self.peek_token_ref().token,
+ Token::Word(Word {
+ keyword: Keyword::ON | Keyword::USING,
+ ..
+ })
+ );
+
+ if has_deferred_constraint {
+ relation = TableFactor::NestedJoin {
+ table_with_joins: Box::new(TableWithJoins {
+ relation,
+ joins: inner_joins,
+ }),
+ alias: None,
+ };
+ } else {
+ 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);
Review Comment:
`pop` then `extend` then `push(last)` rebuilds the same vector, and `last`
pins a whole `Join` in every recursion frame.
```suggestion
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);
```
##########
tests/sqlparser_common.rs:
##########
@@ -17592,6 +17592,108 @@ fn join_precedence() {
);
}
+#[test]
+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);
+}
Review Comment:
```suggestion
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(_))
));
}
```
##########
tests/sqlparser_common.rs:
##########
@@ -17592,6 +17592,108 @@ fn join_precedence() {
);
}
+#[test]
+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);
+}
+
+#[test]
+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);
+}
Review Comment:
```suggestion
#[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);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
