feat: Implement grouping function using grouping id #12704
Conversation
github-actions
bot
added
logical-expr
eejbyfeldt
force-pushed
the
implement-grouping-function-using-grouping-id
branch
from
6b6bf52
to
b67aee9
Compare
eejbyfeldt
force-pushed
the
implement-grouping-function-using-grouping-id
branch
from
b67aee9
to
be38977
Compare
github-actions
bot
removed
logical-expr
eejbyfeldt
force-pushed
the
implement-grouping-function-using-grouping-id
branch
from
be38977
to
bb53649
Compare
|
Are @comphead (because of your review here #12565) or @thinkharderdev (becaues of your review on #12571) be willing to do a review of this one? |
| // Postgres allows grouping function for group by without grouping sets, the result is then | ||
| // always 0 | ||
| if !is_grouping_set { | ||
| return Ok(Expr::Literal(ScalarValue::from(0u32))); |
There was a problem hiding this comment.
should it be u32, or can be u8?
There was a problem hiding this comment.
u32 was incorrect. Currently the return type of the aggregate function is i32 so changed it to that.
The return value of the grouping function should always be the same type. The u8, u16 etc. is only an optimization for the internal grouping id. But we always cast it to i32 here, when computing the grouping column.
| fn is_grouping_function(expr: &Expr) -> bool { | ||
| // TODO: Do something better than name here should grouping be a built | ||
| // in expression? | ||
| matches!(expr, Expr::AggregateFunction(AggregateFunction { ref func, .. }) if func.name() == "grouping") |
There was a problem hiding this comment.
perhaps we can extend AggregateFunction 🤔
There was a problem hiding this comment.
I was thinking maybe adding Expr::Grouping but we match on Expr exhaustively in quite a few places so it becomes a semi big change.
The thing with the grouping function does not look that much like a aggregate function except for that where it can appear in sql. But it does not support extra AggregateFunction stuff like order by/null treatment/filter/distinct.
This patch adds a Analyzer rule to transform the grouping aggreation function into computation ontop of the grouping id that is used internally for grouping sets.
eejbyfeldt
force-pushed
the
implement-grouping-function-using-grouping-id
branch
from
bb53649
to
8acd5cb
Compare
|
Thanks @eejbyfeldt and @comphead |
Which issue does this PR close?
Closes #5647 .
Closes #10208
Rationale for this change
Grouping function is supported by postgres and needed inorder to be able to run TPC-DS (#4763).
What changes are included in this PR?
This PR adds an analyzer rule that will transform the grouping aggregation function into a projection on top of grouping id (added in #12571). Since the grouping aggregation function is so specific and special (and not really a aggregation), I don't think it make sense to generalize the UDAF api to support it.
This PR currently leaves the non-functional grouping UDAF. But since it non functional and will remain so, we might want to replace it with some built in
Expr::Groupinginstead. Would love some more feedback if that should be done in this PR or some other alternative.Are these changes tested?
New slt, mostly copied from #10208 with some additional test of wrong arguments.
Are there any user-facing changes?
Yes, after this grouping function will be usable.