iffyio commented on code in PR #1645:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1645#discussion_r1909605886
##########
tests/sqlparser_snowflake.rs:
##########
@@ -2483,6 +2483,37 @@ fn test_sf_trailing_commas() {
snowflake().verified_only_select_with_canonical("SELECT 1, 2, FROM t",
"SELECT 1, 2 FROM t");
}
+#[test]
+fn test_sf_trailing_commas_in_from() {
+ snowflake().verified_only_select_with_canonical("SELECT 1, 2 FROM t,",
"SELECT 1, 2 FROM t");
+}
+
+#[test]
+fn test_sf_trailing_commas_multiple_sources() {
+ snowflake()
+ .verified_only_select_with_canonical("SELECT 1, 2 FROM t1, t2,",
"SELECT 1, 2 FROM t1, t2");
+}
+
+#[test]
+fn test_from_trailing_commas_with_limit() {
+ let sql = "SELECT a, FROM b, LIMIT 1";
+ let _ = snowflake().parse_sql_statements(sql).unwrap();
+}
+
+#[test]
+fn test_sf_trailing_commas_multiple_subqueries() {
+ snowflake().verified_only_select_with_canonical(
+ "SELECT 1, 2 FROM (SELECT * FROM t1), (SELECT * FROM t2),",
+ "SELECT 1, 2 FROM (SELECT * FROM t1), (SELECT * FROM t2)",
+ );
+}
+
+#[test]
+fn test_from_with_nested_trailing_commas() {
+ let sql = "SELECT 1, 2 FROM (SELECT * FROM t,),";
Review Comment:
Could we merge these test scenarios into a single test case? since they're
testing the same feature
##########
src/parser/mod.rs:
##########
@@ -3947,20 +3951,31 @@ impl<'a> Parser<'a> {
Ok(values)
}
+ //Parse a 'FROM' statment. support trailing comma
+ pub fn parse_from(&mut self) -> Result<Vec<TableWithJoins>, ParserError> {
Review Comment:
```suggestion
/// Parse a list of [TableWithJoins]
fn parse_table_with_joins(&mut self) -> Result<Vec<TableWithJoins>,
ParserError> {
```
Thinking no need to make this function public. And `parse_from` sounded
generic since the actual `FROM` keyword isn't part of the parsed elements
##########
src/dialect/mod.rs:
##########
@@ -758,6 +763,11 @@ pub trait Dialect: Debug + Any {
keywords::RESERVED_FOR_IDENTIFIER.contains(&kw)
}
+ // Returns list of keyword allowed after from
Review Comment:
```suggestion
/// Returns reserved keywords when looking to parse a [TableFactor].
/// See [Self::supports_from_trailing_commas]
```
##########
src/dialect/mod.rs:
##########
@@ -399,6 +399,11 @@ pub trait Dialect: Debug + Any {
self.supports_trailing_commas()
}
+ /// Does the dialect support trailing commas in the FROM clause list?
Review Comment:
```suggestion
/// Returns true if the dialect supports trailing commas in the `FROM`
clause of a `SELECT` statement.
/// Example: `SELECT 1 FROM T, U, LIMIT 1`
```
##########
tests/sqlparser_snowflake.rs:
##########
@@ -2483,6 +2483,37 @@ fn test_sf_trailing_commas() {
snowflake().verified_only_select_with_canonical("SELECT 1, 2, FROM t",
"SELECT 1, 2 FROM t");
}
+#[test]
Review Comment:
Could we move the tests to common and use the
[all_dialects_where](https://github.com/apache/datafusion-sqlparser-rs/blob/5f3de315842963e6db47c9f30ddc914dc7c605c9/tests/sqlparser_common.rs#L8254)
function? That would make it automatic to add test coverage for other dialects
that have this feature enabled in the future
##########
src/keywords.rs:
##########
@@ -975,6 +975,16 @@ pub const RESERVED_FOR_COLUMN_ALIAS: &[Keyword] = &[
Keyword::END,
];
+// Global list of reserved keywords alloweed after FROM.
+// Parser should call Dialect::get_reserved_keyword_after_from
+// to allow for each dialect to customize the list.
+pub const ALLOWED_KEYWORD_AFTER_FROM: &[Keyword] = &[
Review Comment:
```suggestion
pub const RESERVED_FOR_TABLE_FACTOR: &[Keyword] = &[
```
Thinking something like this to keep the naming somewhat similar to the
existing lists?
If this changes the dialect method `get_reserved_keyword_after_from` would
be updated to match the naming
##########
tests/sqlparser_snowflake.rs:
##########
@@ -2483,6 +2483,37 @@ fn test_sf_trailing_commas() {
snowflake().verified_only_select_with_canonical("SELECT 1, 2, FROM t",
"SELECT 1, 2 FROM t");
}
+#[test]
+fn test_sf_trailing_commas_in_from() {
+ snowflake().verified_only_select_with_canonical("SELECT 1, 2 FROM t,",
"SELECT 1, 2 FROM t");
+}
+
+#[test]
+fn test_sf_trailing_commas_multiple_sources() {
+ snowflake()
+ .verified_only_select_with_canonical("SELECT 1, 2 FROM t1, t2,",
"SELECT 1, 2 FROM t1, t2");
+}
+
+#[test]
+fn test_from_trailing_commas_with_limit() {
+ let sql = "SELECT a, FROM b, LIMIT 1";
Review Comment:
Can we add a similar scenario for the other entries in the allowed keyword
list? (i.e. INTO, HAVING, WHERE)
##########
src/parser/mod.rs:
##########
@@ -3974,15 +3989,26 @@ impl<'a> Parser<'a> {
/// Parse the comma of a comma-separated syntax element.
/// Returns true if there is a next element
fn is_parse_comma_separated_end(&mut self) -> bool {
-
self.is_parse_comma_separated_end_with_trailing_commas(self.options.trailing_commas)
+ self.is_parse_comma_separated_end_with_trailing_commas(
+ self.options.trailing_commas,
+ self.default_reserved_keywords(),
+ )
+ }
+
+ fn default_reserved_keywords(&self) -> &'static [Keyword] {
+ keywords::RESERVED_FOR_COLUMN_ALIAS
}
Review Comment:
the function name `default_reserved_keywords` sounds a bit misleading out of
context, I wonder would it make sense to have the parameter optional and use it
internally as the default? e.g.
```rust
fn is_parse_comma_separated_end_with_trailing_commas(
&mut self,
trailing_commas: bool,
reserved_keywords: Option<&[Keyword]>,
) {
let reserved_keywords =
reserved_keywords.unwrap_or(keywords::RESERVED_FOR_COLUMN_ALIAS)
}
```
--
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]
