fix: emit deferred unmatched rows when memory-limited NestedLoopJoin exhausts its left side - #24746
Merged
Merged
Conversation
…exhausts its left side In memory-limited mode the left side is processed in chunks and the right side is replayed for each chunk, so per-right-batch match bitmaps are merged into `global_right_bitmaps` and their emission is deferred to `EmitGlobalRightUnmatched`. `handle_buffering_left` ended the stream as soon as a load produced no batches, described as the "no data at all" case. That exit also fires on the load after the final left chunk, so the accumulated bitmaps were discarded and any probe-side row that no chunk matched was never emitted: the query returned fewer rows than the same query with an ample memory pool, without an error. Route that exit to `EmitGlobalRightUnmatched` when the join tracks unmatched probe-side rows. A left side that was empty from the start is unaffected, since no bitmaps have been accumulated and that state finishes immediately. Closes apache#24745 Co-authored-by: Claude Code
Clippy runs with `-D warnings` over all targets, where `(state >> 33) as u64` on a `u64` trips `unnecessary_cast`. Co-authored-by: Claude Code
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24746 +/- ##
==========================================
- Coverage 81.47% 81.47% -0.01%
==========================================
Files 1122 1122
Lines 403629 403632 +3
Branches 403629 403632 +3
==========================================
- Hits 328866 328863 -3
- Misses 55510 55511 +1
- Partials 19253 19258 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sunchao
approved these changes
Aug 28, 2026
sunchao
left a comment
Member
There was a problem hiding this comment.
Thanks, Liang-Chi. LGTM.
I reviewed 9fef857 against c4910e0 and found no blocking issues. The terminal empty-load path now emits the accumulated right-side results, and clearing right_data correctly opens a fresh replay after the previous pass has finished.
Validation:
- All three new regression tests fail on the base and pass with this fix.
- All 1,116 existing join tests pass.
- All 540 additional direct-NLJ cases match an independent row oracle, including 420 spilling cases. These cover the five right-emission join types, NULLs, duplicates, empty inputs/projections, and chunk boundaries. The 164 failing base cases all pass on this head.
All 41 current CI checks are green. I did not rerun the full workspace suite locally.
Member
Author
|
Thank you @sunchao ! |
viirya
added a commit
to viirya/arrow-datafusion
that referenced
this pull request
Aug 28, 2026
…lback Rebase of the coordinated-fallback work onto current main. The memory-limited NestedLoopJoin fallback built a per-partition `JoinLeftData` with `probe_threads_counter == 1`. For join types whose final emission reads the visited-left bitmap (LEFT, LEFT SEMI, LEFT ANTI, LEFT MARK, FULL) each right partition therefore emitted from a bitmap that had only seen its own right rows, so unmatched rows came out once per partition and rows matched only in another partition came out as unmatched. Because those results are wrong, the fallback was refused for that combination and the query failed with ResourcesExhausted instead of spilling. `FallbackCoordinator` now loads each chunk once via a leader partition and publishes it as a shared `Arc<JoinLeftData>` whose probe-thread counter is seeded with `right_partition_count`, so the last partition to finish a chunk emits its unmatched left rows -- matching how the single-pass path coordinates through `collect_left_input(.., probe_threads_count)`. Those join types now spill instead of erroring. The coordination assumes all right partitions run in one process. Distributed engines run each partition as an independent task with its own coordinator, so the shared counter would never reach zero and the fallback would stall; `enable_nlj_coordinated_fallback = false` lets them opt out and keep the previous fail-fast behavior for the affected join types. Rebase notes: main since shares the spilled left side across partitions via `OnceAsync<LeftLoad>`, so the original `left_spill_fut`/`OnceFut` half of this change is dropped in favor of main's mechanism. The `EmitGlobalRightUnmatched` routing added in apache#24746 is preserved on the coordinator's exhausted-left path, keeping its three regression tests green. Co-authored-by: Claude Code
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?
Rationale for this change
A memory-limited
NestedLoopJoinExecsilently returned fewer rows than the same query with an ample memory pool. No error was raised, so the loss is invisible to the caller.In memory-limited mode the left side is processed in chunks and the right side is replayed for each chunk. A per-right-batch match bitmap therefore cannot be emitted as soon as one chunk finishes probing it — a later chunk may still match those rows. The operator already handles this by merging each bitmap into
SpillStateActive::global_right_bitmapsand deferring emission toNLJState::EmitGlobalRightUnmatched, which is reached fromhandle_emit_left_unmatchedonce the left side is exhausted.handle_buffering_lefthad a separate early exit:Despite the comment, this also fires on the load that follows the final left chunk, when the left side is exhausted and there is nothing more to buffer. Ending the stream there discards the accumulated bitmaps, so every probe-side row that no chunk matched is dropped.
Instrumenting a failing
LEFT JOINshowed 39 bitmap merges and zero emissions, with the completion path only ever running whileleft_exhaustedwas stillfalse.The affected queries reach the operator as swapped join types (
Right,RightAnti,RightSemi) or asFull, so the rows needing unmatched emission are on the operator's probe side. Measured against an unlimited pool (l= 200 rows,r= 90 rows,target_partitions = 1,batch_size = 16, 64-byte pool):NOT EXISTS (... l.v > r.w)LEFT JOIN ... ON l.v > r.wFULL JOIN ... ON l.v > r.wINNERand explicitRIGHT JOINwere already correct.This is pre-existing rather than a regression from #24675: that PR only changes the spill gate in the same file, and its condition (
need_produce_result_in_final(join_type) && right_partition_count > 1) does not cover this path.What changes are included in this PR?
Route that early exit to
EmitGlobalRightUnmatchedinstead ofDonewhen the join tracks unmatched probe-side rows, clearingright_dataso a fresh replay pass is opened. A left side that was empty from the start keeps its previous behaviour: no bitmaps have been accumulated, so that state reports nothing unmatched and finishes immediately.Spilling is preserved rather than refused — the fix corrects the emission instead of turning these queries into
ResourcesExhaustederrors.Are these changes tested?
Yes,
datafusion/core/tests/memory_limit/nlj_spill_unmatched.rsadds three tests comparing memory-limited results against an ample pool forLEFT JOIN,LEFT ANTI, andFULL JOIN. Without the production change they fail with0 instead of 2,9334 instead of 9336, and9337 instead of 9339; with it they pass.The existing
nested_loop_joinunit tests (46) and the widerjoinssuite (1116) pass unchanged, includingtest_nlj_memory_limited_right_join, which asserts that a spillingRIGHTjoin still returns its unmatched rows.Are there any user-facing changes?
No API changes. Queries that previously lost rows under a memory limit now return the correct result.