You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ranflarion (via GitHub)" <gi...@apache.org> on 2026/08/25 20:54:40 UTC
[PR] fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side [datafusion]
ranflarion opened a new pull request, #24675:
URL: https://github.com/apache/datafusion/pull/24675
## Which issue does this PR close?
- Closes #22641.
## 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 `JoinLeftData` per left chunk with `probe_threads_counter == 1` and 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_collect` fixtures (4 right partitions) with a 100-byte memory limit versus unbounded:
```rust
for join_type in [JoinType::Left, JoinType::LeftAnti] {
// unbounded memory -> correct; 100-byte pool -> memory-limited fallback
let (_, correct, _) = multi_partitioned_join_collect(left, right, &join_type, Some(filter), ample_ctx).await?;
let (_, wrong, _) = multi_partitioned_join_collect(left, right, &join_type, Some(filter), tight_ctx).await?;
}
// Left: correct=19 memory_limited=49
// LeftAnti: correct=1 memory_limited=31
```
The existing gate covers only FULL. This extends it to every `need_produce_result_in_final` type, so an over-budget build side fails with `ResourcesExhausted` instead 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_partition` condition in `NestedLoopJoinExec::execute` becomes `need_produce_result_in_final(self.join_type) && right_partition_count > 1`, with the comment updated to describe the failure mode. `test_overallocation` moves 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_overallocation` now asserts the `ResourcesExhausted` refusal 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 42 `nested_loop_join` tests pass; `./dev/rust_lint.sh` is 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 `ResourcesExhausted` instead of returning incorrect results. No API changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24675:
URL: https://github.com/apache/datafusion/pull/24675#discussion_r3862674380
##########
datafusion/physical-plan/src/joins/nested_loop_join.rs:
##########
@@ -3872,24 +3871,34 @@ pub(crate) mod tests {
.await?;
}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24675:
URL: https://github.com/apache/datafusion/pull/24675#issuecomment-5424345996
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24675?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `96.15385%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.44%. Comparing base ([`6e66a85`](https://app.codecov.io/gh/apache/datafusion/commit/6e66a853b664b231737699d6704bdf721d44e92e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4084ad1`](https://app.codecov.io/gh/apache/datafusion/commit/4084ad17c113477cdc02843bd166f2cc0e204da6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 8 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24675?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...fusion/physical-plan/src/joins/nested\_loop\_join.rs](https://app.codecov.io/gh/apache/datafusion/pull/24675?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fjoins%2Fnested_loop_join.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9qb2lucy9uZXN0ZWRfbG9vcF9qb2luLnJz) | 96.15% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24675?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24675 +/- ##
==========================================
- Coverage 81.44% 81.44% -0.01%
==========================================
Files 1118 1118
Lines 399602 399607 +5
Branches 399602 399607 +5
==========================================
- Hits 325460 325458 -2
- Misses 55146 55150 +4
- Partials 18996 18999 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24675?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24675:
URL: https://github.com/apache/datafusion/pull/24675
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side [datafusion]
Posted by "ranflarion (via GitHub)" <gi...@apache.org>.
ranflarion commented on PR #24675:
URL: https://github.com/apache/datafusion/pull/24675#issuecomment-5447937756
> @ranflarion Can you resolve the merge conflicts?
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side [datafusion]
Posted by "ranflarion (via GitHub)" <gi...@apache.org>.
ranflarion commented on code in PR #24675:
URL: https://github.com/apache/datafusion/pull/24675#discussion_r3869214838
##########
datafusion/physical-plan/src/joins/nested_loop_join.rs:
##########
@@ -3872,24 +3871,34 @@ pub(crate) mod tests {
.await?;
}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #24675:
URL: https://github.com/apache/datafusion/pull/24675#issuecomment-5439591327
Thanks @ranflarion and @kosiew, LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: refuse memory-limited NestedLoopJoin fallback for left-emission joins with a multi-partition probe side [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24675:
URL: https://github.com/apache/datafusion/pull/24675#issuecomment-5447827720
@ranflarion
Can you resolve the merge conflicts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org