Repository navigation
Conversation
…25134) ## Which issue does this PR close? Closes apache#25133. ## Rationale for this change Sort-merge mark, semi, and anti joins can fail when floating-point key groups cross input batches. Float32/Float64 keys containing NaN can panic, and Float16 keys can return an unsupported-type error, even though the main merge scan supports these keys. ## What changes are included in this PR? Use the existing `JoinKeyComparator` for batch-boundary comparisons involving floating-point keys, matching the main merge scan's equality semantics. Slice the right-hand keys to one row before constructing the comparator; the left-hand keys are already single-row slices. This bounds signed-zero normalization to the rows being compared. Integer-only comparisons keep the existing scalar path. There are no memory-accounting, buffering, configuration, dependency, or public-API changes. ## What is the testing strategy for this PR? `join_float_key_batch_boundaries` in `datafusion/physical-plan/src/joins/sort_merge_join/tests.rs` exercises actual one- and two-row input batches. Its 144 cases cover Float16/32/64, left/right mark/semi/anti joins, both null-equality modes, and ascending/descending order. Inputs include nulls, infinities, signed zero, unmatched keys, and NaN groups spanning batches. Assertions check exact output row IDs/order, key types, and mark values. A physical-plan test is used because the reproducer needs precise input batch boundaries, independent of SQL optimizer choices. On unpatched Apache main (`40488988ad596c9b093ad60e1453430d803ce33c`) plus only the new tests, all 144 cases fail: 96 NaN panics and 48 Float16 unsupported-type errors. Local validation of `9ea2dea17b53b8ed2f8ea877df3d95604b87f450`: - Physical-plan library: **2,096 passed**, including all 144 new regression cases. - Full extended workspace command below: **11,454 Rust tests passed, 0 failed, 8 ignored**; the SQL logic harness also completed **513/513 files** successfully. - CLI: **107 reported passed, 0 failed**. Six environment-gated cases did not exercise external storage. - `cargo fmt --all -- --check`, `git diff --check`, `cargo clippy --locked --all-targets --all-features -- -D warnings`, and the complete `./dev/rust_lint.sh` suite, including documentation: **passed**. The lint suite used the CI-pinned Hawkeye 7.0.0. ```sh ulimit -n 4096 RUST_BACKTRACE=1 cargo test --locked --profile ci --no-fail-fast \ --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli \ --workspace --lib --tests --bins \ --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption cargo test --locked --profile ci -p datafusion-cli ``` The initial workspace attempt failed four sort-only RSS runners (`sort_no_mem_limit_runner` and `sort_with_mem_limit_{1,2,3}_runner`) with a zero baseline RSS. All four passed an isolated serial rerun, and the subsequent full workspace rerun passed without source/test changes. The cause of the initial intermittent failures has not been established. Supplemental standalone validation, not committed to this PR: an independent row-by-row oracle passed 2,592 cases covering asymmetric/empty input batches, filters, composite keys, both null placements and the float/join options above. An allocation probe confirmed the single-row slicing bounds comparator allocation independently of the full input batch size (32 through 1,048,576 rows). These checks supplement, rather than replace, the committed regression tests. The existing eight-case `sort_merge_join` release benchmark completed on the exact upstream base and PR, with an identical harness, compiler and lockfile. Runs were ordered base→PR and PR→base, using 30 samples, one-second warmup and a three-second requested measurement time. Short-run differences were mixed; the left-semi delta reversed from +3.5% to -2.6%. Inner 1-to-1 was initially slower (+9.9% and +3.7%), so it was repeated in three longer, alternating-order pairs (50 samples, eight-second requested measurement time). The PR/base point-estimate differences were -0.17%, +0.26% and -0.84%; Criterion reported change within its noise threshold or no change. The initial slowdown did not reproduce. These are integer-key control workloads on a development machine, not a floating-point speedup benchmark; no speedup or general performance-neutrality claim is made. Upstream test workflows currently require maintainer approval (`action_required`); local results above are not a claim that GitHub CI or coverage checks have passed. ## Are there any user-facing changes? Affected sort-merge joins can complete instead of failing at floating-point batch boundaries. No public API changes. AI-assisted contribution (OpenAI Codex). (cherry picked from commit 2abf69d) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
3 of 18 tasks
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?
55.3.0(patch) Release (Nov 2026) #26868 (the 55.3.0 patch release tracking issue).branch-55. fix(smj): handle floating-point keys across batch boundaries #25134 fixed Sort-merge joins fail when floating-point key groups cross input batches #25133.Rationale for this change
On 55.2.0, a mark, semi or anti sort-merge join on a floating-point key fails when a key group ends at the end of an input batch: a Float32 or Float64 key that is NaN panics with
called `Option::unwrap()` on a `None` value, and a Float16 key returnsUnsupported data type in sort merge join comparator: Float16. The main merge scan handles the same keys. These are failed queries, which makes this eligible for backport as a correctness fix.#25134 fixed it on
main(labelledv56.0.0); it is onbranch-56but not onbranch-55. It was requested for 55.3.0 on #26868 (#26868 (comment)).What changes are included in this PR?
git cherry-pick -xof 2abf69d (fix(smj): handle floating-point keys across batch boundaries #25134, by @jeffw13). It applied without conflicts and this PR changes nothing else.datafusion/physical-plan/src/joins/sort_merge_join/bitwise_stream.rs:keys_matchuses the existingJoinKeyComparatorfor floating-point keys, matching the merge scan's equality (including signed zero), instead of the scalar comparator whosepartial_cmppanics on NaN. Integer-only comparisons keep the scalar path.datafusion/physical-plan/src/joins/sort_merge_join/tests.rs:join_float_key_batch_boundaries, 144 cases (Float16/32/64, left/right mark/semi/anti, both null-equality modes, ascending/descending, one- and two-row input batches).Are these changes tested?
Yes, by the 144 cases that #25134 added. On this branch (Rust 1.95.0) I ran:
cargo test -p datafusion-physical-plan --lib: 1844 passed.bitwise_stream.rsreverted and the tests kept,cargo test -p datafusion-physical-plan --lib sort_merge_joinfails exactly the 144 new cases (96 NaN panics and 48 Float16 errors).cargo fmt --all -- --checkandcargo clippy -p datafusion-physical-plan --all-targets --all-features -- -D warningspass.Are there any user-facing changes?
Affected sort-merge joins complete instead of failing at floating-point batch boundaries. No public API change.
🤖 Generated with Claude Code