Uh oh!
There was an error while loading. Please reload this page.
fix: make float filters treat -0.0 and 0.0 as the same value - #6236
Conversation
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
ReviewClean, well-structured fix for the IEEE 754 -0.0 vs +0.0 inconsistency. The multi-layer approach (ArrowScalar row encoding, OrderableScalarValue Ord, query expression literals, coercion) is the right strategy — normalizing at each boundary rather than trying a single choke-point. Minor observations (not blocking)Unnecessary allocations for +0.0: The DRY opportunity: Overall: good fix, good tests, good PR description. LGTM. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
LuciferYang
commented
Mar 20, 2026
There are test failures, and I need to investigate them. |
LuciferYang
commented
Mar 20, 2026
The test failure is due to inconsistent behaviors between Rust Arrow/DataFusion and PyArrow. Rust Arrow 57 ( |
| Operation | IEEE 754 | total_cmp (Rust Arrow 57 / DataFusion 52) | PyArrow (C++ Arrow) |
|---|---|---|---|
-0.0 < 0.0 | false | true | false |
-0.0 == 0.0 | true | false | true |
-0.0 <= 0.0 | true | true | true |
Conflict
This pr normalizes -0.0 to +0.0 in multiple locations to implement IEEE 754 semantics:
ArrowScalarrow encoding (Eq/Ord/Hash consistency)OrderableScalarValue::Ord(btree index comparisons)SargableQuery::to_expr/BloomFilterQuery::to_expr(query expression literals)safe_coerce_scalar(float coercion)- Bitmap index training (HashMap key normalization)
This makes Lance's index results inconsistent with DataFusion/Arrow scan results:
- Lance bitmap index (
value < 0.0): excludes-0.0(IEEE 754) - DataFusion scan (
value < 0.0): includes-0.0(total_cmp)
The integration test test_query_float_special_values catches this inconsistency because it validates Lance scanner output against DataFusion SQL output.
Recommended Approach: Option B + C
After analysis, neither Option B (index/query normalization) nor Option C (storage normalization) alone is sufficient. The recommended approach combines both:
Option B: Index/query layer normalization (current commit)
Normalize -0.0 to +0.0 in index building, query expression conversion, and scalar comparisons. This is the current implementation on branch fix-5868.
Handles: Historical data that already has -0.0 stored on disk. When these values flow through indexes and queries, they are normalized at the comparison/expression boundary.
Option C: Storage/ingestion layer normalization (additional work needed)
Normalize -0.0 to +0.0 when writing data to Lance files at the writer/encoding layer.
Handles: New data. Eliminates -0.0 at the source so all downstream code (indexes, scans, DataFusion filters) sees only +0.0. This removes the total_cmp vs IEEE 754 divergence entirely for new writes, since +0.0 < 0.0 is false under both semantics.
Why both are needed
| Scenario | Option B alone | Option C alone | B + C |
|---|---|---|---|
| New data, index query | Correct | Correct (no -0.0 to compare) | Correct |
| New data, DataFusion scan filter | Diverges (index: IEEE 754, scan: total_cmp) | Correct (no -0.0 to compare) | Correct |
| Historical data, index query | Correct | Wrong (-0.0 still on disk) | Correct |
| Historical data, DataFusion scan filter | Diverges | Wrong | Diverges (unavoidable without rewrite) |
- Option B alone fails for new data: Lance index results (IEEE 754) will differ from DataFusion scan results (
total_cmp) when the data contains-0.0. - Option C alone fails for historical data: existing Lance files still contain
-0.0on disk. Indexes built from historical data will encounter-0.0values. - B + C together are fully correct for new data. For historical data, index queries are correct (Option B normalizes at comparison time), but a raw DataFusion scan filter on un-rewritten data may still include
-0.0in< 0.0results due tototal_cmp. This edge case is unavoidable without rewriting the data files, and is acceptable since:- It only affects legacy data written before the fix
- Users can resolve it by rewriting/compacting their datasets
- The index path (which is the primary query path) gives correct results
Integration test update
The integration test test_query_float_special_values compares Lance scanner output against DataFusion SQL on the original in-memory batch. With Option C, the stored data no longer contains -0.0, so the test baseline must be constructed from data read back from the Lance dataset (post-normalization) rather than the pre-write in-memory batch. This is a test-only change.
Other Options (not recommended)
Option A: Match DataFusion/Arrow (total_cmp semantics)
Revert -0.0 normalization from OrderableScalarValue::Ord, bitmap training, and SargableQuery::to_expr. Keep only the ArrowScalar Eq/Hash normalization (which has its own contract independent of DataFusion).
Pros:
- Lance index results match full-scan results (both use
total_cmp) - Consistent with the Rust Arrow/DataFusion ecosystem
- Integration tests pass as-is
Cons:
-0.0treated differently from+0.0everywhere (violates mathematical expectation)- Diverges from PyArrow/C++ Arrow behavior
- Issue incorrect handling of -0.0 in comparison #5868 remains unfixed
Dear project maintainers, what are your thoughts on this?
Xuanwo
left a comment
There was a problem hiding this comment.
- Full-scan and flat BTree paths still evaluate comparisons against original float values, while only the literal side is normalized. Signed-zero queries can still return different results across scan and index paths.
- Bitmap indexes can keep the two signed zeros as separate training keys but load them through a comparator that treats them as equal. Exact bitmap queries can lose one side of the zero rows.
- Bloom pruning still hashes raw float bytes before any normalized expression can recheck results. A zone containing one signed-zero encoding can be pruned for a query using the other encoding.
Conflicts were all additive: arrow-scalar import list (upstream added AsArray/FloatXXType for ArrowScalar::is_nan), SargableQuery::to_expr (upstream added the LikePrefix arm next to Equals), and adjacent test blocks in btree.rs.
Measured on a 4-value dataset [-1.0, -0.0, 0.0, 1.0], all four hunks either did nothing for the reported bug or made results worse: - safe_coerce_scalar normalizes the SQL literal, but the reported filter `x < 0.0` already carries +0.0, so the -0.0 row is still returned. It also turns `x < -0.0` and `x >= -0.0` from IEEE-correct into wrong, because the data side keeps its sign while the literal loses it. - OrderableScalarValue::cmp made -0.0 == +0.0 while ScalarValue's PartialEq and Hash stayed bitwise, so bitmap training still emits two zero keys and the load path collapses them at bitmap.rs:430, orphaning one bitmap. Rows under the dropped key become unreachable, and the loss is re-serialized on remap/merge/update. It also breaks the Ord/Eq contract. - SargableQuery::to_expr builds the recheck expression; semantics there belong to the expression layer, not to the index. - ArrowScalar::compute_row contradicts the type's documented total ordering and arrow-stats' find_extrema_float, which selects extrema with total_cmp precisely to distinguish the two zeros.
Scalar indices select candidates in arrow's total order, where -0.0 sorts strictly below +0.0, and bloom filters hash the raw float bytes. Expression evaluation follows IEEE 754, where the two compare equal. A query on one encoding therefore prunes the rows stored under the other before any recheck can look at them: a btree page or zone whose extremum is -0.0 is skipped for `= +0.0`, a bitmap key lookup finds only one of the two keys, and a bloom probe misses the other block. Nothing is lost today because DataFusion 54 evaluates float comparisons in total order too, so index and scan agree on the same wrong answer. It starts losing rows the moment expression evaluation becomes IEEE-correct, which is what the DataFusion 55 signed-zero normalization (apache/datafusion#22835) does. So widen the query, not the comparators: a query on a float zero offers both encodings as candidates and reports AtMost, and the parser forces the matching recheck so expression evaluation still decides which rows match. Candidate sets only grow, never shrink, which keeps results identical on DataFusion 54 and makes them IEEE-correct on 55 without rebuilding any index. Range bounds only need widening where an inclusive bound leaves the other encoding out (`>= +0.0`, `<= -0.0`); exclusive bounds already admit every row IEEE 754 matches, so `> 0.0` and `< 0.0` stay exact.
# Conflicts: # rust/arrow-scalar/src/lib.rs # rust/lance-index/src/scalar.rs # rust/lance-index/src/scalar/btree.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Review of the previous commit turned up two ways the unconditional widening was wrong. Callers that use an index result as the answer cannot take a wider candidate set. merge_insert's key lookup builds its own query with `needs_recheck: false` and then panics on a non-exact result; the label-list sub-index turns one into an internal error. So make the caller's intent explicit: `SearchOptions` grows an `is_rechecked` flag, `ScalarIndexExpr::evaluate_with_options` sets it from the plan's `needs_recheck`, and btree and bitmap widen only when it is on. Zone map and bloom keep widening unconditionally, since a zone result is always `AtMost` and every caller already narrows it down. The recheck also has to cover more than the queries that get rewritten. A bound like `> -0.0` needs no rewrite, yet total order still admits `+0.0`, which IEEE 754 excludes. Deciding exactness from "did the query get rewritten" left those shapes claiming `Exact`, and the V2 read path keys off exactly that claim rather than the plan flag. Both now come from one boolean.
# Conflicts: # rust/arrow-scalar/src/lib.rs # rust/lance-index/src/scalar.rs # rust/lance-index/src/scalar/btree.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Replaces the index-side candidate widening with a rewrite of the filter expression, so scan and index paths both answer zero comparisons the way IEEE 754 and SQL define, with no recheck.
Filter planning was not the only place Lance compiles an expression, so a projected copy of the same predicate, and a memtable scan, still answered by Arrow's total order. Also covers a zero literal on the probe side of IN, IS [NOT] DISTINCT FROM, and restores the IS NOT NULL elision for NOT IN.
IS [NOT] DISTINCT FROM expanded into a pair of probes that matched again on the next optimize pass, so an expression re-optimized N times grew 2^N. The memtable BTree fast path is chosen from the un-optimized filter, so a float zero got a bit-exact lookup while the scan beside it answered per IEEE 754.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
An IN list can hold hundreds of literals and each one was going through the simplifier for nothing. Paired measurement on a 2048-element int list: 18.6ms to 6.9ms for 20 optimizations.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
LuciferYang
commented
Aug 31, 2026
The test failures seems unrelated to this PR. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The effective PR patch remains unchanged after merging main. The compact fragment-reuse remapper sits below scalar-index predicate evaluation, and the signed-zero scalar-index pushdown regression still passes, so the prior acceptance basis holds.
Uh oh!
There was an error while loading. Please reload this page.
LuciferYang
commented
Sep 1, 2026
Thank you @Xuanwo |
Problem
Fixes#5868.
IEEE 754 and SQL treat
-0.0and0.0as one number. Arrow and DataFusion 54 order floats by total order, which ranks-0.0strictly below0.0, and test equality onto_bits(). A filter over a float column therefore answers by encoding rather than by value. With this data:value < 0.0returns ids 1, 3, 6, 7, and id 1 is-0.0, which is not less than zero.value = 0.0returns id 0 and misses id 1.Approach
Rewrite the literal at the end of
Planner::optimize_expr, replacing a zero with whichever encoding answers the operator correctly:value < 0.0,value >= 0.0-0.0value <= 0.0,value > 0.0+0.0value = 0.0value IN (-0.0, 0.0)value != 0.0value NOT IN (-0.0, 0.0)value IN (0.0, 1.0)value IN (-0.0, 0.0, 1.0)0.0 IN (a, b)a IN (-0.0, 0.0) OR b IN (-0.0, 0.0)value IS NOT DISTINCT FROM 0.0(value IN (-0.0, 0.0)) IS TRUEvalue IS DISTINCT FROM 0.0(value IN (-0.0, 0.0)) IS NOT TRUEThe ordered comparisons stay a single literal, so they cost the same as before. An equality against zero becomes a two-value lookup. The rewrite needs no recheck and changes no index code: the scalar indices and the in-place filter both keep their existing behaviour, and they now agree with each other.
IS [NOT] DISTINCT FROMis reachable from a hand-builtExprbut not from SQL, which rejects it inplanner.rs. TheIS TRUEpairing is what keeps the null case decided while naming the operand once:NULL IN (..)is NULL, andNULL IS TRUEis false, which is what distinctness means for a null against a non-null literal. An earlier revision guarded that arm to a bare column so it could writeIS NOT NULL AND IN (..), which names the operand twice; bailing out on anything else leftfilter_expranswering computed operands likevalue * 2.0on Arrow's order, so the guard bought a rare double evaluation at the price of wrong rows.The rewrite runs in two places:
normalize_zero_comparisonsbefore simplification andrewrite_signed_zero_comparisonsafter it. Coercion has to precede both, orvalue = 0keeps an integer literal and never reaches the float arm.normalize_zero_comparisonsexists because a rewrite that only runs on the finished expression cannot reach a comparison whose zero does not exist yet.ExprSimplifier::simplifyfolds an operand and everything above it in one pass, so-1.0 * 0.0 < (1.0 - 1.0)went straight to a boolean decided by Arrow's total order, answeringtruewhere IEEE saysfalse. It walks bottom-up and folds only the operands of the node in hand, so by the time any container is folded, every comparison inside it already carries the corrected literal.That traversal deliberately does not enumerate which containers may sit above a comparison. An earlier revision listed
AND,ORandNOT, which leftIS TRUE,IS FALSE,= TRUE,CAST(.. AS BOOLEAN)andIN (TRUE)folding the inner comparison under the old semantics, and any such list would keep missing the next spelling.An operand that is already a literal skips the simplifier, because a literal cannot fold further and an
INlist can hold hundreds of them. Without that, planning a large list roughly doubled in cost. Paired measurement, same binary with only the short-circuit toggled, 20 optimizations each: a 2048-element integer list went from 18.62ms to 6.89ms, a 256-element one from 2.10ms to 0.90ms, and a 257-element float list, which still has to be widened, from 2.34ms to 1.03ms.The pass after
simplifycatches what only becomes visible later:simplifyis what expandsBETWEENinto two comparisons and folds the casts coercion inserts.folded_constant_comparisons_use_ieee_semanticscovers 25 shapes across both, and thebetweencase of the fixed-point test covers the last.One consequence worth flagging for review:
BETWEENneeded its own arm in the rewrite. It normally arrives already expanded into>=and<=, which is why it had none, but a fully constantBETWEENnever arrives expanded becausesimplifyexpands and folds it in the same pass. The arm giveslowthe-0.0encoding andhighthe+0.0one, matching what the expanded operators would take.The hook is
optimize_expr, notcreate_filter_plan, because the latter is not the only entry point.projection.rsand the memtable scanner'splan_full_scanfamily calloptimize_exprand then build a physical expression directly, so hooking the filter plan would let one predicate get two different answers inside a single result set.The output has to be a fixed point of
optimize_expr, not of the rewrite alone. DataFusion'sShortenInListSimplifierexpands anINlist of at most three elements over a bare column back into anORchain, and the scan path optimizes twice, so the second pass can reprocess the rewrite's own output. That showed up asvalue = 0.0planning tovalue IN ([-0,0]) OR value IN ([-0,0])and searching the index twice.rewrite_nodetherefore dedupes equivalentORandANDoperands, andoptimizing_twice_changes_nothingdrives a realPlannerrather than the rewrite by itself. That property is now load-bearing twice over: with two passes insideoptimize_exprand the scan path calling it twice, a zero predicate goes through the rewrite four times.Behaviour change
value = 0now matches rows holding-0.0, in Rust and through the Python and Java bindings.The test that pinned the bug
test_query_float_special_valuescheckedvalue > 0.0,value < 0.0andvalue = 0.0withtest_filter, which compares Lance against the same DataFusion release. That makes the reference implementation the source of the bug, so the test passed on the wrong answer. Those three cases now assert row ids, and the case list now also covers both spellings of the literal, a literal on the left,BETWEEN,IN, and composition withIS NULL.assert_filter_idsruns every predicate twice, once through the dataset's index and once with scalar indices turned off, becauseDatasetTestCasesnever generates the no-index variant on its own.Out of scope
None of these turn on a zero literal, so the rewrite leaves them where they are:
>and>=. The new assertions pin this rather than change it.array_hasfamily. There is no literal to rewrite.test_merge_insert_on_float_zero_keynow runs with and without an index to pin current behaviour.safe_coerce_scalarhas no Float16 arm, so no numeric literal resolves against a Float16 column at all. Tracked in bug: filtering a Float16 column with a numeric literal always fails #8846 and fixed by fix(datafusion): coerce numeric literals to and from Float16 #8847, which makes the Float16 branch of this rewrite reachable; it is unreachable dead code until that lands.distance_range. Its bounds do not pass throughPlanner.Test plan
cargo test -p lance-datafusion(142 passed), including 69 tests in the newsigned_zeromodulecargo test -p lance-index(1173 passed)cargo test -p lance --lib -- --test-threads=1(3179 passed)cargo test -p lance --test integration_tests --features slow_tests -- --test-threads=1(51 passed)cargo clippy --all --tests --benches -- -D warningsclean,cargo fmt --allappliedhalfdependency inlance-datafusion