Skip to content

fix: keep every spilled slice of a sort-merge join inner key group - #24056

Open
buraksenn wants to merge 3 commits into
apache:mainfrom
buraksenn:fix-smj-bitwise-multi-spill
Open

fix: keep every spilled slice of a sort-merge join inner key group#24056
buraksenn wants to merge 3 commits into
apache:mainfrom
buraksenn:fix-smj-bitwise-multi-spill

Conversation

@buraksenn

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Filtered semi, anti, and mark sort-merge joins kept only one finished spill file per inner key group. If a group spilled more than once, each spill replaced the previous file, so read-back could lose earlier rows and produce incorrect results.

What changes are included in this PR?

Keep one InProgressSpillFile open per inner key group, append every overflow to it, and finalize it before filter evaluation.

Are these changes tested?

Yes:

cargo test -p datafusion-physical-plan bitwise_multi_spill_inner_key_group
cargo test -p datafusion-physical-plan
cargo test --test sqllogictests -- sort_merge_join_spill

Are there any user-facing changes?

no

BitwiseSortMergeJoinStream kept only a single finished spill file per
inner key group. When one group spanned several inner batches and the
memory reservation failed more than once, each new spill replaced the
previous file, silently dropping its rows: semi joins lost matches, anti
joins returned extra rows, and mark joins produced wrong marks under
memory pressure.

Keep one InProgressSpillFile open per key group instead: every overflow
appends the freshly buffered slices to the same file, and the writer is
finalized for read-back when evaluation of the group starts (buffering
is complete by then). A group therefore produces exactly one spill file
that holds all of its spilled rows.
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Aug 2, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.86%. Comparing base (66c3840) to head (626f325).

Files with missing lines Patch % Lines
...l-plan/src/joins/sort_merge_join/bitwise_stream.rs 85.71% 0 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24056      +/-   ##
==========================================
- Coverage   80.86%   80.86%   -0.01%     
==========================================
  Files        1101     1101              
  Lines      375446   375455       +9     
  Branches   375446   375455       +9     
==========================================
+ Hits       303592   303594       +2     
- Misses      53761    53765       +4     
- Partials    18093    18096       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @buraksenn

Looks good!

@kumarUjjawal
kumarUjjawal added this pull request to the merge queue Aug 3, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants