Repository navigation
feat: push dynamic filters through nested loop joins - #25030
Conversation
github-actions
Bot
added
core
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25030 +/- ##
========================================
Coverage 81.94% 81.94%
========================================
Files 1135 1135
Lines 428278 428433 +155
Branches 428278 428433 +155
========================================
+ Hits 350952 351085 +133
- Misses 56369 56383 +14
- Partials 20957 20965 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The dynamic filter routing through NestedLoopJoinExec looks well scoped, and the tests do a good job covering the join-type routing and projection remapping. I just have one non-blocking suggestion for some additional integration coverage.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_nlj_dynamic_filter_pushdown_from_hashjoin() { |
There was a problem hiding this comment.
Could we add an end-to-end outer or mark NLJ case with unmatched rows? The unit test already covers the routing decisions for all join types, but the integration test only executes an inner NLJ. An unmatched-row case would give us some extra protection around the key safety invariant here: dynamic filters should never be routed to a non-preserved input.
There was a problem hiding this comment.
makes sense, have added this. let me know if that looks fine
There was a problem hiding this comment.
@yashrb24, thanks for the follow-up changes. I went through the updates and the earlier review comments look addressed.
The new end-to-end LEFT NLJ test covers both matching and unmatched rows, and confirms that enabling dynamic-filter routing preserves the results while keeping the filter off the non-preserved right input. This satisfies the request for outer or mark join coverage.
I also checked the NLJ child remapping changes. Using explicit output-to-child column mappings avoids the ambiguity from name-based remapping, and the duplicate-name and reordered-projection tests provide good coverage for those cases.
Looks good to me. Thanks for tightening this up!
|
🚀 |
## Which issue does this PR close? Part of apache#16973. ## Rationale for this change Dynamic filters from a hash join stop at a nested loop join. Scans below it may read rows that cannot match. ## What changes are included in this PR? Allow filters to pass through a nested loop join when all their columns belong to one input and filtering preserves the results. Filters using both inputs remain unchanged (ideally will raise a separate PR for this) ## What is the testing strategy for this PR? Tests cover join types, reordered columns, duplicate names, and filter updates. ## Are there any user-facing changes? Some queries can filter rows earlier. No new settings or API changes.
4 participants
Which issue does this PR close?
Part of #16973.
Rationale for this change
Dynamic filters from a hash join stop at a nested loop join. Scans below it may read rows that cannot match.
What changes are included in this PR?
Allow filters to pass through a nested loop join when all their columns belong to one input and filtering preserves the results. Filters using both inputs remain unchanged (ideally will raise a separate PR for this)
What is the testing strategy for this PR?
Tests cover join types, reordered columns, duplicate names, and filter updates.
Are there any user-facing changes?
Some queries can filter rows earlier. No new settings or API changes.