executor: skip probe for empty hash joins - #11001
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughEmpty-build hash joins now publish probe-skip state, bypass probe processing for eligible join kinds, and prevent pipeline sources from running when transforms can finish without input. Tests cover empty, null-key, filtered, and spilled builds, plus anti-semi behavior. ChangesEmpty-build probe skipping
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change avoids unnecessary probe-side processing when supported hash joins have an empty build side; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant BuildSide
participant Join
participant ProbeTransform
participant PipelineExec
participant ProbeSource
BuildSide->>Join: publish empty build state
ProbeTransform->>Join: request shouldSkipProbe()
Join-->>ProbeTransform: return skip decision
PipelineExec->>ProbeTransform: request shouldSkipSource()
ProbeTransform-->>PipelineExec: return true
PipelineExec-->>ProbeSource: skip source lifecycle
ProbeTransform->>Join: finalize skipped probe
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dbms/src/Interpreters/Join.h (1)
250-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase for newly introduced member names.
The new state variables use snake_case, contrary to the C++ naming guideline. Rename them consistently and update all references:
dbms/src/Interpreters/Join.h#L250-L258,449-449: renamebuild_side_emptytobuildSideEmpty.dbms/src/Interpreters/JoinV2/HashJoin.h#L77-L83,156-156: renamebuild_side_emptytobuildSideEmpty.dbms/src/Interpreters/JoinV2/HashJoin.cpp#L468-L468: update the state publication reference.dbms/src/Flash/Pipeline/Exec/PipelineExec.h#L83-L83: renamesource_prefix_executedtosourcePrefixExecuted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Interpreters/Join.h` around lines 250 - 258, Rename the newly introduced state members to camelCase and update every reference: in dbms/src/Interpreters/Join.h lines 250-258 and 449-449, rename build_side_empty to buildSideEmpty; in dbms/src/Interpreters/JoinV2/HashJoin.h lines 77-83 and 156-156, rename build_side_empty to buildSideEmpty; in dbms/src/Interpreters/JoinV2/HashJoin.cpp line 468, update the state publication reference; and in dbms/src/Flash/Pipeline/Exec/PipelineExec.h line 83, rename source_prefix_executed to sourcePrefixExecuted.Source: Coding guidelines
dbms/src/Flash/tests/gtest_join_executor.cpp (1)
443-469: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding empty-build coverage for Cross-join and NullAware semi/anti kinds.
Current new tests cover Inner/Semi/RightSemi (skip) and Anti (no-skip) for the standard equi hash join, but there's no test for
Cross_LeftOuterAnti/Cross_LeftOuterorNullAware_*kinds with an empty build side. Given the skip logic inJoin::joinBlock(Join.cpp Lines 1990-1994) runs before the kind-specific dispatch, a test confirming these families are not incorrectly skipped would close the verification gap raised on theJoin.cppchange.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Flash/tests/gtest_join_executor.cpp` around lines 443 - 469, Add empty-build coverage alongside EmptyBuildAntiSemiJoinStillReadsProbe for Cross_LeftOuterAnti, Cross_LeftOuter, and NullAware semi/anti join kinds. Build requests with an empty build side and non-empty probe side, then verify execution is not skipped and produces the expected probe-side results, covering the pre-dispatch skip logic in Join::joinBlock.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dbms/src/Flash/tests/gtest_join_executor.cpp`:
- Around line 443-469: Add empty-build coverage alongside
EmptyBuildAntiSemiJoinStillReadsProbe for Cross_LeftOuterAnti, Cross_LeftOuter,
and NullAware semi/anti join kinds. Build requests with an empty build side and
non-empty probe side, then verify execution is not skipped and produces the
expected probe-side results, covering the pre-dispatch skip logic in
Join::joinBlock.
In `@dbms/src/Interpreters/Join.h`:
- Around line 250-258: Rename the newly introduced state members to camelCase
and update every reference: in dbms/src/Interpreters/Join.h lines 250-258 and
449-449, rename build_side_empty to buildSideEmpty; in
dbms/src/Interpreters/JoinV2/HashJoin.h lines 77-83 and 156-156, rename
build_side_empty to buildSideEmpty; in dbms/src/Interpreters/JoinV2/HashJoin.cpp
line 468, update the state publication reference; and in
dbms/src/Flash/Pipeline/Exec/PipelineExec.h line 83, rename
source_prefix_executed to sourcePrefixExecuted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33aad696-443c-48c3-b6c6-f48d79afe38e
📒 Files selected for processing (15)
dbms/src/DataStreams/HashJoinProbeExec.cppdbms/src/Flash/Pipeline/Exec/PipelineExec.cppdbms/src/Flash/Pipeline/Exec/PipelineExec.hdbms/src/Flash/Pipeline/Exec/tests/gtest_simple_operator.cppdbms/src/Flash/tests/gtest_join_executor.cppdbms/src/Interpreters/Join.cppdbms/src/Interpreters/Join.hdbms/src/Interpreters/JoinV2/HashJoin.cppdbms/src/Interpreters/JoinV2/HashJoin.hdbms/src/Operators/HashJoinProbeTransformOp.cppdbms/src/Operators/HashJoinProbeTransformOp.hdbms/src/Operators/HashJoinV2ProbeTransformOp.cppdbms/src/Operators/HashJoinV2ProbeTransformOp.hdbms/src/Operators/HashProbeTransformExec.hdbms/src/Operators/Operator.h
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Flash/tests/gtest_spill_join.cpp`:
- Around line 269-271: Update the test around executeStreams in the spill join
case to verify V1 spill fallback behavior, not just the empty result. Add
assertions that external spilling occurred and that the probe scan or probe
executor was invoked, using the existing test instrumentation and counters,
while preserving the current result assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 871d801b-9eaa-458d-ac64-d4c337559ae7
📒 Files selected for processing (2)
dbms/src/Flash/tests/gtest_join_executor.cppdbms/src/Flash/tests/gtest_spill_join.cpp
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
7 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest pull-sanitizer-tsan |
|
@ChangRui-Ryan: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
|
||
| Block HashJoinProbeExec::probe() | ||
| { | ||
| if (probe_process_info.all_rows_joined_finish && join->shouldSkipProbe()) |
There was a problem hiding this comment.
why need to check probe_process_info.all_rows_joined_finish here?
There was a problem hiding this comment.
After tracing the execution order, this check is redundant in the current flow. HashJoinProbeExec enters the probe stage only after waitUntilAllBuildFinished(). If shouldSkipProbe() is true, the build side was already finalized as empty before the first probe call, so no probe block can have been loaded into probe_process_info; all_rows_joined_finish is therefore necessarily true. If the build side is non-empty, shouldSkipProbe() remains false for the whole probe stage.
I originally kept the condition to make the state-machine precondition explicit and defensive, but it is not needed for the current correctness. I will simplify it as suggested.
| block = {}; | ||
| return OperatorStatus::HAS_OUTPUT; | ||
| } | ||
| if unlikely (probe_context.isAllFinished() && join_ptr->shouldSkipProbe()) |
There was a problem hiding this comment.
it looks to me that there is no need to check probe_context.isAllFinished()?
There was a problem hiding this comment.
Ditto. Similar to the legacy HashJoinProbeExec path, this check is redundant under the current HashJoin V2 execution ordering. The probe pipeline is scheduled only after the build pointer-table event has completed and published build_side_empty. If shouldSkipProbe() is true when this probe transform starts, no probe block can have been accepted into probe_context, so probe_context.isAllFinished() is necessarily true. If the build side is non-empty, shouldSkipProbe() remains false throughout the probe stage.
I originally kept the condition to make the state-machine precondition explicit and defensive, but it is not needed for the current correctness. I will simplify it as suggested.
| if (auto ret = probe_transform->tryFillProcessInfoInProbeStage(probe_process_info); | ||
| ret != OperatorStatus::HAS_OUTPUT) | ||
| return ret; | ||
| if (!probe_transform->shouldSkipProbe()) |
There was a problem hiding this comment.
how about check probe_transform->shouldSkipProbe() at the begining of this method, and just set block to {} and return OperatorStatus::HAS_OUTPUT if shouldSkipProbe is true.
There was a problem hiding this comment.
This is different from the redundant state checks above. Returning {} directly here would bypass the completion state machine of the legacy Join implementation used by HashJoinProbeTransformOp.
In particular, it would skip finishOneProbe() for every probe worker and finalizeProbe() for the last one, so active_probe_threads would not reach zero and wait_probe_finished_future would never be finished. It would also bypass the existing post-probe transitions, such as scan-after-probe and restore handling.
For example, with an empty-build inner join and two probe workers, both probe transforms could emit EOF immediately while neither decrements the shared probe-worker count. The join would never observe probe completion.
The current code avoids reading any new probe input, but still enters onOutput() to reuse the normal completion and finalization path.
| { | ||
| join_ptr->finishOneProbe(op_index); | ||
| probe_context.input_is_finished = true; | ||
| block = join_ptr->probeLastResultBlock(op_index); |
There was a problem hiding this comment.
why not just set block to {}
There was a problem hiding this comment.
probeLastResultBlock() is needed by the normal source-EOF path to drain a result block buffered by earlier probe processing. In this empty-build fast path, however, the probe pipeline is scheduled only after the build pointer-table event, and the source is skipped before any probe block can be accepted into this transform. For the skip-eligible V2 join kinds (Inner and Semi), there can be no independently buffered final result in this case.
Therefore, probeLastResultBlock() is guaranteed to be empty here, and setting block to {} directly makes the logical EOF explicit. I will simplify it as suggested.
| Stopwatch all_watch; | ||
| SCOPE_EXIT({ probe_workers_data[stream_index].probe_time += all_watch.elapsedFromLastTime(); }); | ||
|
|
||
| if unlikely (shouldSkipProbe()) |
There was a problem hiding this comment.
why probeBlock is called if shouldSkipProbe is true?
There was a problem hiding this comment.
Under the current HashJoin V2 execution ordering, this branch is unreachable when shouldSkipProbe() is true. The probe pipeline is scheduled only after the build pointer-table event, and HashJoinV2ProbeTransformOp::tryOutputImpl() completes the probe worker before any probe block can be accepted into probe_context or passed to HashJoin::probeBlock().
This branch was added as a defensive fast path for a hypothetical caller that bypasses the probe transform and invokes probeBlock() directly. Such a caller would still produce the correct result by probing the empty hash table; it would only lose this extra optimization.
To keep the implementation focused on the actual pipeline path, I will remove this redundant branch as suggested.
| return {}; | ||
| } | ||
|
|
||
| if unlikely (shouldSkipProbe()) |
There was a problem hiding this comment.
Ditto for the legacy Join::joinBlock() path. HashJoinProbeExec::probe() and HashJoinProbeTransformOp::tryOutputImpl() both handle shouldSkipProbe() before a probe block is passed to Join::joinBlock(), so this branch is unreachable under the current execution flow.
It was only a defensive fast path for a hypothetical direct caller of joinBlock(). Removing it does not affect correctness; such a caller would still produce an empty result by probing the empty hash table, at the cost of unnecessary work.
I will remove it as well to keep the optimization focused on the actual probe transform and pipeline paths.
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: close #11000
Problem Summary
When the build side of a hash join is empty after filtering, the join result may already be known without reading the probe side. However, the current execution pipeline can still scan the entire probe side and perform unnecessary hash lookups against an empty hash table.
For example:
The filter produces an empty build side, but the profile shows:
This causes unnecessary probe-side I/O, block processing, pipeline scheduling, and hash-table lookup overhead. The same problem also applies to eligible Semi Join cases.
What is changed and how it works
This change allows supported hash joins to terminate the probe side early when the finalized build hash table is empty.
The implementation has two parts:
Track whether the finalized build side is empty.
Skip probe processing at both the join and pipeline levels.
PipelineExecthen avoids reading the probe source and finishes the transform normally, without relying on source EOF or repeatedly producing empty blocks.The optimization is enabled for:
RightSemi Join is not currently supported by Hash Join V2 and therefore falls back to the V1 implementation.
Join types whose output still depends on every probe row, such as AntiSemi Join and LeftOuterSemi Join, are not included in this optimization. V1 also keeps the existing behavior when an actual external join spill has occurred.
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests