Repository navigation
IN LIST: treat signed zeros as equal - #25186
Merged
Merged
Conversation
github-actions
Bot
added
physical-expr
geoffreyclaude
force-pushed
the
codex/in-list-signed-zero
branch
5 times, most recently
from
September 11, 2026 13:58
12098fc to
474474f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25186 +/- ##
==========================================
- Coverage 82.60% 82.58% -0.02%
==========================================
Files 1144 1144
Lines 442793 443398 +605
Branches 442793 443398 +605
==========================================
+ Hits 365779 366191 +412
- Misses 54863 54991 +128
- Partials 22151 22216 +65 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
geoffreyclaude
force-pushed
the
codex/in-list-signed-zero
branch
3 times, most recently
from
September 11, 2026 15:02
aa5a037 to
eaee11e
Compare
geoffreyclaude
marked this pull request as ready for review
September 14, 2026 11:39
geoffreyclaude
force-pushed
the
codex/in-list-signed-zero
branch
from
September 14, 2026 11:40
eaee11e to
8713a66
Compare
Contributor
Author
|
run benchmark in_list_strategy |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing codex/in-list-signed-zero (8713a66) to 7b00b63 (merge-base) diff Run configurationrun benchmark in_list_strategyResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing codex/in-list-signed-zero (8713a66) to 7b00b63 (merge-base) diff Run configurationrun benchmark in_list_strategyCPU Details (lscpu)Details
Resource Usagein_list_strategy — base (merge-base)
in_list_strategy — branch
File an issue against this benchmark runner |
This was referenced Sep 16, 2026
geoffreyclaude
force-pushed
the
codex/in-list-signed-zero
branch
8 times, most recently
from
September 22, 2026 16:36
58f9412 to
6bd52aa
Compare
neilconway
reviewed
Sep 29, 2026
neilconway
left a comment
neilconway
left a comment
Contributor
There was a problem hiding this comment.
Thanks @geoffreyclaude. I think this is heading in the right direction but it seems there is a lot more work we need to do if we want zeros to be handled correctly throughout DF. We don't need to take all of that work on in this PR but hopefully we can find a self-consistent subset...
Comment on lines
1569
to
1573
| ScalarValue::Dictionary(key, mut value) => { | ||
| *value = normalize_float_zero_scalar(*value); | ||
| ScalarValue::Dictionary(key, value) | ||
| } | ||
| other => other, |
Contributor
There was a problem hiding this comment.
Shouldn't we handle other nested types, like REEs, unions, lists, ...?
Contributor
Author
There was a problem hiding this comment.
I'll see if I can squeeze those in without blowing up the PR. Otherwise I'd prefer keeping the broader fixes in a new PR or series of PRs.
Contributor
Author
There was a problem hiding this comment.
Added in 6ea876b: all nested scalar types now normalize recursively, including unions and REEs. The array-backed variants reuse the existing array normalizer.
| fn normalize_in_list_float_zero_value(value: ColumnarValue) -> ColumnarValue { | ||
| match value { | ||
| ColumnarValue::Array(array) | ||
| if dictionary_value_type(array.data_type()).is_floating() => |
Contributor
There was a problem hiding this comment.
is_floating seems like too shallow of a check for nested types. What if we just did
ColumnarValue::Array(array) => ColumnarValue::Array(normalize_float_zero(&array)),
Contributor
Author
There was a problem hiding this comment.
Agreed, I've used your suggested array arm in 6ea876b.
12 tasks done
Add the missing signed-zero encoding once in the shared static-filter factory after dictionary flattening. Select the filter using the expanded physical non-null list length. Remove per-filter zero insertion and extra-length branchless kernels. Cover both signs across cutoffs and preserve nulls, NaN payloads and slices.
Normalize dynamic and generic IN operands through normalize_floats and recursive ScalarValue::normalize. Keep primitive static filters on constructor-time signed-zero expansion and use the shared negative-zero predicate before selecting their lookup strategy. Move normalization regressions to the shared module and cover nested scalar self-membership and encoded scalar needles.
geoffreyclaude
force-pushed
the
codex/in-list-signed-zero
branch
from
November 7, 2026 06:43
a17cf88 to
4a60604
Compare
Contributor
Author
|
run benchmark in_list |
1 similar comment
Contributor
Author
|
run benchmark in_list |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing codex/in-list-signed-zero (4a60604) to 4bf32cb (merge-base) diff Run configurationrun benchmark in_listResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing codex/in-list-signed-zero (4a60604) to 4bf32cb (merge-base) diff Run configurationrun benchmark in_listResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing codex/in-list-signed-zero (4a60604) to 4bf32cb (merge-base) diff Run configurationrun benchmark in_listCPU Details (lscpu)Details
Resource Usagein_list — base (merge-base)
in_list — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing codex/in-list-signed-zero (4a60604) to 4bf32cb (merge-base) diff Run configurationrun benchmark in_listCPU Details (lscpu)Details
Resource Usagein_list — base (merge-base)
in_list — branch
File an issue against this benchmark runner |
geoffreyclaude
marked this pull request as ready for review
November 7, 2026 07:35
alamb
approved these changes
Nov 7, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thanks @geoffreyclaude -- this looks good to me.
It is also nice that it doesn't suffer the slowdown of my attempt. I merged this up from main and added a small slt test update. I think it is ready to go
| pub fn normalize(mut self) -> Self { | ||
| use crate::float_semantics::{normalize_float, normalize_floats}; | ||
|
|
||
| fn normalize_array<A: Array + From<ArrayData> + 'static>(array: &mut Arc<A>) { |
Contributor
There was a problem hiding this comment.
I found this overly complicated -- I think it is working around the fact you can't pass an owned array to normalize_floats. Here is a proposed follow on PR to fix it
Contributor
Author
There was a problem hiding this comment.
That does seem cleaner 😅
| } | ||
|
|
||
| /// SQL membership equates the zero signs, while the primitive filters compare bits. | ||
| fn add_missing_signed_zero<T: ArrowPrimitiveType>(in_array: ArrayRef) -> Result<ArrayRef> |
| } | ||
| } | ||
|
|
||
| fn normalize_in_list_value(value: ColumnarValue) -> ColumnarValue { |
Contributor
There was a problem hiding this comment.
this I also think should be a method on ColumnarValue. I will try and refactor it
alamb
enabled auto-merge
November 7, 2026 10:49
alamb
added a commit
to alamb/datafusion
that referenced
this pull request
Nov 7, 2026
…ize` in `InListExpr` Rebased on apache#25186. Its `normalize_in_list_value` helper, `ArrayStaticFilter` and `ScalarValue::normalize` now call the owned `float_semantics::normalize` and `ColumnarValue::normalize`. Drop the upgrade guide entry. Co-Authored-By: Claude Fable 5.1 <[email protected]>
This was referenced Nov 7, 2026
Contributor
|
Here is my proposal for cleaning up the floating point code: |
xudong963
pushed a commit
that referenced
this pull request
Nov 9, 2026
…27204) ## Which issue does this PR close? - part of #24461 - Backport of #25186 to `branch-56` - Related to #27121. Co-authored-by: Geoffrey Claude <[email protected]>
robertmu
added a commit
to lagodb/lagodb
that referenced
this pull request
Nov 10, 2026
Push float comparisons to Iceberg as exact predicates. Arrow and Iceberg order floats by IEEE totalOrder, while PostgreSQL treats -0 and 0 as equal and orders every NaN above every number. The new `predicate::float` rewrites each comparison so it accepts exactly the rows PostgreSQL accepts: - A zero constant gets the sign that puts both zeros on the correct side. - `>` and `>=` also accept `isnan`, and `<` and `<=` reject it. - A double value compared with a real column is rounded to the neighbouring real on the side each operator keeps. The planner costs `>` and `>=` as the OR with `isnan` that they bind to. Query offload now accepts float comparisons, GROUP BY keys and join keys. It no longer rejects float join keys. DataFusion folds -0 into +0 and keeps NaN payloads apart, which is an accepted engine difference. Float ORDER BY stays in PostgreSQL. Comments now record the upstream defects that are not worked around here: - DataFusion 55's float IN-list, and the IN-list dynamic filter built from a small hash-join build side, compare float bits and miss -0 rows (apache/datafusion#25186). - The sort-merge semi/anti/mark fallback panics on a NaN key at a batch boundary (apache/datafusion#25134). - Iceberg filters on files written before a type promotion round the literal down to the old type (apache/iceberg-rust#3122, #3540). SUM(int8) now casts its input to Decimal128(38,0) before DataFusion's native SUM, because the native Int64 SUM wraps on overflow. The sum stays exact and can be compared in HAVING. Remove the now-unused `NumericFromInt64` output codec.
limadog9
pushed a commit
to limadog9/datafusion
that referenced
this pull request
Nov 11, 2026
…apache#27202) ## Which issue does this PR close? - Related tof apache#27121 - Split out of apache#27181 ## Rationale for this change DataFusion normalizes `-0.0` to `+0.0` via `float_semantics::normalize_floats(&ArrayRef)`. @mkleen centralized the float normalization in - apache#26595 Thanks to @jayzhan211 for responding However, because it borrows its argument, any array that contains `-0.0` is copied into a new buffer, even if there are no other users. Also it forces a bunch of unecessary (Arc) clones; It would be nice to reuse the allocation if possible ## What changes are included in this PR? Add `float_semantics::normalize` that takes an owned array and reuses the allocation if possible deprecaate the old dversion ## What is the testing strategy for this PR? Added some new tests ## Are there any user-facing changes? * `float_semantics::normalize_floats` is deprecated in favor of `float_semantics::normalize`, * `ColumnarValue::normalize` is new. ## Follow ons - I think we can clean up a non trivial amount of apache#25186 with this function, specifically: https://github.com/apache/datafusion/pull/25186/changes#r4501319899 --------- Co-authored-by: Claude Fable 5.1 <[email protected]> Co-authored-by: Jay Zhan <[email protected]>
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 (shared floating-point semantics).Rationale for this change
DataFusion equality treats
+0.0and-0.0as equal, but the optimizedINfilters compare or hash floating-point bit patterns. Because the two zeros have different encodings, anINpredicate could therefore return a different result depending on whether the optimizer retained it or rewrote it into equality comparisons.This signed-zero limitation predates the IN-list optimization epic #19241: #7378 already used bit-pattern equality for floating-point membership in August 2023. #22835 corrected ordinary comparisons in June 2026 but did not fix
INmembership. This PR addresses that remaining correctness gap.This PR makes
Float16,Float32, andFloat64INpredicates, including floats inside nested and encoded values, follow the same signed-zero equality as ordinary comparisons. The shared static-filter factory adds the missing zero encoding before choosing a primitive filter. Strategy selection uses the expanded physical non-null list length, so both encodings count toward the existing cutoffs. Generic static filters and non-static lists use the sharednormalize_floatsand recursiveScalarValue::normalizeAPIs before comparison. Distinct NaN payloads remain distinct, preserving the existing bit-equality behavior for NaNs.What changes are included in this PR?
ScalarValue::normalizeto nested and encoded values, and usefloat_semantics::normalize_floatsfor arrays in dynamic comparisons and generic static filters.IN, andNOT IN.What is the testing strategy for this PR?
SQL regressions in
negative_zero.sltcover both signed-zero directions, static filters, dynamic scalar and array operands, nested values, IN-list intersection/difference,IN,NOT IN, and nulls. Focused Rust tests cover dictionary inputs, distinct NaN payloads, encoded scalar metadata, and sliced values. Factory tests exercise both zero signs immediately below and at each floating-point branchless cutoff, including dispatch after adding the missing physical key. SQL tests also check constant nested scalar self-membership; an existing Rust test covers nested dictionary scalar inputs.Validation after rebasing onto
upstream/main(4bf32cb46d): formatting, CI Clippy with its standard feature set, all 111 focused Rust tests, and all 546 sqllogictest files (including Parquet encryption) passed.Are there any user-facing changes?
Yes. Floating-point
INandNOT IN, including nested and encoded values, now treat+0.0and-0.0as equal, consistently with ordinary DataFusion equality.ScalarValue::normalizenow applies these rules recursively; public API signatures are unchanged.Prepared with AI assistance.