Repository navigation
fix: map pushed-down filter columns by position instead of by name - #187
Merged
LiaCastaneda merged 1 commit intoSep 29, 2026
Merged
Conversation
…pache#25259) - Closes apache#25244. - Closes apache#25262. - Closes apache#25263. - Closes apache#25264. - Closes apache#21246. - Closes apache#25296. Enabling `datafusion.optimizer.enable_join_dynamic_filter_pushdown` can silently drop matching rows when the probe side of a join contains several columns with the same name, for example `a.id` and `b.id` from a nested join. Physical filter pushdown resolved a pushed filter's columns in the child schema by name, so a predicate on the second `id` column was rewritten to the first `id` column, which holds a different value. The same name-based mapping also affects TopK dynamic filters under the default configuration (apache#25296). For a nested join of orders and payments that both expose `amount`, `ORDER BY p.amount LIMIT 1` can push the payment threshold onto `orders.amount`. Later matching orders are incorrectly pruned, returning `(100, 30)` instead of `(300, 10)`. Every operator that forwards parent filters had the same weakness, in slightly different forms: - `HashJoinExec` computed which output columns belong to each side by position, but then resolved the child column by name. - `FilterExec`, `SortExec`, `RepartitionExec`, `CoalesceBatchesExec`, `UnionExec` and similar nodes used the generic name lookup even though their output positions equal their input positions. - `FilterExec` with an embedded projection resolved unconsumed parent filters back into input coordinates by name. - `ProjectionExec` looked up output aliases by name to find the expression to substitute, so two outputs aliased `id` both mapped to the first. - `AggregateExec` restricted pushdown to grouping positions but resolved the input column by name. The join-filter regression queries return one row with dynamic filtering disabled and zero rows with it enabled across these shapes. The `FilterExec`, `ProjectionExec` and `AggregateExec` cases were found while auditing the remaining name-based paths for apache#25244; they share the root cause, so this PR fixes them together. The TopK case in apache#25296 is fixed by the same positional mapping. Built-in operators now remap filter columns by position. The deprecated public API retains its historical name-based resolution for compatibility. - `FilterRemapper` uses a `ColumnMapping` enum: `Identity` preserves positions and checks that names match; `Explicit` uses a caller-supplied parent-output to child-input mapping and allows names to differ. - `ChildFilterDescription::from_child` now maps by position. New `ChildFilterDescription::from_child_with_column_mapping` takes an explicit `HashMap<usize, usize>`. - `HashJoinExec` builds the explicit mapping from its `column_indices` and output projection. For semi joins, output join keys on the emitted side are mapped to the paired key on the other side, which also supports differently named keys. - `FilterExec` maps through its embedded projection in both pushdown phases and when folding unconsumed parent filters back into its predicate. - `ProjectionExec` substitutes the expression at each output position instead of looking the alias up in the output schema. - `AggregateExec` maps each grouping output position to the input column that grouping expression reads; non-column grouping expressions are not forwarded, as before. - `ChildFilterDescription::from_child_with_allowed_indices` is kept as a deprecated wrapper that preserves name-based resolution to the first matching child field. It translates allowed parent column references into an explicit positional mapping; new callers should supply positions directly to avoid ambiguous duplicate names. - `dynamic_filter_pushdown_config.slt` gains four regression queries over two small Parquet tables, run once with join dynamic filtering disabled and once enabled, asserting identical rows: nested joins with `RepartitionExec` between them (the query from apache#25244), a `FilterExec` with an embedded projection, a `ProjectionExec` with duplicate aliases, and an `AggregateExec` grouping on same-named columns. All four return zero rows on `main` with dynamic filtering enabled. - `dynamic_filter_pushdown_config.slt` also covers apache#25296 using nested joins of orders, payments and customers, with one row per batch and one row per orders Parquet row group. The query returns `(300, 10)` with TopK dynamic filtering disabled, enabled, and enabled together with Parquet filter pushdown. On `main` at `85d4cbb0a9`, both enabled cases reproduce the incorrect `(100, 30)` result; all three pass on this branch. - `datafusion/core/tests/physical_optimizer/filter_pushdown.rs` gains focused tests that call `gather_filters_for_pushdown` directly on `RepartitionExec`, `HashJoinExec` (duplicate child columns with and without projection, both semi join directions with differently named keys, and a semi join whose key is not a plain column), `FilterExec` with a projection in both phases, `ProjectionExec` with duplicate aliases, and `AggregateExec` with reordered same-named grouping columns. Further tests cover the deprecated `from_child_with_allowed_indices` wrapper preserving the first name match, accepting allowed parent indices outside the child schema, and rejecting unresolvable names, the identity mapping rejecting a column whose name differs from the child field at that position, and `FilterExec` rejecting an unconsumed parent filter outside its projection. Passed locally on the updated implementation: - `cargo fmt --all` - `./ci/scripts/doc_prettier_check.sh --write --allow-dirty` - `cargo test --profile ci -p datafusion --test core_integration physical_optimizer::filter_pushdown` — 70 tests passed. - `cargo test --profile ci --test sqllogictests -- dynamic_filter_pushdown_config.slt` — passed. - The isolated apache#25296 regression fails on `main` in both TopK-enabled configurations with the expected wrong-result mismatch. Queries that push join or TopK dynamic filters through operators with duplicate column names now return the correct rows, including the default-configuration TopK query in apache#25296. API change in `datafusion-physical-plan`: `ChildFilterDescription::from_child_with_allowed_indices` is deprecated in favour of `from_child` and the new `from_child_with_column_mapping`; the deprecated function preserves its previous name-based behavior. Callers should migrate to explicit positions because duplicate names make name resolution ambiguous. `ChildFilterDescription::from_child` also resolves by position, which only affects callers that used it on a node whose output positions differ from its child's. (cherry picked from commit b376290)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## branch-55 #187 +/- ##
==========================================
Coverage 81.18% 81.19%
==========================================
Files 1111 1111
Lines 387430 387468 +38
Branches 387430 387468 +38
==========================================
+ Hits 314546 314595 +49
+ Misses 54376 54369 -7
+ Partials 18508 18504 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Rich-T-kid
approved these changes
Sep 29, 2026
LiaCastaneda
deleted the
lia/cherry-pick-pr-25259-filter-pushdown-by-position
branch
September 29, 2026 09:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry picks apache#25259