You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "SubhamSinghal (via GitHub)" <gi...@apache.org> on 2026/09/18 08:01:55 UTC
[PR] feat: add GroupColumn support for Decimal32/Decimal64 [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add GroupColumn support for Decimal32/Decimal64 [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25470:
URL: https://github.com/apache/datafusion/pull/25470#issuecomment-5727382191
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25470?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `98.50746%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.35%. Comparing base ([`0e292dc`](https://app.codecov.io/gh/apache/datafusion/commit/0e292dcbfdabc41a1297c57df9a1e0368b53e64d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`fab3521`](https://app.codecov.io/gh/apache/datafusion/commit/fab352159611aee2db103f27cb373443bb5887d3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25470?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../src/aggregates/group\_values/multi\_group\_by/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/25470?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fgroup_values%2Fmulti_group_by%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL2dyb3VwX3ZhbHVlcy9tdWx0aV9ncm91cF9ieS9tb2QucnM=) | 98.50% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25470?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25470 +/- ##
=======================================
Coverage 82.35% 82.35%
=======================================
Files 1137 1137
Lines 432746 432813 +67
Branches 432746 432813 +67
=======================================
+ Hits 356375 356433 +58
- Misses 54843 54850 +7
- Partials 21528 21530 +2
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25470?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add GroupColumn support for Decimal32/Decimal64 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25470:
URL: https://github.com/apache/datafusion/pull/25470#issuecomment-5783997798
Thanks @SubhamSinghal and @martin-g -- i merged up to resolve a conflict to put it in the merge queue
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add GroupColumn support for Decimal32/Decimal64 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25470:
URL: https://github.com/apache/datafusion/pull/25470
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org