Skip to content

fix: remap SortMergeJoin filter during projection pushdown - #24415

Open
haohuaijin wants to merge 8 commits into
apache:mainfrom
haohuaijin:codex/fix-sort-merge-join-filter-projection
Open

fix: remap SortMergeJoin filter during projection pushdown#24415
haohuaijin wants to merge 8 commits into
apache:mainfrom
haohuaijin:codex/fix-sort-merge-join-filter-projection

Conversation

@haohuaijin

@haohuaijin haohuaijin commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Projection pushdown through SortMergeJoinExec could reuse stale join-filter indices and return incorrect results.

What changes are included in this PR?

  • Rewrite the JoinFilter during projection pushdown.
  • Reject the swap when a filter column cannot be mapped.
  • Add an SQLLogicTest for the incorrect-result case.

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.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Aug 16, 2026
@codecov-commenter

codecov-commenter commented Aug 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.39%. Comparing base (5134a1a) to head (14a2891).

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.
📢 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.

@kosiew kosiew 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.

@haohuaijin,

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

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added in b5c7a48

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

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SortMergeJoin projection pushdown can reuse stale JoinFilter indices

3 participants