fix: remap SortMergeJoin filter during projection pushdown - #24415
fix: remap SortMergeJoin filter during projection pushdown#24415haohuaijin wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24415 +/- ##
==========================================
+ Coverage 81.38% 81.39% +0.01%
==========================================
Files 1116 1116
Lines 397960 397973 +13
Branches 397960 397973 +13
==========================================
+ Hits 323880 323935 +55
+ Misses 55120 55075 -45
- Partials 18960 18963 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The fix looks good to me. Remapping the join filter alongside the join keys addresses the stale column index issue, and the added unit and SQL regression coverage looks solid.
I left one optional suggestion for extending the SQL regression coverage.
| statement ok | ||
| set datafusion.optimizer.repartition_joins = true; | ||
|
|
||
| query TT |
There was a problem hiding this comment.
Could we also add a LEFT or FULL join case with an unmatched row and a join filter that references projected-away columns? The current regression test covers the inner join case well. An outer join case would also exercise the remapping when null-extended rows are involved. This is just a suggestion and does not need to block this PR.
Which issue does this PR close?
SortMergeJoinprojection pushdown can reuse stale JoinFilter indices #24414.Rationale for this change
Projection pushdown through
SortMergeJoinExeccould reuse stale join-filter indices and return incorrect results.What changes are included in this PR?
JoinFilterduring projection pushdown.Are these changes tested?
Yes. The SQLLogicTest verifies the physical plan and the expected
(1, 1)result. The full Rust lint and extended workspace test suites also pass.Are there any user-facing changes?
Affected queries now return correct results. There are no API changes.