Repository navigation
fix: keep volatile semi joins in place during semi-join pushdown - #2793
Conversation
github-actions
Bot
added
the
documentation
9bf2fcb to
9a4fb91
Compare
| // An uncorrelated EXISTS keeps all or none of the rows; leave it. | ||
| return Ok(None); | ||
| } | ||
| match semi.left.as_ref() { |
There was a problem hiding this comment.
should semi.filter expression be checked for volatile expressions?
There was a problem hiding this comment.
Yes, good call. It now leaves the semi join in place when any key or the filter is volatile. Otherwise something like EXISTS (... WHERE s.x = a.v AND random() < 0.5) would draw one number per a row instead of one per joined row. Added keeps_semi_join_with_a_volatile_filter.
| .position(|l| l.contains(needle)) | ||
| .unwrap_or_else(|| panic!("{needle:?} not found in:\n{display}")) | ||
| }; | ||
| assert!( |
There was a problem hiding this comment.
could we also compare plans string as well, it helps with visibility later
There was a problem hiding this comment.
Done, the session test now compares the whole optimized plan.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Consider a small SQL test that compares the result with the rule on and off, using IN, EXISTS and NOT EXISTS over a join, with NULL keys on the anti side. (not sure if we will catch such discrepancies in TPCH test)
There was a problem hiding this comment.
Added semi_join_pushdown_preserves_results. It runs IN, EXISTS and NOT EXISTS over inner and left joins, with NULL keys on both the outer and subquery sides, with the rule on and off. It compares the rows and checks where the semi join ended up. I tested it by letting the rule push into a left join's null-supplying side, and the test catches the extra null-extended row.
milenkovicm
left a comment
milenkovicm
left a comment
There was a problem hiding this comment.
thanks @andygrove few minor comments
github-actions
Bot
removed
the
documentation
0612af5 to
1f2a6bf
Compare
andygrove
changed the title
Which issue does this PR close?
No single issue. A follow-up to #2799, which landed the
SemiJoinPushdownrule (ballista.optimizer.semi_join_pushdown) originally proposed here. Related: #2792, #2520 (performance review tracking).Rationale for this change
SemiJoinPushdownmoves a semi or anti join below inner joins on the assumption that its result depends only on the left row. That does not hold when a join key or the join filter is volatile. For example,EXISTS (... WHERE s.x = a.v AND random() < 0.5)pushed ontoawould draw one random number perarow instead of one per joined row, which changes which rows the query returns.The rule's existing tests also only checked plan shapes, not that results stay the same with the rule on and off.
What changes are included in this PR?
is_pushable_semi_joinleaves a semi or anti join in place when any of itsonkeys or its filter is volatile, with a unit test for the volatile filter case.semi_join_pushdown_preserves_results, runsIN,EXISTSandNOT EXISTSqueries over joins through every Ballista optimizer rule withsemi_join_pushdownon and off and checks the sorted results match. The tables have NULL keys on both the outer and subquery sides plus duplicate join matches. Each case also asserts whether the rule is expected to move the semi join, covering the cases where it must stay, such as an anti join that reads the null-supplying side or a correlated filter that reads both join sides.Are there any user-facing changes?
No new config. Queries whose semi or anti join has a volatile key or filter are no longer rewritten by
SemiJoinPushdown.