-
Notifications
You must be signed in to change notification settings - Fork 609
Parse ARRAY_AGG for Bigquery and Snowflake #662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,12 @@ pub trait Dialect: Debug + Any { | |
fn supports_filter_during_aggregation(&self) -> bool { | ||
false | ||
} | ||
/// Returns true if the dialect supports ARRAY_AGG() [WITHIN GROUP (ORDER BY)] expressions. | ||
/// Otherwise, the dialect should expect an `ORDER BY` without the `WITHIN GROUP` clause, e.g. `ANSI` [(1)]. | ||
/// [(1)]: https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#array-aggregate-function | ||
fn supports_within_after_array_aggregation(&self) -> bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is confusing. The comment make it sounds like if any of both is valid, it will return true. Could be something like /// Returns true if the dialect supports ARRAY_AGG() [WITHIN GROUP (ORDER BY)] expressions.
///
/// Otherwise, the dialect should expect an `ORDER BY` without the `WITHIN GROUP` clause, e.g. `ANSI` [(1)].
/// [(1)]: https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#array-aggregate-function
Having the function here is not my usual approach, as it will require maintenance over other dialects individually, but I don't think it's wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, @SuperBo, one thing:
@alamb what do you think? This is a divergence from dialects. I'd go for the one with more dialects, so the generic is as generic as possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you are right, the ANSI one should be default option. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AugustoFKL, are we good to go with this or should we change to more generics behavior? |
||
false | ||
} | ||
/// Dialect-specific prefix parser override | ||
fn parse_prefix(&self, _parser: &mut Parser) -> Option<Result<Expr, ParserError>> { | ||
// return None to fall back to the default behavior | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SuperBo any reason why you did the implementation for all dialects the same time?
You didn't complete none, but added parts of different ones. Got me a little confused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this. You can review again