Skip to content

minor(test): strengthen bitwise sort-merge join spill coverage - #24192

Open
buraksenn wants to merge 4 commits into
apache:mainfrom
buraksenn:test-smj-bitwise-no-spill
Open

minor(test): strengthen bitwise sort-merge join spill coverage#24192
buraksenn wants to merge 4 commits into
apache:mainfrom
buraksenn:test-smj-bitwise-no-spill

Conversation

@buraksenn

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Continuing the work after #23988 and #24056. BitwiseSortMergeJoinStream (filtered semi/anti/mark joins) buffers the inner key group and spills it under memory pressure, but existing tests only cover the successful spill path:

  • when the memory pool is exhausted and disk spilling is disabled, the error path had no coverage
  • the multi-spill slice retention regression test only asserted the semi join outcome; a dropped slice resurrects the anti row and flips mark to false, which row counts alone cannot catch
  • no spill test exercised NullEqualsNull, so NULL-key group boundary detection through the spill path was untested

What changes are included in this PR?

  • new bitwise_filtered_no_spill: all six semi/anti/mark join types must surface a clear "Disk spilling disabled" error (not spill or panic) when the pool is exhausted with the DiskManager disabled
  • extend bitwise_multi_spill_inner_key_group over LeftAnti/LeftMark, asserting the mark column value
  • new bitwise_spill_null_key_group: under NullEqualsNull, a NULL-key group spanning a batch boundary must spill as one group with one spill file and match a NULL-key outer row
  • extract shared setup of bitwise_spill_with_filter into a fixture

Are these changes tested?

test only change and it can be tested via:

cargo test -p datafusion-physical-plan --lib -- sort_merge_join::tests::bitwise

Are there any user-facing changes?

no test only change

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Aug 8, 2026
@codecov-commenter

codecov-commenter commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.17%. Comparing base (e76f1af) to head (de81e38).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24192      +/-   ##
==========================================
- Coverage   81.17%   81.17%   -0.01%     
==========================================
  Files        1109     1109              
  Lines      388033   388033              
  Branches   388033   388033              
==========================================
- Hits       314992   314985       -7     
- Misses      54501    54507       +6     
- Partials    18540    18541       +1     

☔ 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.

@saadtajwar saadtajwar 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.

Nice! Not a maintainer but these changes make sense and LGTM :)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants