bench: pwmj left semi/anti join - #24160
Conversation
github-actions
Bot
added
sqllogictest
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24160 +/- ##
==========================================
+ Coverage 81.02% 81.05% +0.02%
==========================================
Files 1106 1107 +1
Lines 380718 381555 +837
Branches 380718 381555 +837
==========================================
+ Hits 308489 309269 +780
- Misses 54001 54024 +23
- Partials 18228 18262 +34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@kumarUjjawal build successful. PTAL |
| BenchmarkId::new(format!("pwmj_{jt}_{regime}"), right_rows), | ||
| |b| { | ||
| b.iter(|| { | ||
| let (left, right) = build_inputs(); |
There was a problem hiding this comment.
build_inputs() is timed, including generating 40,000 values and allocating arrays and batches. This can dominate the ~0.5 ms PWMJ result. Could we prebuild the batches and use iter_batched for fresh plans outside the timed section?
| // Selectivity is set by how far the right key range sits above the left range. | ||
| // - "high": right keys mostly above left keys -> most left rows match (Semi large) | ||
| // - "low": right keys mostly below left keys -> few left rows match (Anti large) | ||
| let regimes: [(&str, i32); 2] = [("sel_high", key_span), ("sel_low", -key_span)]; |
There was a problem hiding this comment.
These offsets produce exactly all-match and no-match cases, not “mostly” and “few.” Could we label them as all/none and add an overlapping-range case to measure partial suffix marking?
| Arc::new(Column::new("key", 0)), | ||
| ); | ||
| Arc::new( | ||
| PiecewiseMergeJoinExec::try_new( |
There was a problem hiding this comment.
Could we benchmark this through the SQL/physical planner instead of constructing PiecewiseMergeJoinExec directly? The current benchmark will panics on because existence joins are unsupported until #23870.
github-actions
Bot
added
core
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Hi @SubhamSinghal Thanks for iterating.
This became a little tricky that's why we had to do more follow up. If you have any ideas you can share as-well.
| ] { | ||
| let ctx = create_context(right_offset, pwmj, &s); | ||
| let name = format!("{arm}_{label}_{regime}"); | ||
| assert_plan_contains( |
There was a problem hiding this comment.
This still panics on this PR’s current head because enabling PWMJ selects NestedLoopJoinExec until #23870, while this assertion requires PiecewiseMergeJoin. To use this revision as the pre-change benchmark baseline, could we name this arm pwmj_enabled and accept either NLJ or PWMJ, while keeping the disabled arm pinned to NLJ? #23870’s tests can assert that the enabled plan switches to PWMJ.
3 participants
related: https://github.com/apache/datafusion/pull/23870/changes