Skip to content

fix: emit unmatched NULL-key right rows in RIGHT/FULL PiecewiseMergeJoin - #24336

Merged
viirya merged 3 commits into
apache:mainfrom
viirya:pwmj-right-full-null-fix
Aug 13, 2026
Merged

fix: emit unmatched NULL-key right rows in RIGHT/FULL PiecewiseMergeJoin#24336
viirya merged 3 commits into
apache:mainfrom
viirya:pwmj-right-full-null-fix

Conversation

@viirya

@viirya viirya commented Aug 13, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

A RIGHT/FULL PiecewiseMergeJoin with a range predicate drops an unmatched right-side row whose join key is NULL. A NULL key never matches (NULL < x is UNKNOWN), so in a RIGHT/FULL join the row is unmatched and must still be emitted with NULLs on the left — but PiecewiseMergeJoinExec omits it, diverging from NestedLoopJoin.

create table l(v int) as values (5);
create table r(v int) as values (10), (NULL);
select l.v, r.v from l right join r on l.v < r.v;   -- drops (NULL, NULL)

Root cause: resolve_classic_join starts the match scan past the streamed side's NULL-keyed rows (they sort to the front under nulls_first). Those rows are never revisited, so for Right/Full they were never added to unmatched_indices and got dropped.

What changes are included in this PR?

  • In resolve_classic_join, when skipping the streamed side's leading NULL-key rows, record them as unmatched for Right/Full joins so they are emitted (with NULLs on the buffered side).

Are these changes tested?

Yes.

  • Regression test in pwmj.slt: a RIGHT JOIN over the existing null_join_* tables now emits the (NULL, NULL) row. The test fails on main (the row is dropped) and passes with this change.
  • Verified more broadly with a differential fuzz against NestedLoopJoin (same SQL, enable_piecewise_merge_join on vs off): 1200 checks over random RIGHT JOIN inputs with </<=/>/>= and high right-side NULL density, 0 mismatches.

Are there any user-facing changes?

RIGHT/FULL range joins via PiecewiseMergeJoin (behind enable_piecewise_merge_join, default off) now return unmatched right rows with NULL keys, matching NestedLoopJoin. No API changes.

`resolve_classic_join` starts the match scan past the streamed side's
NULL-keyed rows, which sort to the front under `nulls_first`. A NULL join
key never matches (`NullEqualsNothing`), so for `Right`/`Full` joins those
rows are unmatched and must still be emitted — but because the scan skips
them, they were never recorded in `unmatched_indices` and got dropped.

Record the skipped NULL-key streamed rows as unmatched for `Right`/`Full`
so they are emitted with NULLs on the buffered side, matching
`NestedLoopJoin`.

Closes apache#24335.

Co-authored-by: Claude Code
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Aug 13, 2026
@viirya
viirya requested a review from comphead August 13, 2026 16:44
@codecov-commenter

codecov-commenter commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.17%. Comparing base (66c3840) to head (be109bf).
⚠️ Report is 148 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24336      +/-   ##
==========================================
+ Coverage   80.86%   81.17%   +0.31%     
==========================================
  Files        1101     1109       +8     
  Lines      375446   388038   +12592     
  Branches   375446   388038   +12592     
==========================================
+ Hits       303592   314980   +11388     
- Misses      53761    54513     +752     
- Partials    18093    18545     +452     

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

# NULL rows are skipped by the match scan and were previously dropped instead of
# being reported as unmatched.
query II
SELECT t1.id AS left_id, t2.id AS right_id

@comphead comphead Aug 13, 2026

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.

is FULL join supposed to be tested as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good question — the fix path is Right | Full, so it covers FULL JOIN too. I've added a FULL JOIN test in the latest commit.

I couldn't reuse the null_join_* tables for it: on this branch's base, a FULL JOIN over those still drops the unmatched left NULL-keyed row (t1.id = NULL). That's a separate pre-existing bug — the classic Left/Full unmatched-row drop, which #23870 fixes but main doesn't have yet — unrelated to the right-side NULL fix here, so a FULL JOIN on null_join_* would fail on this base for that other reason.

To keep the test focused on what this PR fixes (the unmatched right-side NULL row), the added FULL JOIN case uses data where every left row matches, so only the right-NULL emission is exercised.

The fix applies to both `Right` and `Full`. Add a `FULL JOIN` test whose
left rows all match, so it exercises only the right-side NULL-key
unmatched-row emission fixed here (a separate pre-existing left-side
unmatched-row drop, not addressed by this PR, would otherwise interfere).

Co-authored-by: Claude Code

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

Thanks @viirya makes sense to me

Comment thread datafusion/physical-plan/src/joins/piecewise_merge_join/classic_join.rs Outdated
Address review: cast `stream_null_idx` to `u32` for the loop bound instead
of casting each index inside the loop.

Co-authored-by: Claude Code
@viirya
viirya added this pull request to the merge queue Aug 13, 2026
@viirya

viirya commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

Thanks @comphead for review!

Merged via the queue into apache:main with commit 0a429a3 Aug 13, 2026
64 of 65 checks passed
@viirya
viirya deleted the pwmj-right-full-null-fix branch August 13, 2026 22:10
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.

PiecewiseMergeJoin drops unmatched NULL-key right rows in RIGHT/FULL joins

3 participants