You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "tohuya6 (via GitHub)" <gi...@apache.org> on 2026/07/22 03:06:16 UTC
[PR] feat: add GroupColumn support for Float16 in multi-column GROUP BY [datafusion]
tohuya6 opened a new pull request, #23785:
URL: https://github.com/apache/datafusion/pull/23785
## Which issue does this PR close?
- Part of #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` key stays on the
`GroupValuesColumn` path, dedups including nulls, folds `-0.0`/`+0.0` into one
group (stored as `+0.0`), groups equal `NaN`s, 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 `aggregate.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.
--
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 Float16 in multi-column GROUP BY [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #23785:
URL: https://github.com/apache/datafusion/pull/23785#discussion_r3664264762
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1352,14 +1359,65 @@ mod tests {
}
}
+ /// End-to-end coverage for `Float16` group keys. Beyond routing through the
+ /// column-wise `GroupValuesColumn` fast path, this exercises the float
+ /// canonicalization the builder relies on (shared with Float32 / Float64 via
+ /// `HashValue`): `-0.0` and `+0.0` collapse into one group stored as `+0.0`,
+ /// and equal `NaN`s collapse into a single group.
+ #[test]
+ fn test_group_values_column_float16() {
+ use half::f16;
+
+ let schema =
Review Comment:
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.
--
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 Float16 in multi-column GROUP BY [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #23785:
URL: https://github.com/apache/datafusion/pull/23785#issuecomment-5141923435
@tohuya6
Can you resolve the conflicts?
--
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 Float16 in multi-column GROUP BY [datafusion]
Posted by "tohuya6 (via GitHub)" <gi...@apache.org>.
tohuya6 commented on PR #23785:
URL: https://github.com/apache/datafusion/pull/23785#issuecomment-5142877397
@kosiew resolved
--
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 Float16 in multi-column GROUP BY [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23785:
URL: https://github.com/apache/datafusion/pull/23785#issuecomment-5141409588
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-plan v54.1.0 (current)
Built [ 38.640s] (current)
Parsing datafusion-physical-plan v54.1.0 (current)
Parsed [ 0.150s] (current)
Building datafusion-physical-plan v54.1.0 (baseline)
Built [ 37.352s] (baseline)
Parsing datafusion-physical-plan v54.1.0 (baseline)
Parsed [ 0.150s] (baseline)
Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.957s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/inherent_method_missing.ron
Failed in:
ExecutionPlanDecodeCtx::expr_ctx, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/0b0f5244e930748b51242fd8a282cef534e7c632/datafusion/physical-plan/src/proto.rs:352
ExecutionPlanEncodeCtx::expr_ctx, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/0b0f5244e930748b51242fd8a282cef534e7c632/datafusion/physical-plan/src/proto.rs:227
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 79.384s] datafusion-physical-plan
Building datafusion-sqllogictest v54.1.0 (current)
Built [ 197.741s] (current)
Parsing datafusion-sqllogictest v54.1.0 (current)
Parsed [ 0.024s] (current)
Building datafusion-sqllogictest v54.1.0 (baseline)
Built [ 197.474s] (baseline)
Parsing datafusion-sqllogictest v54.1.0 (baseline)
Parsed [ 0.024s] (baseline)
Checking datafusion-sqllogictest v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.112s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 398.858s] datafusion-sqllogictest
```
</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 Float16 in multi-column GROUP BY [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23785:
URL: https://github.com/apache/datafusion/pull/23785#issuecomment-5141574048
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23785?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.85%. Comparing base ([`5de7f1d`](https://app.codecov.io/gh/apache/datafusion/commit/5de7f1db95191f81ce6472361785db8f63ac2db1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4683424`](https://app.codecov.io/gh/apache/datafusion/commit/468342468b2518b6fe463720472b637281612be6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 130 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #23785 +/- ##
==========================================
+ Coverage 80.71% 80.85% +0.13%
==========================================
Files 1089 1099 +10
Lines 368760 374355 +5595
Branches 368760 374355 +5595
==========================================
+ Hits 297647 302686 +5039
- Misses 53372 53610 +238
- Partials 17741 18059 +318
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23785?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 Float16 in multi-column GROUP BY [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #23785:
URL: https://github.com/apache/datafusion/pull/23785
--
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 Float16 in multi-column GROUP BY [datafusion]
Posted by "tohuya6 (via GitHub)" <gi...@apache.org>.
tohuya6 commented on code in PR #23785:
URL: https://github.com/apache/datafusion/pull/23785#discussion_r3683647324
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1352,14 +1359,65 @@ mod tests {
}
}
+ /// End-to-end coverage for `Float16` group keys. Beyond routing through the
+ /// column-wise `GroupValuesColumn` fast path, this exercises the float
+ /// canonicalization the builder relies on (shared with Float32 / Float64 via
+ /// `HashValue`): `-0.0` and `+0.0` collapse into one group stored as `+0.0`,
+ /// and equal `NaN`s collapse into a single group.
+ #[test]
+ fn test_group_values_column_float16() {
+ use half::f16;
+
+ let schema =
Review Comment:
I appreciate the advice, tried to make the distinction clearer
--
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