iffyio commented on code in PR #1653:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1653#discussion_r1948012742
##########
src/parser/mod.rs:
##########
@@ -9092,6 +9092,16 @@ impl<'a> Parser<'a> {
});
}
}
+ if self.dialect.supports_group_by_special_grouping_sets()
+ && self.parse_keywords(&[Keyword::GROUPING, Keyword::SETS])
+ {
+ self.expect_token(&Token::LParen)?;
+ let result = self.parse_comma_separated(|p|
p.parse_tuple(true, true))?;
Review Comment:
should this be comma separate expr or similar instead of tuple?
here's one example from the [hive
doc](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=30151323#EnhancedAggregation,Cube,GroupingandRollup-GROUPINGSETSclause)
that we seem to not be able to support as a result of using parse_tuple
```sql
SELECT a, b, SUM( c ) FROM tab1 GROUP BY a, b GROUPING SETS ( (a, b), a, b,
( ) )
```
##########
src/parser/mod.rs:
##########
@@ -9092,6 +9092,16 @@ impl<'a> Parser<'a> {
});
}
}
+ if self.dialect.supports_group_by_special_grouping_sets()
Review Comment:
I wonder if this dialect method is needed? the clause isn't special from
what I can tell its the normal grouping set clause so I'm thinking we can skip
the method and parse unconditionally
(the current setup seems to have similar impls/repr for group by clauses,
other being `parse_group_by_expr`, which isn't ideal but I think that's out of
scope for this PR).
##########
src/ast/query.rs:
##########
@@ -2537,6 +2542,19 @@ impl fmt::Display for GroupByWithModifier {
GroupByWithModifier::Rollup => write!(f, "WITH ROLLUP"),
GroupByWithModifier::Cube => write!(f, "WITH CUBE"),
GroupByWithModifier::Totals => write!(f, "WITH TOTALS"),
+ GroupByWithModifier::GroupingSets(expr) => match expr {
+ Expr::GroupingSets(sets) => {
+ write!(f, "GROUPING SETS (")?;
+ let mut sep = "";
+ for set in sets {
+ write!(f, "{sep}")?;
+ sep = ", ";
+ write!(f, "({})", display_comma_separated(set))?;
+ }
+ write!(f, ")")
+ }
+ _ => unreachable!(),
Review Comment:
I think the panic here would be undesirable in any case. Can we reuse the
existing impl on expr? its not clear to me why the display cares about the kind
of expression here. e.g.
```rust
GroupByWithModifier::GroupingSets(expr) => {
write!(f, "{expr}")
}
```
##########
src/dialect/mod.rs:
##########
@@ -245,6 +245,16 @@ pub trait Dialect: Debug + Any {
false
}
+ /// Returns true if the dialects supports `with rollup/cube/all`
expressions.
Review Comment:
```suggestion
/// Returns true if the dialects supports `GROUP BY` modifiers prefixed
by a `WITH` keyword.
/// Example: `GROUP BY value WITH ROLLUP`.
```
--
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]
