feat: add GroupColumn support for Float16 in multi-column GROUP BY by tohuya6 · Pull Request #23785 · apache/datafusion

tohuya6 · 2026-07-22T03:06:14Z

Which issue does this PR close?

Part of EPIC: complete GroupValuesColumn type coverage (nested types + remaining primitives) #22715.

Rationale for this change

multi_group_by::group_column_supported_type gates which GROUP BY columns may
use the column-wise GroupValuesColumn fast path, and the gate is
all-or-nothing: a single unsupported column forces the entire grouping onto
the byte-encoded GroupValuesRows fallback, even when every other key column
would have qualified. A Float16 key triggers exactly that today.

Float16 reuses the existing PrimitiveGroupValueBuilder with no new builder
type: its native half::f16 already implements the HashValue
canonicalization (hash_float!(f16, f32, f64)), so -0.0/+0.0 folding and
NaN grouping match Float32/Float64 with no extra handling.

What changes are included in this PR?

Accept Float16 in group_column_supported_type and dispatch it in
make_group_column.
Move Float16 from the rejected to the accepted set in the
group_column_supported_type ↔ make_group_column consistency fuzz, and
repoint the two tests that used Float16 as their stock "unsupported" example
to a permanently-invalid unit combo (Time64(Second)), so they stay stable as
sibling primitive builders (Decimal256, Interval, …) land independently.
Add a (Float16, Int32) group-count benchmark to benches/multi_group_by.rs
(capped below f16's ~63.5k distinct finite values).

Are these changes tested?

Yes.

New unit test test_group_values_column_float16: a (Float16, Int32) key
stays on the GroupValuesColumn path, dedups including nulls, folds
-0.0/+0.0 into one group (stored as +0.0), groups equal NaNs, keeps
(0.0, 4) distinct from (±0.0, 3) via the Int32 key, and round-trips
with the Float16 output type preserved.
The consistency fuzz now asserts Float16 routes through the dispatcher.
New single- and multi-column Float16 GROUP BY coverage in group_by.slt.

Are there any user-facing changes?

No API changes. GROUP BY queries with a Float16 key now use the column-wise
fast path instead of the row-encoded fallback; results are unchanged.

…umn GROUP BY `multi_group_by::group_column_supported_type` gates which GROUP BY columns can use the column-wise `GroupValuesColumn` fast path. Any unsupported column forces the entire grouping onto the byte-encoded `GroupValuesRows` fallback, so a single `Float16` key dragged an otherwise-qualifying multi-column GROUP BY onto the slow path. `Float16` reuses the existing `PrimitiveGroupValueBuilder` with no new builder type: its native `half::f16` already implements the `HashValue` canonicalization (`hash_float!(f16, f32, f64)`), so `-0.0` / `+0.0` folding and NaN grouping match Float32 / Float64 with no extra handling. - accept `Float16` in `group_column_supported_type` and dispatch it in `make_group_column` - move `Float16` from the rejected to the accepted set in the `group_column_supported_type` <-> `make_group_column` consistency fuzz, and repoint the two tests that used `Float16` as their stock "unsupported" example to a permanently-invalid unit combo (`Time64(Second)`), so they stay stable as sibling primitive builders (Decimal256, Interval, ...) land - add an end-to-end unit test (Float16 GROUP BY dedups including nulls, folds -0.0/+0.0 into one group stored as +0.0, groups equal NaNs, and preserves the Float16 output type) and a Float16 GROUP BY block in aggregate.slt - add a `(Float16, Int32)` group-count benchmark to `benches/multi_group_by.rs` (capped below f16's ~63.5k distinct finite values) Part of apache#22715

kosiew

@tohuya6
Nice improvement.
I just have one small suggestion that could make the regression test a bit more targeted.

kosiew · 2026-07-28T09:13:23Z

+    fn test_group_values_column_float16() {
+        use half::f16;
+
+        let schema =


Nice test! One small suggestion: since the user-visible regression is the all-or-nothing multi-column gate, it might be worth making this schema (Float16, Int32) instead of a single Float16 column.

That would let the test assert that (-0.0, 3) and (+0.0, 3) resolve to the same group, while values like (0.0, 4) remain distinct because the Int32 key differs. The SLT already verifies the SQL behaviour, but this would exercise the exact multi-column GroupValuesColumn path and make the gate and factory integration a little more explicit.

I appreciate the advice, tried to make the distinction clearer

… unit test Per kosiew's review: a single-column Float16 test doesn't exercise the all-or-nothing multi-column gate this PR is actually about. Switching to (Float16, Int32) does — group count goes from 4 (single column: 1.0, ±0.0, NaN, null) to 5, since (0.0, 4) now splits off from (±0.0, 3) on the Int32 key even though the Float16 values are equal. Also trimmed the slt/test comments to match the Duration and Interval siblings.

…he GroupColumn unit test Per kosiew's review: a single-column Duration test doesn't exercise the all-or-nothing multi-column gate this PR is actually about. Switching to (Duration(Microsecond), Int64) does — rows 3 and 4 now repeat row 0 and the (null, null) pair across both columns, so dedup is proven on the composite key rather than the Duration value alone. Also renumber the bench to Experiment 9 (float16 apache#23785 takes 8) and trim the slt/test comments to match the sibling PRs.

The 30-days case duplicated the slt, which already proves 1 month and 30 days don't fold. Dropping it leaves the assertions unique to the unit level: null keys dedup, the Int32 key splits equal intervals, and emit gives back Interval rather than the raw native. Also renumber the bench to Experiment 10 (float16 apache#23785 takes 8, duration apache#23783 takes 9), trim comments to match the sibling PRs, and normalize the blank lines after the new slt block to two, matching upstream.

codecov-commenter · 2026-07-31T09:47:42Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.85%. Comparing base (aa9b7bb) to head (9ffada0).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #23785      +/-   ##
==========================================
- Coverage   80.86%   80.85%   -0.01%     
==========================================
  Files        1099     1101       +2     
  Lines      374766   374984     +218     
  Branches   374766   374984     +218     
==========================================
+ Hits       303058   303209     +151     
- Misses      53617    53676      +59     
- Partials    18091    18099       +8

☔ 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.

kosiew · 2026-07-31T10:31:18Z

@tohuya6
Can you resolve the conflicts?

Duration apache#23783 landed on main and touched the same regions as this PR. Kept both sides: - mod.rs: unioned the arrow imports, kept both unit tests, Float16 added to the supported arms - benches: Float16 stays Experiment 8, Duration keeps 9 - group_by.slt: both coverage blocks

tohuya6 · 2026-07-31T12:30:44Z

@kosiew resolved

github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Jul 22, 2026

tohuya6 mentioned this pull request Jul 23, 2026

EPIC: complete GroupValuesColumn type coverage (nested types + remaining primitives) #22715

Open

7 tasks

kosiew approved these changes Jul 28, 2026

View reviewed changes

test: move Float16 GROUP BY coverage to group_by.slt

4683424

tohuya6 requested a review from kosiew July 31, 2026 00:08

github-actions Bot added the auto detected api change Auto detected API change label Jul 31, 2026

github-actions Bot removed the auto detected api change Auto detected API change label Jul 31, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add GroupColumn support for Float16 in multi-column GROUP BY - #23785

feat: add GroupColumn support for Float16 in multi-column GROUP BY#23785
tohuya6 wants to merge 4 commits into
apache:mainfrom
tohuya6:feat-22715-float16-group-column

tohuya6 commented Jul 22, 2026 •

edited

Loading

Uh oh!

kosiew left a comment •

edited

Loading

Uh oh!

kosiew Jul 28, 2026

Uh oh!

tohuya6 Jul 30, 2026

Uh oh!

codecov-commenter commented Jul 31, 2026 •

edited

Loading

Uh oh!

kosiew commented Jul 31, 2026

Uh oh!

tohuya6 commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tohuya6 commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

kosiew left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

tohuya6 Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kosiew commented Jul 31, 2026

Uh oh!

tohuya6 commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tohuya6 commented Jul 22, 2026 •

edited

Loading

kosiew left a comment •

edited

Loading

codecov-commenter commented Jul 31, 2026 •

edited

Loading