Skip to content

feat: add GroupColumn support for Decimal32/Decimal64 - #25470

Merged
alamb merged 2 commits into
apache:mainfrom
SubhamSinghal:group-column-decimal32-64
Sep 22, 2026
Merged

alamb merged 2 commits into
apache:mainfrom
SubhamSinghal:group-column-decimal32-64

Conversation

@SubhamSinghal

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Sep 18, 2026
@codecov-commenter

codecov-commenter commented Sep 18, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.43%. Comparing base (5de9302) to head (993ea78).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
.../src/aggregates/group_values/multi_group_by/mod.rs 98.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #25470   +/-   ##
=======================================
  Coverage   82.43%   82.43%           
=======================================
  Files        1140     1140           
  Lines      435862   435929   +67     
  Branches   435862   435929   +67     
=======================================
+ Hits       359312   359370   +58     
- Misses      54832    54840    +8     
- Partials    21718    21719    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alamb

alamb commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Thanks @SubhamSinghal and @martin-g -- i merged up to resolve a conflict to put it in the merge queue

@alamb
alamb enabled auto-merge September 22, 2026 20:54
@alamb
alamb added this pull request to the merge queue Sep 22, 2026
Merged via the queue into apache:main with commit d5216e6 Sep 22, 2026
41 checks passed
@SubhamSinghal
SubhamSinghal deleted the group-column-decimal32-64 branch September 23, 2026 03:12
Omega359 pushed a commit to Omega359/arrow-datafusion that referenced this pull request Oct 11, 2026
## Which issue does this PR close?

- Part of apache#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 apache#17501, and apache#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 apache#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.

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt) v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants