bench: add PWMJ vs NestedLoopJoin criterion coverage for LeftMark/Rig… - #24955
bench: add PWMJ vs NestedLoopJoin criterion coverage for LeftMark/Rig…#24955SubhamSinghal wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24955 +/- ##
==========================================
- Coverage 81.61% 81.61% -0.01%
==========================================
Files 1124 1124
Lines 411978 412077 +99
Branches 411978 412077 +99
==========================================
+ Hits 336236 336310 +74
- Misses 55936 55951 +15
- Partials 19806 19816 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @SubhamSinghal
Left few comments please take a look.
| // true vs false), even though the row count it returns cannot show that. | ||
| let pwmj_rows = run(pwmj_plan(), &ctx, &rt); | ||
| let nlj_rows = run(nlj_plan(), &ctx, &rt); | ||
| assert_eq!( |
There was a problem hiding this comment.
run returns only the number of rows, but RightMark always returns every row from the right input regardless of the match regime. Because LEFT_ROWS == RIGHT_ROWS, these assertions cannot even detect returning the wrong side. A PWMJ implementation that produces incorrect or uniformly false mark values would therefore pass these guards and could report misleading timings.
Please inspect the materialized mark column before timing. Ideally compare PWMJ and NLJ results per input row; at minimum, compare their true/false/null counts and assert the expected all-match, no-match, and half-match distributions.
| // `try_new` directly and skip the whole group rather than panic, so this benchmark stays | ||
| // runnable (as a no-op) against a build that has not merged `RightMark` support yet, and | ||
| // starts measuring on its own once that support lands. | ||
| if let Err(err) = PiecewiseMergeJoinExec::try_new( |
There was a problem hiding this comment.
This branch converts every try_new error into a successful benchmark skip. Once RightMark support lands, an unrelated schema, operator, or invariant regression would silently remove the benchmark while reporting that the build merely lacks support.
Please skip only the specific expected NotImplemented error and fail on every unexpected error. The compatibility skip can then be removed when the implementation lands.
| //! falls back to `NestedLoopJoinExec`, same as every other case here before its dependency | ||
| //! landed. | ||
| //! | ||
| //! `RightMark` has no SQL surface at all: no keyword parses to it, and no optimizer rule |
There was a problem hiding this comment.
There is no direct SQL syntax for RightMark, but the physical optimizer can construct one indirectly: JoinSelection swaps a statistics-selected nested-loop join, and NestedLoopJoinExec::swap_inputs maps LeftMark to RightMark through JoinType::swap.
Please narrow this to “no SQL syntax or logical optimizer rule emits RightMark directly” and describe this group as a deliberate direct-operator microbenchmark.
3 participants
Which issue does this PR close?
PiecewiseMergeJoinwork in Datafusion #17427.Rationale for this change
What changes are included in this PR?
datafusion/core/benches/pwmj_semi_anti_sql.rs's existing PWMJ-vs-NestedLoopJoinsweep with two more cases:Kind::LeftMark, planned from SQL via anEXISTSwrapped in an always-false disjunction (x > 100 OR EXISTS (...)), the one shape that decorrelates toLeftMark. It needs no special handling: like every other case in this file, thepwmj_enabledarm accepts eitherPiecewiseMergeJoinorNestedLoopJoinExec, since the planner itself falls back on a build that doesn't yet routeLeftMarkto PWMJ.bench_pwmj_right_mark_hand_builtbenchmark group forRightMark, which has no SQL surface at all (no keyword, no optimizer rule ever constructs it), so both arms are built directly instead of planned from SQL text. Since there is no planner to fall back through, it probesPiecewiseMergeJoinExec::try_newup front and skips the whole group with a printed note instead of panicking, on a build that does not supportRightMarkyet.What is the testing strategy for this PR?
This is a benchmark, not a behavior change, so there are no new correctness tests.
Are there any user-facing changes?
No. This adds a criterion benchmark only; no production code, public API, or behavior changes.