feat: transfer parent and dynamic filters across HashJoinExec equi-join keys (inner and semi joins) - #25255
feat: transfer parent and dynamic filters across HashJoinExec equi-join keys (inner and semi joins)#25255jayzhan211 wants to merge 2 commits into
Conversation
…inner and semi joins
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25255 +/- ##
==========================================
- Coverage 81.95% 81.91% -0.04%
==========================================
Files 1133 1134 +1
Lines 423828 425683 +1855
Branches 423828 425683 +1855
==========================================
+ Hits 347344 348694 +1350
- Misses 55890 56304 +414
- Partials 20594 20685 +91 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adriangb
left a comment
There was a problem hiding this comment.
I reviewed the code, the tests and the benchmark data. I also ran a differential test and mutation tests against the new code. This review uses Simplified Technical English.
Summary
The transfer logic is correct. I did not find a regression.
- For
Inner,LeftSemiandRightSemijoins, each output row has equal keys on the two sides. Thus a filter over one key gives the same result over the other key. Theif_anypropagation stays exact, also when only the transferred side accepts the filter. with_new_childrenkeeps the originalchildrenand only replacesremapped_children. Thus the transfer composes with a projection remap that occurred before it.HashExpr,HashTableLookupExprandRangeExprbuild again throughwith_new_children. Thus theCASE hash(keys) % nrouting filter stays correct on the other side.- The proto encoder accepts remapped children that are not a
Column. - The optimizer removes volatile predicates before
gather_filters_for_pushdownruns. Thus the transfer cannot copy a volatile predicate.
Tests that I ran
cargo fmt,cargo clippy -D warnings, thefilter_pushdownintegration tests, thehash_joinunit tests, the proto round-trip tests and the full sqllogictest suite pass.- A differential harness ran 67 curated queries and 300 random queries in 12 configurations. The configurations toggle dynamic filters, parquet row-level pushdown,
CollectLeftand forcedPartitionedmode, bounds-only filters, 1 and 4 partitions, and the logicalpush_down_filterrule. The results were the same in all configurations. - The queries include CAST keys,
IS NOT DISTINCT FROM, NULL keys, empty build sides, outer, anti and mark joins, projections that remove or duplicate keys, four-level nesting, recursive CTEs and TopK filters over joins.
Findings
- This PR corrects a wrong-result bug on
main, but the description does not say so. The removed semi-join code pushed a key filter to the other side by column name. See the inline comment on the semi-join test. Please add a regression test and a note in the description. - Mutation tests show branches that no test covers: the
Jumprecursion control, theRightSemibranch, thePushedDown::Yesguard, theany_columncondition, theJoinSide::Nonearm and the first-pair tie-break. See the inline comments. TheJumpcase is the important one, because it prevents an infinite loop. lr_is_preserveddoes not gate the transfer. See the inline comment.
Related bug that this PR does not cause
FilterRemapper::try_remap maps a column to the first field with the same name. When a child schema has two fields with the same name, a filter over the second field moves to the first field. This gives wrong results with the default configuration. Example: a TopK filter over a non-key column with a duplicated name, above two joins where the planner swaps the lower join. This PR removes one caller of that remap and does not make the bug worse. This bug needs a separate issue with a reproducer.
| // non-output side. | ||
| if Self::supports_key_transfer(self.join_type) { | ||
| let (to_right, to_left) = self.key_transfer_maps(&column_indices); | ||
| transfer_key_filters(&parent_filters, &to_right, &mut right_child)?; |
There was a problem hiding this comment.
transfer_key_filters writes into the child descriptions after the lr_is_preserved gate. It also overwrites all_unsupported entries.
For the three permitted join types, both sides are preserved. Thus the gate has no effect today. If a future change adds a join type to supports_key_transfer that is not preserved on one side, the transfer pushes filters to that side without a warning. A mutant that returns (true, false) for semi joins passes all tests.
Please transfer only into a side that lr_is_preserved permits, or add an assertion that couples the two functions.
| return Ok(()); | ||
| } | ||
| for (filter, pushed) in parent_filters.iter().zip(child.parent_filters.iter_mut()) { | ||
| if matches!(pushed.discriminant, PushedDown::Yes) { |
There was a problem hiding this comment.
This guard cannot trigger. to_right contains only left-side output indices, and to_left contains only right-side output indices. A filter is directly pushable to a child only when all its columns are on that side. Thus a filter cannot be both directly pushable and transferable to the same child.
A mutant that removes the guard passes all tests. Please remove the guard, or add a comment that says it is a safety check.
| key_map: &KeyTransferMap, | ||
| ) -> Result<Option<Arc<dyn PhysicalExpr>>> { | ||
| let mut all_keys = true; | ||
| let mut any_column = false; |
There was a problem hiding this comment.
The any_column condition is redundant. try_remap already marks a filter with no columns as supported for both children, as the comment above says.
A mutant that removes this condition passes all tests.
| JoinType::Right => (false, true), | ||
| JoinType::Full => (false, false), | ||
| // Callers restrict the non-output side of semi joins to join-key columns. | ||
| // The non-output side of a semi join only receives filters transferred |
There was a problem hiding this comment.
This comment describes a rule that the code does not enforce. See my comment on the transfer_key_filters calls above.
…transfers; gate transfer on lr_is_preserved
|
Thanks @adriangb for the detailed review and helpful suggestions! I've addressed everything. |
|
Thanks @jayzhan211 ! I'll let @kosiew review this but from my perspective this is g2g. Nice change! |
Which issue does this PR close?
No dedicated issue yet. Related: #18290, #19858, #7955 (dynamic filter pushdown follow-ups).
Rationale for this change
What "transferring a filter across join keys" means
For an inner or semi join
ON a.k = b.k, every output row hasa.k = b.k. So a filter that only touchesa.k(saya.k IN (1, 3)) is also true ofb.kfor every row that can match. TodayHashJoinExecroutes a parent filter only to the side that owns its columns, so that filter reachesaand neverb. With this PR the join also pushesb.k IN (1, 3)tob.Static predicates already get this at the logical level (equality inference in
push_down_filter). Dynamic filters do not exist there, and they are the ones that matter: the dynamic filter of a join above is a parent filter for the joins below it.Before, with
dim(small, filtered) joined tomid, andmidjoined tofact:After:
The transferred copy is the same
DynamicFilterPhysicalExprwith its key columns remapped, so it is populated whendimfinishes building and prunesfactat the scan (file / row-group / page pruning always, row-level withpushdown_filters=true). Becausefactis now pruned before the lower join builds, the lower join's own filter tightens too.Why the lower join's own filter is not enough on its own:
hash_join_inlist_pushdown_max_size/_max_distinct_values, which cannot prune files, row groups or pages, while the transferred filter carries the small table's min/max bounds and IN list;preserve_file_partitions, null-aware joins with nullable build keys, a build-side scan that does not accept filters);customerhad no dynamic filter at all before).Semi-join routing bug fixed on the way
The same mechanism replaces the semi-join special case in
gather_filters_for_pushdown. That code added the key's output index to the non-output side's allowed set and then letFilterRemapper::try_remapmap the column to that side by name. Two failure modes followed when the keys were named differently:FilterExecabove the join is removed. That is a wrong-result bug. Example:LeftSemionleft.k = right.jwhererightalso has a non-key columnk. A parent filterk = 'x'reached the right scan ask@2 = 'x'; with this PR it isj@0 = 'x'. TheRightSemibranch had the mirror-image bug.The existing
test_hashjoin_parent_filter_pushdown_semi_anti_joindid not catch either, because both keys in it are namedk. The transfer rewrites over the key expression on the other side, so column names no longer matter.Benchmarks
TL;DR: JOB total 3 % faster with row-level parquet pushdown (16a 2.3x, 16c/16d 1.6x, 33a/33c 1.25x), TPC-H SF10 Q5 1.3x, everything else within noise, no reproducible regression. Details below.
M4 Pro (12 cores / 24 GB), release binaries built in separate target dirs, sides alternated per round, per-query minimum over all iterations.
default= stock parquet config (dynamic filters prune files / row groups / pages only);pushdown=datafusion.execution.parquet.pushdown_filters=true.Queries outside +/-5 % (pushdown mode, ms):
Where the time goes, from per-scan
EXPLAIN ANALYZEmetrics in pushdown mode.JOB 16a: the top join builds on the filtered
title(68 K rows) and its dynamic filter lands onci.movie_id, the build-side key of the join below. Transferred across that join's keys it now also reaches two scans:TPC-H SF10 Q5: the 5-row
nationfilter reachescustomerthrough the key equivalence, which previously received no dynamic filter, and the pruning cascades down the join chain:Cost side: the transferred copy is rewritten over the target side's key expression, so where the join key is a
CASTit pays a cast per row. The bounds builder also emits duplicated bound pairs when several key pairs share one column (pre-existing, visible in the before plans too, just more often now).What changes are included in this PR?
HashJoinExec::gather_filters_for_pushdown: after the plain column-based routing, every parent filter whose columns are all plainColumnjoin keys of one side is rewritten over the other side's key expressions and marked supported for that child. Inner, LeftSemi and RightSemi only: outer, anti and mark joins also emit unmatched rows, so the transferred filter would not be exact there andif_anycould wrongly drop the parent filter. Outer joins would need "prune-only" semantics and are left as a follow-up.DynamicFilterPhysicalExpris rewritten throughwith_new_children, so the transferred copy shares the original's state and keeps tracking the build side.lr_is_preservedpermits, the same gate as the plain column routing (a no-op for the three permitted join types today, but it couples the two functions). Join keys aredebug_asserted to have equal data types, since the planner coerces them andtry_newdoes not check.What is the testing strategy for this PR?
test_hashjoin_parent_filter_transferred_across_join_keys: key names differ; key filters land on both scans, a non-key filter stays on its side.test_hashjoin_parent_filter_transfer_semi_join_different_key_namesand itsRightSemivariant: the case the old name-based routing missed (nothing reached the non-output scan).test_hashjoin_parent_filter_transfer_semi_join_key_name_shadowed_by_non_key: regression test for the wrong-column pushdown,LeftSemiandRightSemi.test_hashjoin_parent_filter_transfer_uses_first_on_pair:ON k = x AND k = ytransfers over the first pair.test_hashjoin_parent_filter_transfer_cast_key_with_projection: aCASTkey with a projection that puts the other side's key at the same output index as the cast's inner column. The rewrite must not descend into the substituted expression, or it would loop forever.test_hashjoin_dynamic_filter_transferred_through_nested_join: an upper join's dynamic filter reaches the lower probe scan. The lower build scan rejects filters, so the lower join's own filter still lists all four keys and the two pruned rows are attributable to the transfer alone (checked via scan metrics).join_dynamic_filter_transfer.slt: SQL plan shape and results.cargo fmtandclippy -D warningspass.Are there any user-facing changes?
No new configuration.
EXPLAINmay now show a parent or dynamic filter on both scans of an inner or semi join where it previously appeared on one.