Parse ARRAY_AGG for Bigquery and Snowflake #662
Conversation
|
@AugustoFKL do you have time to review this PR? |
Pull Request Test Coverage Report for Build 3435201483
💛 - Coveralls |
|
@SuperBo please fix the linter warnings |
| pub distinct: bool, | ||
| pub expr: Box<Expr>, | ||
| pub order_by: Option<Box<OrderByExpr>>, | ||
| pub limit: Option<Box<Expr>>, |
There was a problem hiding this comment.
@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.
I changed this. You can review again
| @@ -71,6 +71,10 @@ pub trait Dialect: Debug + Any { | |||
| fn supports_filter_during_aggregation(&self) -> bool { | |||
| false | |||
| } | |||
| /// Does the dialect supports ARRAY_AGG() [WITHIN GROUP (ORDER BY)] or ARRAY_AGG([ORDER BY]) | |||
| fn supports_within_after_array_aggregation(&self) -> bool { | |||
There was a problem hiding this comment.
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.
Also, @SuperBo, one thing:
- Generic dialect should be the most permissive one so, if you actually can have both structures in a query, generic dialect should allow both. But considering my previous comment, I'd allow the most common one, maybe?
@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.
I think you are right, the ANSI one should be default option.
There was a problem hiding this comment.
@AugustoFKL, are we good to go with this or should we change to more generics behavior?
SuperBo
force-pushed
the
add-array-agg-support
branch
from
c1af40b to
64efa11
Compare
SuperBo
force-pushed
the
add-array-agg-support
branch
from
64efa11 to
a1c179b
Compare
Can drop this after rebase on commit 87b4a16 "Parse ARRAY_AGG for Bigquery and Snowflake (apache#662)", first released in 0.27.0
Can drop this after rebase on commit 87b4a16 "Parse ARRAY_AGG for Bigquery and Snowflake (apache#662)", first released in 0.27.0
There are subtle differences of ARRAY_AGG in SQL dialects:
This the very first attempt to support correct ARRAY_AGG function while parsing BigQuery and snowflake.