fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side - #24675
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24675 +/- ##
==========================================
+ Coverage 81.45% 81.47% +0.01%
==========================================
Files 1122 1122
Lines 403051 403629 +578
Branches 403051 403629 +578
==========================================
+ Hits 328317 328855 +538
- Misses 55498 55517 +19
- Partials 19236 19257 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The change looks good to me. Expanding the fallback gate to the join types that depend on the visited-left bitmap avoids returning incorrect results when there are multiple right partitions, while keeping the fallback available for the join types where it is safe.
I have one non-blocking suggestion for some additional test coverage.
| @@ -3872,24 +3871,34 @@ pub(crate) mod tests { | |||
| .await?; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Could we add a tight-memory, single-right-partition spill test for LeftSemi, LeftAnti, and LeftMark as well? The new gate specifically applies when there is more than one right partition, but the current single-partition success coverage only exercises Left and Full. Adding these cases would help make sure the supported single-partition fallback continues to work while #22038 remains open.
There was a problem hiding this comment.
sure, added in ab71c9c: test_nlj_memory_limited_left_semi_join, test_nlj_memory_limited_left_anti_join, test_nlj_memory_limited_left_mark_join, same shape as the existing single-partition tests, 50-byte pool with spill_count asserted > 0 so the fallback path is actually exercised, results snapshot-checked against the unlimited-memory expectations.
|
Thanks @ranflarion and @kosiew, LGTM |
|
@ranflarion |
…joins with a multi-partition probe side
…tAnti, and LeftMark
ab71c9c to
ce9440f
Compare
done |
…exhausts its left side (apache#24746) ## Which issue does this PR close? - Closes apache#24745. ## Rationale for this change A memory-limited `NestedLoopJoinExec` silently 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_bitmaps` and deferring emission to `NLJState::EmitGlobalRightUnmatched`, which is reached from `handle_emit_left_unmatched` once the left side is exhausted. `handle_buffering_left` had a separate early exit: ```rust if active.pending_batches.is_empty() { // No data at all — go directly to Done self.left_exhausted = true; self.state = NLJState::Done; return ControlFlow::Continue(()); } ``` 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 JOIN` showed 39 bitmap merges and zero emissions, with the completion path only ever running while `left_exhausted` was still `false`. The affected queries reach the operator as swapped join types (`Right`, `RightAnti`, `RightSemi`) or as `Full`, 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): | query | ample | memory-limited (before) | | --- | --- | --- | | `NOT EXISTS (... l.v > r.w)` | 2 | 0 | | `LEFT JOIN ... ON l.v > r.w` | 9336 | 9334 | | `FULL JOIN ... ON l.v > r.w` | 9339 | 9337 | `INNER` and explicit `RIGHT JOIN` were already correct. This is pre-existing rather than a regression from apache#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 `EmitGlobalRightUnmatched` instead of `Done` when the join tracks unmatched probe-side rows, clearing `right_data` so 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 `ResourcesExhausted` errors. ## Are these changes tested? Yes, `datafusion/core/tests/memory_limit/nlj_spill_unmatched.rs` adds three tests comparing memory-limited results against an ample pool for `LEFT JOIN`, `LEFT ANTI`, and `FULL JOIN`. Without the production change they fail with `0 instead of 2`, `9334 instead of 9336`, and `9337 instead of 9339`; with it they pass. The existing `nested_loop_join` unit tests (46) and the wider `joins` suite (1116) pass unchanged, including `test_nlj_memory_limited_right_join`, which asserts that a spilling `RIGHT` join 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.
Which issue does this PR close?
Rationale for this change
The memory-limited fallback returns wrong rows for join types whose final emission reads the visited-left bitmap (LEFT, LEFT SEMI, LEFT ANTI, LEFT MARK) when the probe side has more than one partition. The fallback rebuilds a per-partition
JoinLeftDataper left chunk withprobe_threads_counter == 1and a per-partition bitmap, so every partition believes it is the last to finish probing and emits its own unmatched set, from a bitmap that only saw its own right rows. Unmatched rows are emitted once per partition, and rows matched only in another partition are emitted as unmatched.Proof, using this file's own
build_table/prepare_join_filter/multi_partitioned_join_collectfixtures (4 right partitions) with a 100-byte memory limit versus unbounded:The existing gate covers only FULL. This extends it to every
need_produce_result_in_finaltype, so an over-budget build side fails withResourcesExhaustedinstead of returning wrong rows. Right-side emission types (RIGHT, RIGHT SEMI, RIGHT ANTI, RIGHT MARK) keep the fallback: each partition owns its right rows exclusively, so their bitmaps are complete per partition. Proper cross-partition coordination of the left bitmap, which would re-enable the fallback for these types, is #22038.What changes are included in this PR?
The
full_join_multi_partitioncondition inNestedLoopJoinExec::executebecomesneed_produce_result_in_final(self.join_type) && right_partition_count > 1, with the comment updated to describe the failure mode.test_overallocationmoves LEFT/LEFT SEMI/LEFT ANTI/LEFT MARK from the succeed-via-fallback group (which collected but never checked the row values) into the must-OOM group alongside FULL.Are these changes tested?
test_overallocationnow asserts theResourcesExhaustedrefusal for all five gated join types with a multi-partition probe side, and still asserts fallback success for Inner and the right-emission types. All 42nested_loop_jointests pass;./dev/rust_lint.shis clean.Are there any user-facing changes?
Left-family nested loop joins with a multi-partition probe side whose build side exceeds the memory budget now fail with
ResourcesExhaustedinstead of returning incorrect results. No API changes.