Repository navigation
refactor: Centralize float normalization - #26595
Merged
Merged
Conversation
github-actions
Bot
added
physical-expr
mkleen
changed the title
mkleen
force-pushed
the
centralize-float-canonicalization
branch
from
October 22, 2026 18:34
f5d50d6 to
81591a9
Compare
|
Thank you for opening this pull request! Reviewer note: 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 |
mkleen
force-pushed
the
centralize-float-canonicalization
branch
3 times, most recently
from
October 23, 2026 06:58
0b01d91 to
6cdd45d
Compare
mkleen
changed the title
mkleen
force-pushed
the
centralize-float-canonicalization
branch
from
October 23, 2026 07:58
e021e20 to
c25cabe
Compare
…oat_semantics` Add a `float_semantics` module with a `CanonicalFloat` trait (f16/f32/f64) providing `canonicalize`, `canonical_bits`, `compare_floats`, `float_lt` and `float_gt`, plus `canonicalize_float_array`/`canonicalize_float_scalar`, and route all of those sites through it. No behavior change: `compare_floats` is still `total_cmp`. `normalize_float_zero` and `normalize_float_zero_scalar` are deprecated in favor of the new functions.
mkleen
force-pushed
the
centralize-float-canonicalization
branch
from
October 23, 2026 07:59
c25cabe to
a2b14eb
Compare
mkleen
marked this pull request as ready for review
October 23, 2026 08:10
Contributor
Author
mkleen
changed the title
mkleen
force-pushed
the
centralize-float-canonicalization
branch
from
October 23, 2026 08:24
b807c90 to
f1027d7
Compare
Contributor
Author
|
Btw, I have also some performance improvements in the pipeline, but i wanted to keep the scope of this pr small. |
jayzhan211
approved these changes
Oct 23, 2026
jayzhan211
left a comment
jayzhan211
left a comment
Contributor
There was a problem hiding this comment.
@mkleen I compared each call site with the code it replaces, and they match bit for bit. Only -0.0 is folded, NaN payloads are kept, and compare_floats is still total_cmp, so min/max, TopK and range repartitioning order floats exactly as before. All of the #22835 call sites are covered. Clippy (also with force_hash_collisions), the unit tests of the touched crates, and the negative-zero, group-by, set-function and PWMJ sqllogictests all pass locally.
Since this touches hashing and grouping, I also ran a quick A/B against the merge base on an M4 Pro. Everything is within noise:
| before | this PR | |
|---|---|---|
| hash 8192 f64 values | 2.44 µs | 2.51 µs |
| hash 8192 f32 values | 2.46 µs | 2.49 µs |
| hash 8192 f16 values | 2.54 µs | 2.48 µs |
normalize f64, no -0.0 |
2.12 µs | 2.14 µs |
normalize f64, with -0.0 |
1.29 µs | 1.29 µs |
multi_group_by float16, vectorized, 60k groups |
7.89 ms | 7.95 ms |
multi_group_by float16, row-based, 60k groups |
7.56 ms | 7.61 ms |
For the perf work you mentioned (not something for this PR): when there is nothing to normalize, the pre-scan in normalize_primitive is the expensive part. any() exits early, so it doesn't vectorize; Comet ran into the same thing in apache/datafusion-comet#6590. In a standalone test, a reduction that doesn't short-circuit took the check on a clean 8192-value f64 array from about 2.1 µs to 0.5 µs:
- if !arr.values().iter().any(|v| v.needs_normalization()) {
+ if !arr
+ .values()
+ .iter()
+ .fold(false, |acc, v| acc | v.needs_normalization())
+ {I left one small doc suggestion inline.
Contributor
Author
|
@jayzhan211 wow, thanks for the fast review and the great feedback! |
alamb
approved these changes
Oct 24, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
I agree this looks like a nice improvement -- thank you @mkleen and @jayzhan211
| /// Only arrays that actually contain `-0.0` pay for a new buffer. | ||
| #[deprecated( | ||
| since = "56.0.0", | ||
| note = "use `datafusion_common::float_semantics::normalize_floats` instead" |
|
|
||
| /// Applies [`normalize_float`] to a `Float16`, `Float32` or `Float64` scalar. | ||
| /// Other scalars are returned as is. | ||
| pub fn normalize_float_scalar(scalar: ScalarValue) -> ScalarValue { |
Contributor
There was a problem hiding this comment.
FWIW this might be easier to find if it belongs on a method of ScalrValue -- ScalarValue::normalize() or something 🤔
Contributor
Author
There was a problem hiding this comment.
You want this in this pr or as a follow-up ?
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing centralize-float-canonicalization (87e59ea) to 2717e25 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing centralize-float-canonicalization (87e59ea) to 2717e25 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing centralize-float-canonicalization (87e59ea) to 2717e25 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing centralize-float-canonicalization (87e59ea) to 2717e25 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing centralize-float-canonicalization (87e59ea) to 2717e25 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing centralize-float-canonicalization (87e59ea) to 2717e25 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing centralize-float-canonicalization (87e59ea) to 2717e25 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing centralize-float-canonicalization (87e59ea) to 2717e25 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
|
no regressions, only noise. |
Contributor
I agree -- thanks @mkleen |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
-0.0(and NaN) float keys in one place instead of per call site #26138 bringing the existing0.0/-0.0float handling into one place.Rationale for this change
DataFusion treats
-0.0and0.0as the same number #22835. The code that makes this work is spread across multiple places. This PR moves the existing code into one module without changing its semantics.The new module
DataFusion Comet has a similar module,
native/spark-expr/src/float_semantics, which puts Spark's-0.0/NaN rules in one place. This PR follows its naming. The public API is a set of free functions:Comet's functions are generic over
num::Float. DataFusion also has to coverf16, andhalfis built without itsnum-traitsfeature, so a small trait,FloatSemantics, serves as the bound. It is implemented forf16,f32andf64:There are two major differences from Comet.
normalize_floatsalso reaches floats inside nested types, where Comet has a separatenormalize_nested_floats. And normalization here only folds-0.0; NaN canonicalization and SQL ordering are not part of this PR. Once those are added, Comet's layout (normalize,compareandkernelssubmodules) is a good template for growing the module.What changes are included in this PR?
A new module,
datafusion_common::float_semantics, now decides when two floats count as the same value and how floats are ordered. All the following parts now use this module:What is the testing strategy for this PR?
Are there any user-facing changes?
datafusion_common::float_semanticswith the functions above and theFloatSemanticstrait.normalize_float_zeroandnormalize_float_zero_scalarindatafusion_common::utilsare deprecated in favor ofnormalize_floatsandnormalize_float_scalar.