feat: extend SLT tests for hash join vs PWMJ, hash join vs SMJ - #24805
feat: extend SLT tests for hash join vs PWMJ, hash join vs SMJ#24805comphead wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24805 +/- ##
==========================================
+ Coverage 81.50% 81.52% +0.02%
==========================================
Files 1123 1123
Lines 404760 406148 +1388
Branches 404760 406148 +1388
==========================================
+ Hits 329896 331120 +1224
- Misses 55552 55660 +108
- Partials 19312 19368 +56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
viirya
left a comment
There was a problem hiding this comment.
Nice to see configMatrix from #24493 get its first real use — this is exactly the kind of equivalence checking that's been done ad hoc until now, and the two files read well.
I ran both files on top of current main and they pass. I also checked that the matrix isn't accidentally vacuous, since that's the usual failure mode for this shape of test — with these small fixtures I half expected the SMJ side to collapse into CollectLeft HashJoin on both arms. It doesn't: prefer_hash_join=false really does plan SortMergeJoinExec and true plans HashJoinExec, and on the PWMJ side enable_piecewise_merge_join flips between PiecewiseMergeJoin (both Inner and LeftSemi) and NestedLoopJoinExec. So the differential is real on both files.
What I liked most is that the cases are written against the implementation rather than being generic smoke tests. The ej_dup_l.v = {5,5,3,3,1} fixture with a deciding streamed key of 3, and the comment about binary search needing to return the first index of a run of equal keys, lands right on the boundary that the suffix-watermark encoding from #24579 depends on. Same for splitting all-NULL buffered from all-NULL streamed, and for sweeping batch_size=1. The "Rules for a matrix file" note in both headers is a good idea too — it should stop someone quietly breaking the matrix later by adding an EXPLAIN or an in-file SET.
Two things, one substantive and one a nit.
Mark joins in the SMJ file. The file covers Inner/Left/Right/Full and LeftSemi/LeftAnti/RightSemi/RightAnti, but I don't see a mark join. Was that a deliberate scope decision? Asking because SMJ does seem to implement LeftMark/RightMark (there's dedicated handling in sort_merge_join/bitwise_stream.rs), the existing sort_merge_join.slt does cover mark joins, and when I tried ... WHERE l.k = 3 OR EXISTS (SELECT 1 FROM smj_r r WHERE r.k = l.k) under this file's own settings it planned as SortMergeJoinExec: join_type=LeftMark. If that's right, mark joins would be the one existence type not getting the batch-size sweep here — and being the type that emits one row per build row with a boolean, it's arguably the one where batch boundaries are most interesting. Would you consider adding it, or is there a reason it's better left in sort_merge_join.slt?
The RightSemi claim in the PWMJ header. The header says RightSemi/RightAnti/Mark "stay on NestedLoopJoin in both combinations". That's true of the queries in the file — I checked, the explicit RIGHT SEMI JOIN / RIGHT ANTI JOIN shapes do stay on NLJ, so the tests and expected values are fine, and the "(explicit syntax)" qualifier is doing real work. But the same semantics written as a correlated EXISTS gets its correlation flipped by the planner and came out as PiecewiseMergeJoin ... join_type=LeftSemi for me. Since the sentence reads like a property of the join type rather than of the SQL shape, could it be worth making that distinction explicit? Otherwise someone may later reason from that line and conclude a RightSemi-flavoured query can't reach PWMJ.
Neither of these blocks merging from my side — the file passes and the coverage it adds is real.
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @comphead
This is nice! I left one comment for your consideration.
| # ------------------------------------------------------------------ | ||
| # Multi-batch stress (verified by count) | ||
| # ------------------------------------------------------------------ | ||
| # Larger inputs so batch_size=1 and 2 split each side into many batches, driving |
There was a problem hiding this comment.
batch_size=1 does not split this VALUES input into one-row batches. This matrix does not cover the stated per-batch extreme-key path or the cross-batch watermark path. Can we build the streamed fixture from a source that emits several batches, such as generate_series and add a plan or metric assertion.
Which issue does this PR close?
Rationale for this change
PiecewiseMergeJoinExecandSortMergeJoinExecmust return the same results asthe mature join implementations they can be swapped for, across every batch
boundary. Today that equivalence is checked ad hoc. Using the
# configMatrix:directive from #24493, one
.sltfile can assert it directly: run the samequeries once per join implementation and once per batch size, and require
identical output.
What changes are included in this PR?
What is the testing strategy for this PR?
Are there any user-facing changes?