feat(pwmj): support LeftSemi/LeftAnti existence joins via classic scan - #23870
feat(pwmj): support LeftSemi/LeftAnti existence joins via classic scan#23870SubhamSinghal wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23870 +/- ##
==========================================
+ Coverage 80.71% 81.03% +0.31%
==========================================
Files 1090 1105 +15
Lines 370339 381191 +10852
Branches 370339 381191 +10852
==========================================
+ Hits 298925 308895 +9970
- Misses 53605 54002 +397
- Partials 17809 18294 +485 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Oh nice! @coderfender FYI |
|
@kumarUjjawal Can you review this PR? Sorry for tagging you everywhere |
No worries. I'm happy to help. Should we split the benchmark in a new pr so we can assess easily? |
|
benchmark PR: #24160 |
|
Thank you for this really nice PR, I got some suggestions:
This high-level approach LGTM, I might not be able to do detailed follow-up review timely, but it should be good to go if others can do the review. |
Thanks you @2010YOUY01 I will move this forward from here. |
viirya
left a comment
There was a problem hiding this comment.
Took a focused correctness pass over the LeftSemi/LeftAnti path — not a line-by-line/differential review, but I traced the four spots most likely to hide the anti-vs-semi asymmetry class of bug (cf. #24002), and empirically checked one of them. Sharing what I verified and two small suggestions. The high-level approach looks sound to me.
What I checked (all held up):
break 'stream_rowsafter the first match — the correctness of stopping the batch scan on the first match rests entirely on the sort invariant (first stream row yields the maximal matching buffered suffix, so later rows can only re-mark a subset). I built an adversarial single-batch, multi-row case (left.b1 > right.b1, buffered[2,4,6,8], streamed[1,3,5,7]) and confirmed it still emits all four expected rows. Correct — but the reasoning is subtle and load-bearing.- Empty / all-null streamed side for LeftAnti — the "emit all buffered rows" case (the failure mode from #24002, where filtering the probe side of an anti join wrongly creates output).
join_left_anti_empty_rightandjoin_left_anti_all_right_nullscover this and assert the full buffered set. Good. - Multi-partition final pass —
fetch_sub(1, SeqCst) == 1correctly gates the final emit to the last partition, matching the existing classic Left/Full coordination. - NULL join keys — never marked, so correctly excluded from Semi / included in Anti.
Two non-blocking suggestions:
- The
break 'stream_rowscomment explains what it does but not that its correctness depends on the streamed side being sorted in the same direction as the buffered side. Since a future change to the sort logic could silently break this, it'd help to state that invariant explicitly at thebreak. required_input_orderingdoesunimplemented!()for right-existence joins (exec.rs). That's a runtime panic guarded only by the planner gate (physical_planner.rs) not routing those types here — the two are far apart. When someone implements the RightSemi/RightAnti follow-up, the natural first step (opening the planner gate) would panic the optimizer if they forget this spot. Considernot_impl_err!here instead, so it degrades to an error rather than a panic.
This dovetails with @2010YOUY01's point about classic_join.rs now hosting semi/anti — if you do split the existence path into its own stream, suggestion (1)'s invariant and the naming confusion get resolved together.
For the RightSemi/RightAnti follow-up: the swap approach turns RightAnti into LeftAnti, which relocates the NULL-key handling into a new sort/operator-flip context that the current (all Left*) NULL tests don't exercise — worth dedicated null coverage there.
Scope caveat: this is a targeted correctness pass on the four points above, not a line-by-line audit or a differential (vs NestedLoopJoin) fuzz check — so treat it as "these specific high-risk areas look correct," not a full sign-off.
Which issue does this close?
Part of #17427 (Make
PiecewiseMergeJoinwork in DataFusion). AddsLeftSemi/LeftAntisupport, one of the epic's checklist items. Supersedes the stale #18392, taking the alternative approach that @2010YOUY01 suggested there (reuse the classic join path for generality) rather than a dedicated existence stream.Rationale for this change
An inequality-correlated
EXISTS/NOT EXISTS(e.g.WHERE EXISTS (SELECT 1 FROM r WHERE l.x < r.y)) has no equi-key, so it decorrelates to aLeftSemi/LeftAntijoin with a single range predicate. TodayPiecewiseMergeJoinExecrejects existence joins (not_impl_err!) and these queries fall back toNestedLoopJoinExec, which is O(n*m).Microbenchmark (20K × 20K rows, single inequality,
enable_piecewise_merge_joinon vs off), added in this PR as
piecewise_merge_join_semi_anti:What changes are included in this PR?
Existence joins (
LeftSemi/LeftAnti) forPiecewiseMergeJoin:LeftSemi/LeftAntiwith a single range predicate toPiecewiseMergeJoinExecin the physical planner (still gated behindenable_piecewise_merge_join, defaultfalse).LeftSemi= marked rows,LeftAnti= unmarked; NULL join keys are never marked, so they are correctly excluded from Semi and included in Anti). Only left-side columns are produced.RightSemi/RightAnti/Markremain unsupported (they require swapping the inputs); they are still rejected intry_newand excluded in the planner. Left as a follow-up.Are these changes tested?
Yes.
classic_join.rscoveringLeftSemi/LeftAntiacross<,<=,>,>=; NULL join keys; all-null streamed side; empty inputs;Date32keys; multi-batch and multi-partition streamed inputs; and the low-water-mark skip branch.pwmj.sltforEXISTS/NOT EXISTS(including NULLs) withEXPLAINassertions confirming the plan usesPiecewiseMergeJoin.Are there any user-facing changes?
No behaviour change by default:
enable_piecewise_merge_joinremainsfalse. When enabled, single-range-predicateLeftSemi/LeftAntijoins are planned asPiecewiseMergeJoininstead ofNestedLoopJoin. No API changes.