namanjain24-sudo commented on PR #25527:
URL: https://github.com/apache/datafusion/pull/25527#issuecomment-6344403608
@kosiew both addressed.
**Test isolation** — you were right, and it was worse than a naming nit: the
eight `a + <offset>` expressions plus the trailing bare `a` are nine distinct
grouping expressions, so `UInt16` was already forced by column count and the
duplicate ordinal proved nothing. Switched to bare `a` plus `a + 1`..`a + 7` so
the trailing `(a)` reuses one of the eight. Measured on the branch:
- eight columns, no repeat → `__grouping_id` is `UInt8`
- same eight with the set repeated → `UInt16`
Both are asserted now, so the widening is pinned to the ordinal rather than
the column count.
**Consumer coverage** — added
`consume_grouping_sets_63_columns_repeated_thrice_is_rejected` and
`consume_grouping_sets_64_columns_repeated_is_rejected`. Each produces a
representable plan, edits the `AggregateRel` groupings in the proto into the
shape the producer refuses to emit but another producer could send, then
asserts the consumer's specific error ("A grouping set with 63 columns cannot
be repeated ... does not fit in the 1 bit(s) left above the mask", and the
64-column / 0-bit variant). Since `to_substrait_plan` succeeds before the edit,
the error provably comes from the consumer path.
Tests only, no production changes.
@alamb — this adds ~145 lines of tests, which cuts against your point that
total diff size is the right metric, so happy to drop either of these if you'd
rather keep it tight. The open question is still yours though: did the examples
I posted above answer why the index can't come out of `__grouping_id` by
formula? I'd rather settle that direction before putting anything more into
this PR.
--
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]
