LucaCappelletti94 commented on code in PR #2521:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2521#discussion_r4098154756
##########
src/parser/mod.rs:
##########
@@ -13546,9 +13559,18 @@ impl<'a> Parser<'a> {
result,
)));
};
- let group_by = match expressions {
- None => GroupByExpr::All(modifiers),
- Some(exprs) => GroupByExpr::Expressions(exprs, modifiers),
+ let group_by = match (modifier, expressions) {
+ (None, None) => GroupByExpr::All(modifiers),
+ (Some(modifier), Some(exprs)) => {
+ GroupByExpr::ExpressionsWithModifier(modifier, exprs,
modifiers)
+ }
+ (None, Some(exprs)) => GroupByExpr::Expressions(exprs,
modifiers),
+ (Some(_), None) => {
+ return parser_err!(
+ "BUG: GROUP BY modifier requires expressions",
+ self.peek_token_ref().span.start
+ )
+ }
};
Review Comment:
You could drop the `(Some(_), None)` arm. It can never run, because a parsed
modifier always leads to parsed expressions, and matching on `expressions`
first makes the match exhaustive without it.
```suggestion
let group_by = match (expressions, modifier) {
(None, _) => GroupByExpr::All(modifiers),
(Some(exprs), None) => GroupByExpr::Expressions(exprs,
modifiers),
(Some(exprs), Some(modifier)) => {
GroupByExpr::ExpressionsWithModifier(modifier, exprs,
modifiers)
}
};
```
##########
src/ast/query.rs:
##########
Review Comment:
You may want to update the enum doc, which still says `GROUP BY` has two
forms now that `ExpressionsWithModifier` adds a third.
```suggestion
/// Represents the three syntactic forms that `GROUP BY` can take, `GROUP BY
ALL`
/// with optional modifiers, ordinary `GROUP BY <exprs>` and `GROUP BY ALL |
DISTINCT <exprs>`.
```
##########
src/ast/query.rs:
##########
@@ -3819,6 +3840,10 @@ pub enum GroupByExpr {
All(Vec<GroupByWithModifier>),
/// `GROUP BY <expressions>` with optional modifiers.
Expressions(Vec<Expr>, Vec<GroupByWithModifier>),
+ /// `GROUP BY ALL | DISTINCT <expressions>` with optional modifiers.
Review Comment:
You should reference `[PostgreSQL]` in the text. A link reference definition
with no reference renders nothing, so `cargo doc` output for `GroupByExpr`
currently has no link to the PostgreSQL docs.
```suggestion
/// `GROUP BY ALL | DISTINCT <expressions>` of [PostgreSQL] with
optional modifiers.
```
--
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]
