eejbyfeldt commented on code in PR #12565:
URL: https://github.com/apache/datafusion/pull/12565#discussion_r1775747830
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -578,7 +579,13 @@ impl GroupedHashAggregateStream {
/// [`GroupsAccumulatorAdapter`] if not.
pub(crate) fn create_group_accumulator(
agg_expr: &AggregateFunctionExpr,
+ group_by: &PhysicalGroupBy,
) -> Result<Box<dyn GroupsAccumulator>> {
+ // GROUPING is a special fxn that exposes info about group organization
+ if let Some(grouping) =
agg_expr.fun().inner().as_any().downcast_ref::<Grouping>() {
+ let args = agg_expr.all_expressions().args;
+ return grouping.create_grouping_accumulator(&args, &group_by.expr);
+ }
Review Comment:
> It's kind of in a weird place, it's sort of not a real aggregation
function but instead a way to leak metadata. That might be a reason to make it
a built-in.
> Do you have ideas about how and when to go about doing that?
One way might be that we expose the `grouping_id` column used in
https://github.com/apache/datafusion/pull/12571 and implement the function as
transformation on that. This should be possible as that column should "leak"
the needed metadata. This is what was proposed in
https://github.com/apache/datafusion/pull/5749
--
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]
