xiedeyantu commented on PR #21058:
URL: https://github.com/apache/datafusion/pull/21058#issuecomment-4148408141
> It's a little unfortunate that we don't support duplicate grouping sets
for 8/16/32 grouping columns. I guess there's no easy way around that?
>
> `grouping_function_on_id` has this code:
>
> ```rust
> let grouping_id_column =
Expr::Column(Column::from(Aggregate::INTERNAL_GROUPING_ID));
> // The grouping call is exactly our internal grouping id
> if args.len() == group_by_expr_count
> && args
> .iter()
> .rev()
> .enumerate()
> .all(|(idx, expr)| group_by_expr.get(expr) == Some(&idx))
> {
> return Ok(cast(grouping_id_column, DataType::Int32));
> }
> ```
>
> We probably want to mask out the ordinal bits that this PR adds in to the
grouping ID?
@neilconway Apologies for the delayed response. I have reviewed all your
thoughtful comments and agree that the approach using high-bit flags indeed has
significant flaws. I have now refactored the entire logic and updated the PR
description; if you have the time, could you please help review it again? Thank
you!
--
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]
