SubhamSinghal opened a new pull request, #25470:
URL: https://github.com/apache/datafusion/pull/25470
## Which issue does this PR close?
- Part of #24268.
## Rationale for this change
`Decimal32` and `Decimal64` group-by keys currently force the whole grouping
onto
the row-encoded `GroupValuesRows` fallback.
`group_column_supported_type` is an all-or-nothing gate: if any one column
in a
multi-column key is missing from it, `new_group_values` drops the *entire*
key to
`GroupValuesRows`, so a `(Utf8, Decimal32)` key loses the column-wise path
for
its string column too:
```
(Utf8, Decimal32 ) -> GroupValuesRows
(Utf8, Decimal64 ) -> GroupValuesRows
(Utf8, Decimal128) -> GroupValuesColumn
```
DataFusion has supported these two widths since #17501, and #23849 added
`Decimal256` to the allow-list, but the two narrow widths were never
backfilled.
Both implement `ArrowPrimitiveType`, so they reuse the existing
`PrimitiveGroupValueBuilder` — no new builder is required, exactly like
`Decimal128` and `Decimal256`.
The gap is also internally inconsistent today: `Struct("a": Decimal32)` *is*
already supported, because nested types reach the generic `RowsGroupColumn`
fallback added in #23523. Only the bare top-level type is rejected.
## What changes are included in this PR?
- Support `Decimal32` / `Decimal64` in `group_column_supported_type` and
`make_group_column`.
- `Dictionary(K, Decimal32 | Decimal64)` starts working as a side effect of
the
existing dictionary recursion in both functions — no extra code.
- Extend the `group_column_supported_type_matches_make_group_column`
biconditional test with the two scalar types and the two dictionary-wrapped
variants.
- Add `test_group_values_column_narrow_decimals`, which drives both widths
through one body rather than only the first, asserting `supported_schema`
routing, dedup including nulls, and that precision/scale survive `emit`.
- Add a `bench_narrow_decimals` benchmark mirroring `bench_decimal256`.
## Are these changes tested?
Yes.
- The consistency test plus the new
`test_group_values_column_narrow_decimals`
round-trip test in `multi_group_by/mod.rs`. The round-trip test includes a
value at each type's full width (`999_999_999` and
`999_999_999_999_999_999`) so a truncating storage type would fail it.
- Multi-column `Decimal32` / `Decimal64` GROUP BY with a NULL key in
`group_by.slt`. These widths are not reachable from a SQL `DECIMAL(p, s)`
declaration — that maps to `Decimal128` or `Decimal256` by precision — so
the
keys are built with `arrow_cast`, and each case is paired with an
`arrow_typeof` assertion so it cannot silently degrade into more
`Decimal128`
coverage.
`cargo test -p datafusion-physical-plan --lib` (1921 passed),
`aggregate.slt group_by.slt dictionary.slt decimal.slt`, and
`dev/rust_lint.sh` are all clean.
## Are there any user-facing changes?
No. This only changes which `GroupValues` implementation is selected;
grouping
semantics and output types are unchanged.
--
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]
