Skip to content

feat: extend SLT tests for hash join vs PWMJ, hash join vs SMJ - #24805

Open
comphead wants to merge 1 commit into
apache:mainfrom
comphead:join_combined_test
Open

feat: extend SLT tests for hash join vs PWMJ, hash join vs SMJ#24805
comphead wants to merge 1 commit into
apache:mainfrom
comphead:join_combined_test

Conversation

@comphead

@comphead comphead commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

PiecewiseMergeJoinExec and SortMergeJoinExec must return the same results as
the 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 .slt file can assert it directly: run the same
queries 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?

@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 30, 2026
@comphead comphead changed the title feat: add combined join tests hash vs PWMJ, hash vs SMJ feat: extend SLT tests for hash join vs PWMJ, hash join vs SMJ Aug 30, 2026
@comphead
comphead requested a review from viirya August 30, 2026 19:57
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.52%. Comparing base (791b32e) to head (4b9ff12).
⚠️ Report is 19 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@viirya viirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create a combined correctness join SLT tests

4 participants