namanjain24-sudo commented on PR #25527: URL: https://github.com/apache/datafusion/pull/25527#issuecomment-6200847345
Fair point on the metric, happy to count tests in too. Examples — these are two real ones out of the test suite (`aggregate_grouping_sets_keep_grouping_function`, `aggregate_duplicate_grouping_sets`), already passing in CI: ```sql -- (a) first, then (c): __grouping_id's mask comes out 1 then 2, but the index Substrait wants is 0 then 1 SELECT a, c, GROUPING(a) AS ga, GROUPING(c) AS gc, avg(b) FROM data GROUP BY GROUPING SETS ((a), (c), (a, c)); ``` mask(a)=1 -> index 0, mask(c)=2 -> index 1, mask(a,c)=0 -> index 2. Reading the mask straight as the index is off on every row here. This is the actual bug: the old code wrote the bitmask into the column Substrait defines as the index. ```sql -- (a) repeated: both copies have mask 0, only the ordinal tells them apart SELECT a, GROUPING(a) AS ga, avg(b) FROM data GROUP BY GROUPING SETS ((a), (a), ()); ``` ids work out to [0, 2, 1] for indices [0, 1, 2] (1 grouping column, so the second `(a)`'s ordinal lands in bit 1: `(1<<1)|0 = 2`). Two different indices, same mask - you need the ordinal and the aggregate's actual set list to tell them apart, not just the bits. On the projection: there already is one. `grouping_set_output` does exactly that - `project(plan, exprs)`, swapping `__grouping_id` for `index_from_grouping_id(...)`, a `CASE` built over this aggregate's own `(mask, ordinal)` list. So it's not projection vs. no projection, it's whether that projection's expression has to be this per-aggregate CASE table or if there's a closed form I'm missing - genuinely open to that, but the two examples above are why I don't think mask+ordinal alone gets you there without the list. -- 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]
