adriangb opened a new pull request, #25455: URL: https://github.com/apache/datafusion/pull/25455
## Which issue does this PR close? - Closes https://github.com/apache/datafusion/issues/14540. ## Rationale for this change `PushDownFilter` and `PushDownLeafProjections` both move nodes towards the leaves. For an adjacent filter and *pure extraction projection* they want the opposite order. A pure extraction projection is the node `ExtractLeafExpressions` creates: its expressions are only `__datafusion_extracted_N` aliases and pass-through columns. ```text Filter: t1.date = Date32("2025-01-03") <-- (A) Projection: get_field(t1.ids, "id1") AS __datafusion_extracted_1, t1.date <-- (B) TableScan: t1 ``` `push_down_filter` puts (A) below (B). `push_down_leaf_projections` puts (B) below (A). On `main` both rules report a change in every optimizer pass, and the rule that runs later in the rule list decides the plan. Nobody had picked a side. The last comment on the issue asks which rule must yield. ### Experiment Three variants were measured with `datafusion-cli`, on the query from the issue and on the simpler `SELECT ids['id1'] FROM t1 WHERE date = '2025-01-03'`, against a memory table and a Parquet file, with `datafusion.execution.parquet.pushdown_filters` set to `false` (the default) and to `true`: - **(a) main**: the extraction projection ends below the filter, because `push_down_leaf_projections` runs last. - **(b) leaf rule yields**: `PushDownLeafProjections` does not move a pure extraction projection through a filter whose predicate does not reference the extracted aliases. The filter ends below the extraction projection. - **(c) filter rule yields**: `PushDownFilter` treats a pure extraction projection as non-pushable. The filter stays above it, and the leaf rule has nothing to undo. Two properties must both survive: the struct leaf must reach `DataSourceExec` (`projection=[get_field(...) ...]`), and the `date` predicate must reach `DataSourceExec` (`predicate=` plus `pruning_predicate=`). | variant | query | source | `pushdown_filters` | leaf in scan | filter in scan | passes | | --- | --- | --- | --- | --- | --- | --- | | (a) main | simple | Parquet | false | yes | yes | 2 | | (a) main | simple | Parquet | true | yes | yes | 2 | | (a) main | simple | memory | n/a | n/a | n/a | 2 | | (a) main | #14540 | Parquet | false | yes | yes | 3 | | (a) main | #14540 | Parquet | true | yes | yes | 3 | | (a) main | #14540 | memory | n/a | n/a | n/a | 3 | | (b) leaf yields | simple | Parquet | false | **no** (`projection=[date, ids]`) | yes | 2 | | (b) leaf yields | simple | Parquet | true | yes | yes | 2 | | (b) leaf yields | simple | memory | n/a | n/a | n/a | 2 | | (b) leaf yields | #14540 | Parquet | false | **no** (`projection=[date, timestamp, ids, structs]`) | yes | 2 | | (b) leaf yields | #14540 | Parquet | true | yes | yes | 2 | | (b) leaf yields | #14540 | memory | n/a | n/a | n/a | 2 | | (c) filter yields | simple | Parquet | false | yes | yes | 2 | | (c) filter yields | simple | Parquet | true | yes | yes | 2 | | (c) filter yields | simple | memory | n/a | n/a | n/a | 2 | | (c) filter yields | #14540 | Parquet | false | yes | yes | 3 | | (c) filter yields | #14540 | Parquet | true | yes | yes | 3 | | (c) filter yields | #14540 | memory | n/a | n/a | n/a | 3 | "passes" counts the invocations of `push_down_filter`, that is the number of optimizer passes the plan needs. ### Decision **`PushDownFilter` yields. Pure extraction projections win.** Variant (c). Variant (b) fails the goal. At the default `pushdown_filters = false` the `get_field` no longer reaches `DataSourceExec`, so the scan reads the whole struct. The physical `ProjectionPushdown` rule cannot recover it. `FilterExec::try_swapping_with_projection` rewrites the predicate against the projection output, and the predicate needs the `date` column, which the projection does not produce. The `ProjectionExec` therefore stays above the `FilterExec` and never reaches the scan. Only `pushdown_filters = true` recovers it, and that is not the default. Variants (a) and (c) keep both properties everywhere, so the tie breaks on the remaining differences. Variant (c) wins on three of them: 1. The plan is simpler. Under (a) the #14540 query keeps two `Filter` nodes and an extra pass-through `Projection` between them. Under (c) the two filters merge into one node and the pass-through projection is gone. 2. The plan is a verified fixed point. Under (a) the last optimizer pass still changes the plan, so the plan that survives is the one the last rule left, not a stable one. Under (c) the last pass changes nothing. 3. The cheap conjunct runs first. Under (c) the merged filter evaluates `date = '2025-01-03'` before the four `get_field` comparisons. The filter loses nothing by staying one node higher. `PushDownFilter` runs before `ExtractLeafExpressions` in the rule list, so it records the predicate in `TableScan::filters` in the first pass, before any extraction projection exists. Row group pruning and source level filtering are unaffected, which the Parquet rows of the table show. On a source that cannot absorb the projection, such as a memory table, (c) is also the better order, because the filter runs before the `get_field`. ## What changes are included in this PR? - `rewrite_projection` in `push_down_filter.rs` returns the filter unchanged when the projection is a pure extraction projection. This is the whole behaviour change. - `is_pure_extraction_projection` in `extract_leaf_expressions.rs` is now `pub(crate)` and takes the expression list, so both rules use one predicate. - The precedence is recorded as an invariant in the module documentation of both rules, and in a new "Rule Precedence" section in `docs/source/library-user-guide/query-optimizer.md`. No new public API and no new configuration option. ## What is the testing strategy for this PR? - Two unit tests in `push_down_filter.rs`: `filter_not_pushed_through_pure_extraction_projection` shows the pair does not move, and `filter_pushed_through_mixed_extraction_projection` shows a projection that also computes an expression is still pushed through. - A new section in `datafusion/sqllogictest/test_files/projection_pushdown.slt`: - the #14540 query on a memory table, with its final `EXPLAIN` plan; - a determinism check. The simple shape gives the same plan at `datafusion.optimizer.max_passes = 1` as at the default. The #14540 shape gives the same plan at `max_passes = 2` as at the default. Two passes are needed there because the two filters merge in the second pass, which is real work and not a rule fight; - a correctness query over the same data; - a Parquet backed check, where `DataSourceExec` shows both the `get_field` leaf projection and the `date` predicate. Commands run: - `cargo test --profile ci -p datafusion-optimizer` -> 882 passed, 0 failed (lib), 26 passed (integration), 5 passed / 1 ignored (doctests). - `cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests` -> 520 of 520 files pass. - `cargo clippy --profile ci -p datafusion-optimizer --all-targets -- -D warnings` -> clean. - `RUSTDOCFLAGS="-D warnings" cargo doc --profile ci -p datafusion-optimizer --no-deps` -> clean. - The extended workspace test command (`--workspace --lib --tests --bins` with `avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`, without `datafusion-examples`, `datafusion-benchmarks`, `datafusion-cli` and `datafusion-sqllogictest`) -> 68 suites, 11910 passed, 0 failed, 8 ignored. ### Changed expectations Two lines in `projection_pushdown.slt` change. Both are in queries that the fight used to rewrite. 1. Line 1058, `EXPLAIN SELECT s['value'] * 2 + length(s['label']) as score FROM simple_struct WHERE id > 1;`. The plan shape does not change. The alias inside the cast is no longer stripped: `CAST(character_length(...) AS Int64)` becomes `CAST(character_length(...) AS length(get_field(simple_struct.s, Utf8("label"))) AS Int64)`. The SQL planner writes `length(x)` as `character_length(x) AS length(x)`, and the coercion pass wraps that alias in the cast. On `main` this alias is visible too, with no struct and no extraction: `EXPLAIN SELECT a * 2 + length(b) AS score FROM tt;` prints `CAST(character_length(tt.b) AS length(tt.b) AS Int64)`. It only disappeared in this test because the extra projection merge that the rule fight caused removed it. The physical plan is identical. 2. Line 2308, a `TableScan` loses a `Boolean(true)` entry from `partial_filters`. The entry was a no-op. ## Are there any user-facing changes? The logical plan of a query that reads a struct field and filters on another column changes shape. The filter node now sits above the extraction projection instead of below it. Results do not change, and the physical plan keeps both the leaf projection and the scan predicate. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
