iffyio commented on code in PR #1880:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1880#discussion_r2140409425
##########
src/parser/mod.rs:
##########
@@ -11516,7 +11524,10 @@ impl<'a> Parser<'a> {
}
} else {
None
- };
+ }
+ } else {
+ None
Review Comment:
would it make sense to unnest the logic with something like?
```rust
if self.parse_keywords([Keyword::DISTINCT, Keyword::AS, Keyword::VALUE]) {
Some()
} else if self.parse_keywords([Keyword::DISTINCT, Keyword::AS,
Keyword::STRUCT]) {
Some()
} else if self.parse_keywords([Keyword::AS, Keyword::VALUE]) {
Some()
} else if self.parse_keywords([Keyword::AS, Keyword::STRUCT]) {
Some()
} else {
None
};
```
Also maybe we can move the logic to its own method now like `let
value_table_mode = self.parse_value_table_mode()`, since its no longer trivial
##########
src/ast/query.rs:
##########
@@ -3340,20 +3340,29 @@ impl fmt::Display for OpenJsonTableColumn {
/// BigQuery supports ValueTables which have 2 modes:
/// `SELECT AS STRUCT`
/// `SELECT AS VALUE`
+///
+/// They can be combined with `[ { ALL | DISTINCT } ]`, e.g.
+/// `SELECT DISTINCT AS STRUCT`
+///
Review Comment:
```suggestion
/// `SELECT [DISTINCT] AS STRUCT`
/// `SELECT [DISTINCT] AS VALUE`
///
```
thinking we can do this instead to flag optionality. Or we can include the
specific examples as doc strings on each variant
##########
tests/sqlparser_bigquery.rs:
##########
@@ -2377,3 +2377,14 @@ fn test_any_type() {
fn test_any_type_dont_break_custom_type() {
bigquery_and_generic().verified_stmt("CREATE TABLE foo (x ANY)");
}
+
+#[test]
+fn test_select_distinct_as_struct_or_value() {
+ for sql in [
+ "SELECT DISTINCT AS STRUCT a, ABS(b) FROM UNNEST(c) AS T",
+ "SELECT DISTINCT AS VALUE a, ABS(b) FROM UNNEST(c) AS T",
+ "SELECT ARRAY(SELECT DISTINCT AS STRUCT a, b, ABS(c) AS c, ABS(d) AS d
FROM UNNEST(e) AS T)",
+ ] {
+ bigquery().verified_stmt(sql);
+ }
Review Comment:
thinking we can extend [this test
case](https://github.com/apache/datafusion-sqlparser-rs/blob/6a774dabfc3573679a3ba392244f20461c0cb0f5/tests/sqlparser_bigquery.rs#L2322)
instead of adding a new one I think, since they're the same feature
--
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]
