You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/09/18 02:08:00 UTC
[PR] fix: decide the precedence between PushDownFilter and PushDownLeafProjections [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: decide the precedence between PushDownFilter and PushDownLeafProjections [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #25455:
URL: https://github.com/apache/datafusion/pull/25455#issuecomment-6017720587
Thanks @kosiew, added the tests in [406e428](https://github.com/apache/datafusion/pull/25455/commits/406e428657115bbd268bf1ed13bd746a3080835e).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: decide the precedence between PushDownFilter and PushDownLeafProjections [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #25455:
URL: https://github.com/apache/datafusion/pull/25455
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: decide the precedence between PushDownFilter and PushDownLeafProjections [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #25455:
URL: https://github.com/apache/datafusion/pull/25455#discussion_r4194939366
##########
docs/source/library-user-guide/query-optimizer.md:
##########
@@ -162,6 +162,16 @@ individual operators or expressions.
Sometimes there is an initial pass that visits the plan and builds state that is used in a second pass that performs
the actual optimization. This approach is used in projection push down and filter push down.
+### Rule Precedence
+
+Two rules can want the opposite order for the same pair of adjacent plan nodes. Each rule then undoes the work of the other one on every optimizer pass. The loop stops at `datafusion.optimizer.max_passes`, and the position of the two rules in the rule list decides the plan. This wastes a pass and makes the plan depend on the rule list.
+
+Do not let two rules compete. Give one rule precedence, make that rule yield, and record the decision in the module documentation of both rules.
Review Comment:
I think this should say "Give one rule precedence; make the competing rule yield," since the rule with precedence is the one that wins. Also, the optimizer can stop early when an end-of-pass plan signature repeats, so please avoid saying that the loop necessarily runs until `max_passes`.
##########
datafusion/sqllogictest/test_files/projection_pushdown.slt:
##########
@@ -2331,12 +2331,232 @@ logical_plan
05)--------SubqueryAlias: inner_t
06)----------Projection: simple_struct.id, simple_struct.s, __datafusion_extracted_1
07)------------Limit: skip=0, fetch=1
-08)--------------Filter: __datafusion_extracted_2 > Int64(120) AND __datafusion_extracted_1 != Utf8("delta")
-09)----------------Filter: simple_struct.id = outer_ref(outer_t.id)
-10)------------------Projection: get_field(simple_struct.s, Utf8("value")) AS __datafusion_extracted_2, get_field(simple_struct.s, Utf8("label")) AS __datafusion_extracted_1, simple_struct.id, simple_struct.s
-11)--------------------TableScan: simple_struct, partial_filters=[simple_struct.id = outer_ref(outer_t.id), get_field(simple_struct.s, Utf8("value")) > Int64(120)]
-12)--SubqueryAlias: outer_t
-13)----TableScan: simple_struct projection=[id]
+08)--------------Filter: simple_struct.id = outer_ref(outer_t.id) AND __datafusion_extracted_2 > Int64(120) AND __datafusion_extracted_1 != Utf8("delta")
+09)----------------Projection: get_field(simple_struct.s, Utf8("value")) AS __datafusion_extracted_2, simple_struct.id, simple_struct.s, get_field(simple_struct.s, Utf8("label")) AS __datafusion_extracted_1
+10)------------------TableScan: simple_struct, partial_filters=[simple_struct.id = outer_ref(outer_t.id), get_field(simple_struct.s, Utf8("value")) > Int64(120)]
+11)--SubqueryAlias: outer_t
+12)----TableScan: simple_struct projection=[id]
statement ok
set datafusion.explain.logical_plan_only = false;
+
+#####################
+# Section 17: Precedence between PushDownFilter and PushDownLeafProjections
+#
+# Both rules move nodes towards the leaves, and for an adjacent filter and
+# "pure extraction projection" they want the opposite order. The precedence is:
+# the extraction projection wins. It stays next to the scan, so the scan can
+# absorb it and read only the struct leaf, and the filter stays above it.
+#
+# See https://github.com/apache/datafusion/issues/14540
+#####################
+
+statement ok
+SET datafusion.execution.target_partitions = 1;
+
+statement ok
+CREATE TABLE events_mem (
+ "date" DATE,
+ "timestamp" TIMESTAMP,
+ ids STRUCT<id1 VARCHAR, extra INT>,
+ structs STRUCT<var1 VARCHAR, extra VARCHAR>
+) AS VALUES
+ (DATE '2025-01-03', TIMESTAMP '2025-01-03 01:00:00', {id1: 'dev1', extra: 1}, {var1: 'user1', extra: 'e1'}),
+ (DATE '2025-01-03', TIMESTAMP '2025-01-03 02:00:00', {id1: 'dev1', extra: 2}, {var1: 'user2', extra: 'e2'}),
+ (DATE '2025-01-04', TIMESTAMP '2025-01-04 01:00:00', {id1: 'dev2', extra: 3}, {var1: 'user3', extra: 'e3'});
+
+# The query from #14540. The extraction projection is the bottom node and the
+# filter sits above it.
+query TT
+EXPLAIN WITH events AS (
+ SELECT ids.id1 AS device, structs.var1 AS user, "timestamp"
+ FROM events_mem
+ WHERE "date" = '2025-01-03'
+)
+SELECT *, LAG(user, 1) OVER (PARTITION BY device ORDER BY "timestamp") AS prev
+FROM events
+WHERE device IS NOT NULL AND device != '' AND user IS NOT NULL AND user != ''
+LIMIT 100;
+----
+logical_plan
+01)Projection: events.device, events.user, events.timestamp, lag(events.user,Int64(1)) PARTITION BY [events.device] ORDER BY [events.timestamp ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW AS prev
+02)--Limit: skip=0, fetch=100
+03)----WindowAggr: windowExpr=[[lag(events.user, Int64(1)) PARTITION BY [events.device] ORDER BY [events.timestamp ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW]]
+04)------SubqueryAlias: events
+05)--------Projection: __datafusion_extracted_1 AS device, __datafusion_extracted_2 AS user, events_mem.timestamp
+06)----------Filter: events_mem.date = Date32("2025-01-03") AND __datafusion_extracted_1 IS NOT NULL AND __datafusion_extracted_1 != Utf8View("") AND __datafusion_extracted_2 IS NOT NULL AND __datafusion_extracted_2 != Utf8View("")
+07)------------Projection: get_field(events_mem.ids, Utf8("id1")) AS __datafusion_extracted_1, get_field(events_mem.structs, Utf8("var1")) AS __datafusion_extracted_2, events_mem.date, events_mem.timestamp
+08)--------------TableScan: events_mem projection=[date, timestamp, ids, structs]
+physical_plan
+01)ProjectionExec: expr=[device@0 as device, user@1 as user, timestamp@2 as timestamp, lag(events.user,Int64(1)) PARTITION BY [events.device] ORDER BY [events.timestamp ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW@3 as prev]
+02)--GlobalLimitExec: skip=0, fetch=100
+03)----BoundedWindowAggExec: wdw=[lag(events.user,Int64(1)) PARTITION BY [events.device] ORDER BY [events.timestamp ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW: Field { "lag(events.user,Int64(1)) PARTITION BY [events.device] ORDER BY [events.timestamp ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW": nullable Utf8View }, frame: RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW], mode=[Sorted]
+04)------SortExec: expr=[device@0 ASC NULLS LAST, timestamp@2 ASC NULLS LAST], preserve_partitioning=[false]
+05)--------ProjectionExec: expr=[__datafusion_extracted_1@0 as device, __datafusion_extracted_2@1 as user, timestamp@2 as timestamp]
+06)----------FilterExec: date@2 = 2025-01-03 AND __datafusion_extracted_1@0 IS NOT NULL AND __datafusion_extracted_1@0 != AND __datafusion_extracted_2@1 IS NOT NULL AND __datafusion_extracted_2@1 != , projection=[__datafusion_extracted_1@0, __datafusion_extracted_2@1, timestamp@3]
+07)------------ProjectionExec: expr=[get_field(ids@2, id1) as __datafusion_extracted_1, get_field(structs@3, var1) as __datafusion_extracted_2, date@0 as date, timestamp@1 as timestamp]
+08)--------------DataSourceExec: partitions=1, partition_sizes=[1]
+
+# Determinism: the plan is a fixed point reached before the default pass limit,
Review Comment:
These pass-limit checks show that the final plans match, but they do not prove that individual rules have stopped undoing each other within a pass. As optional extra coverage, could you add an observer-based test that verifies each rule leaves the plan unchanged during the final pass?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: decide the precedence between PushDownFilter and PushDownLeafProjections [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25455:
URL: https://github.com/apache/datafusion/pull/25455#issuecomment-5724280294
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25455?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `79.59184%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.33%. Comparing base ([`3a647e4`](https://app.codecov.io/gh/apache/datafusion/commit/3a647e49dd79b19d92eddd094bf927f28363332d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`43fbf0f`](https://app.codecov.io/gh/apache/datafusion/commit/43fbf0fbb8bfbb9a9cb05033578c65460e93c52b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25455?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/optimizer/src/push\_down\_filter.rs](https://app.codecov.io/gh/apache/datafusion/pull/25455?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fpush_down_filter.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL3B1c2hfZG93bl9maWx0ZXIucnM=) | 77.77% | [2 Missing and 8 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25455 +/- ##
==========================================
- Coverage 82.33% 82.33% -0.01%
==========================================
Files 1137 1137
Lines 432498 432542 +44
Branches 432498 432542 +44
==========================================
+ Hits 356115 356148 +33
- Misses 54844 54845 +1
- Partials 21539 21549 +10
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25455?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: decide the precedence between PushDownFilter and PushDownLeafProjections [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #25455:
URL: https://github.com/apache/datafusion/pull/25455#issuecomment-6018329602
@kosiew a followup in https://github.com/apache/datafusion/pull/26085
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org