iffyio commented on code in PR #1799:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1799#discussion_r2034488630
##########
src/parser/mod.rs:
##########
@@ -11823,7 +11828,16 @@ impl<'a> Parser<'a> {
}
_ => break,
};
- let relation = self.parse_table_factor()?;
+ let mut relation = self.parse_table_factor()?;
+
+ if self.is_next_token_a_join() {
+ let joins = self.parse_joins()?;
+ relation = TableFactor::NestedJoin {
+ table_with_joins: Box::new(TableWithJoins { relation,
joins }),
+ alias: None,
+ };
+ }
Review Comment:
would it make more sense to have this logic as part of the table factor
parsing instead? thinking that would be by definition, and so that any caller
of `self.parse_table_factor` gets the full behavior including nested joins.
i imagine essentially to split table factor parsing instead e.g.
```rust
fn parse_table_factor() {
let table_factor = self.parse_table_factor_inner()?;
if self.is_next_token_a_join() {
// ...
}
}
```
##########
tests/sqlparser_snowflake.rs:
##########
@@ -3516,3 +3516,87 @@ fn test_alter_session_followed_by_statement() {
_ => panic!("Unexpected statements: {:?}", stmts),
}
}
+
+#[test]
+fn test_nested_join_without_parentheses() {
+ let query = "SELECT DISTINCT p.product_id FROM orders AS o INNER JOIN
customers AS c INNER JOIN products AS p ON p.customer_id = c.customer_id ON
c.order_id = o.order_id";
Review Comment:
Can we add coverage for all the supported join types?
##########
tests/sqlparser_snowflake.rs:
##########
@@ -3516,3 +3516,87 @@ fn test_alter_session_followed_by_statement() {
_ => panic!("Unexpected statements: {:?}", stmts),
}
}
+
+#[test]
+fn test_nested_join_without_parentheses() {
+ let query = "SELECT DISTINCT p.product_id FROM orders AS o INNER JOIN
customers AS c INNER JOIN products AS p ON p.customer_id = c.customer_id ON
c.order_id = o.order_id";
+ assert_eq!(
+ only(
+ snowflake()
+ .verified_only_select_with_canonical(query, "")
Review Comment:
oh what does the empty string second argument imply?
##########
src/parser/mod.rs:
##########
@@ -11833,7 +11847,21 @@ impl<'a> Parser<'a> {
};
joins.push(join);
}
- Ok(TableWithJoins { relation, joins })
+ Ok(joins)
+ }
+
+ fn is_next_token_a_join(&self) -> bool {
+ matches!(
+ self.peek_token().token,
Review Comment:
```suggestion
self.peek_token_ref().token,
```
##########
src/parser/mod.rs:
##########
@@ -11833,7 +11847,21 @@ impl<'a> Parser<'a> {
};
joins.push(join);
}
- Ok(TableWithJoins { relation, joins })
+ Ok(joins)
+ }
+
+ fn is_next_token_a_join(&self) -> bool {
Review Comment:
```suggestion
fn peek_parens_less_nested_join(&self) -> bool {
```
maybe something like this naming wise? peek uses existing parser
terminology. Also would it be possible with a doc comment of the function
mentioning the use case? I think that would be helpful to future readers
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
