adriangb opened a new issue, #25459: URL: https://github.com/apache/datafusion/issues/25459
## Summary This EPIC tracks the open bugs, the design decisions and the test work for the two logical optimizer features that move expressions and filters towards the table scan: - `ExtractLeafExpressions` and `PushDownLeafProjections` (`datafusion.optimizer.enable_leaf_expression_pushdown`, default `true`, in `datafusion/optimizer/src/extract_leaf_expressions.rs`) - `PushDownFilter` (`datafusion/optimizer/src/push_down_filter.rs`) The leaf rules have received ten bug fixes since they landed in February 2026 (https://github.com/apache/datafusion/pull/20117). All ten were found by users or by an ad-hoc fuzzer. None was found by an existing test. Two mechanisms explain most of them: 1. Columns are resolved by name string (`flat_name()` or the bare name) when a projection is moved through another node. A computed column with the same name as a table column is then confused with that column. This gave https://github.com/apache/datafusion/issues/25414 (wrong results), https://github.com/apache/datafusion/pull/25412 (planning error), https://github.com/apache/datafusion/issues/24241 and https://github.com/apache/datafusion/issues/22895. 2. A column definition is inlined into a consumer at several sites, and each site has its own guard against duplicating a volatile or expensive expression. Each guard was added after a bug report: https://github.com/apache/datafusion/pull/24720, https://github.com/apache/datafusion/pull/23691, https://github.com/apache/datafusion/pull/23459, https://github.com/apache/datafusion/pull/25416. https://github.com/apache/datafusion/issues/25329 is the shape that none of them covers. The report behind this EPIC, with SQL reproductions verified on `main` at 4e907557ad, is summarized in the sections below. ## Strategy Work in this order. Each step is independent of the ones after it. 1. Fix the wrong-results bugs in the default configuration first. Add every reproduction to `sqllogictest` with a result assertion, not only an `EXPLAIN`. 2. Replace per-site guards with one inlining policy, so the next duplication bug cannot appear at a site that has no guard. 3. Lock down the decisions that are ambiguous today, so plans are intentional and deterministic (section "Decisions" below). A decision that is wrong but documented is better than a plan that depends on rule order. 4. Add tests that find the next bug before a user does: a differential fuzz (leaf pushdown on vs off, Parquet filter pushdown on vs off), an optimizer invariant that forbids new evaluations of volatile and `KeepInPlace` expressions, and the test gaps that mutation testing found. 5. Then, and only then, the large refactor: resolve columns by schema index instead of name, and replace the alias-prefix protocol with a typed marker. Items that depend on data or cost that the planner does not know (which function is cheap for which source, filter-first vs projection-first on a non-Parquet source, `pushdown_filters` by default) are listed as trade-offs. They are not bugs. They get a decision and a benchmark, not a heuristic. ## Decisions These are the ambiguous behaviours that this EPIC locks down. Each one gets a documented answer in the code, a test that fails if the answer changes, and no configuration option. | Decision | Today | Decided behaviour | Where it is enforced | |---|---|---|---| | Order of a `Filter` and a pure extraction projection that it does not reference (https://github.com/apache/datafusion/issues/14540) | Depends on rule order in the list. `PushDownFilter` and `PushDownLeafProjections` undo each other. One optimizer pass is wasted. | `PushDownFilter` yields. A pure extraction projection stays below a filter that does not reference its aliases. Measured on Parquet: the opposite order (leaf rule yields) loses the struct leaf in `DataSourceExec` at the default `pushdown_filters = false`, because the physical `ProjectionPushdown` cannot move the projection through a `FilterExec` whose predicate needs a column the projection does not produce. The filter loses nothing: `PushDownFilter` runs before `ExtractLeafExpressions`, so the predicate is already recorded in `TableScan::filters` before any extraction projection exists. On a source that cannot absorb the projection the filter node still sits above the projection, so this order is deterministic, not op timal, for that case. | https://github.com/apache/datafusion/pull/25455 (docs in both rules; `max_passes` pinned in slt so a change in the order fails a test) | | Inlining a column definition into a consumer | Five guards at five sites, each with its own rule. | Never inline a volatile definition. Never inline a `KeepInPlace` definition into more than one evaluation site. `Column`, `Literal` and `MoveTowardsLeafNodes` definitions may always be inlined. | https://github.com/apache/datafusion/pull/25456 (crate-private `ProjectionInliner`, four leaf-rule sites plus `PushDownFilter::rewrite_projection` and the `OptimizeProjections` merge routed through it; `would_duplicate_volatile` and `merge_would_duplicate_kept_expr` deleted) | | When `PushDownLeafProjections` needs a recovery projection | Compares the set of unqualified field names, so a computed column with the same name as an input column is dropped. | A recovery projection is required whenever a recovery expression is not a pass-through column, or when qualifier, name or type differ. | https://github.com/apache/datafusion/pull/25445 | | Pushing a predicate through a projection that computes a `KeepInPlace` expression referenced once | Pushed. The expression is evaluated in the filter and again in the projection. | Keep pushing it. The predicate can reach the scan (row-group pruning, `partial_filters`), which the planner cannot value without a cost model. The draft https://github.com/apache/datafusion/pull/25388 shows the cost of the opposite choice: `CAST(ts ...)` range filters stop reaching the scan. | Documented in the inlining policy; test in `push_down_filter.rs`. | | Which functions are `MoveTowardsLeafNodes` | A global constant per UDF. | Unchanged for now. A source-level veto is a follow-up (see design issues). `array_length(arr, dim)` with a non-literal `dim` stays `KeepInPlace`. | https://github.com/apache/datafusion/pull/25025 review | ## Open bugs | Issue | Effect | Fix | |---|---|---| | https://github.com/apache/datafusion/issues/25414 | Wrong results, default config, five SQL shapes | https://github.com/apache/datafusion/pull/25445 | | https://github.com/apache/datafusion/pull/25412 | Planning error, `UNION ALL` with an empty branch over a CTE | PR open | | https://github.com/apache/datafusion/issues/25329 | `KeepInPlace` UDF evaluated two times in a `WHERE` | https://github.com/apache/datafusion/pull/25456 (crate-private `ProjectionInliner`, four leaf-rule sites plus `PushDownFilter::rewrite_projection` and the `OptimizeProjections` merge routed through it; `would_duplicate_volatile` and `merge_would_duplicate_kept_expr` deleted) (narrow rule); https://github.com/apache/datafusion/pull/25388 (draft, broader rule) | | https://github.com/apache/datafusion/issues/14540 | Two rules undo each other, one wasted pass | https://github.com/apache/datafusion/pull/25455 (docs in both rules; `max_passes` pinned in slt so a change in the order fails a test) | | https://github.com/apache/datafusion/issues/15046 | Subquery outer references invisible to `column_refs`, plan fails to execute past an extension node | https://github.com/apache/datafusion/pull/25294 | | https://github.com/apache/datafusion/issues/25268 | A conjunct marked pushed at plan time is dropped per file at run time with `pushdown_filters = true` | Not started. The silent `debug!` fallback must become an error or a post-scan filter. | | https://github.com/apache/datafusion/issues/25446 | Planning error when a sub-query projection renames or swaps columns and a struct field is read above | Not started. Not fixed by https://github.com/apache/datafusion/pull/25412 (verified on its head). | | https://github.com/apache/datafusion/issues/25447 | The same `get_field` is extracted two times into one projection | Not started. | | https://github.com/apache/datafusion/issues/25457 | Wrong results: `BETWEEN` on a volatile operand evaluates it two times (`simplify_expressions` expansion). Found by the invariant in https://github.com/apache/datafusion/pull/25458. | Not started. | Fixed recently, listed so the pattern is visible: https://github.com/apache/datafusion/issues/25415 (https://github.com/apache/datafusion/pull/25416), https://github.com/apache/datafusion/issues/24678 (https://github.com/apache/datafusion/pull/24720), https://github.com/apache/datafusion/issues/23655 (https://github.com/apache/datafusion/pull/23691), https://github.com/apache/datafusion/issues/24241, https://github.com/apache/datafusion/issues/22955, https://github.com/apache/datafusion/issues/22895, https://github.com/apache/datafusion/issues/22615, https://github.com/apache/datafusion/issues/20430. ## Tests | Item | Status | |---|---| | Differential fuzz: leaf pushdown on vs off, Parquet `pushdown_filters` on vs off | https://github.com/apache/datafusion/pull/25453 (reproduces https://github.com/apache/datafusion/issues/25414 in 526 of 3000 seeds, 50 of them silent wrong results, and the https://github.com/apache/datafusion/pull/25412 error in 404 of 3000; both shapes gated by a `const` until the fixes land). Note: `fuzz_cases` only runs in the merge-queue `extended_tests` job, not in PR CI. | | Optimizer invariant: no rule may add an evaluation site of a volatile or `KeepInPlace` expression | https://github.com/apache/datafusion/pull/25458 (`evaluation_sites.rs`, zero false positives on the full suite, +12% to +15% CPU on the sqllogictest run when on, shipped off behind two `const` switches). With the switches on it reports 14 queries in 7 files, all true positives, including https://github.com/apache/datafusion/issues/25329 and https://github.com/apache/datafusion/issues/25457. | | Test gaps found by mutation testing of the two files | https://github.com/apache/datafusion/issues/25451 (220 mutants, 139 caught, 25 missed, 9 real gaps); tests for 3 functions in https://github.com/apache/datafusion/pull/25452 | | Result assertions (not only `EXPLAIN`) for every reproduction in this EPIC | Part of each fix PR | ## Design issues (larger refactors) | Item | Issue | |---|---| | Resolve columns by schema index, not by name, in the leaf rules | https://github.com/apache/datafusion/issues/25448 | | Replace the `__datafusion_extracted` / `__common_expr` alias-prefix protocol with a typed marker | https://github.com/apache/datafusion/issues/25449 | | Let the data source veto `MoveTowardsLeafNodes` per function | https://github.com/apache/datafusion/issues/25450 | ## Trade-offs (not bugs, need a benchmark, not a heuristic) - Filter-first vs extraction-first on sources that cannot absorb the projection (see Decisions). - `datafusion.execution.parquet.pushdown_filters` default: https://github.com/apache/datafusion/issues/3463, https://github.com/apache/datafusion/issues/20324, https://github.com/apache/datafusion/issues/24393. - Which scalar functions are cheap enough to move to the scan: https://github.com/apache/datafusion/issues/25036, https://github.com/apache/datafusion/pull/25025. - Nested pruning and `IS NULL` on whole structs in the Parquet row filter: https://github.com/apache/datafusion/issues/24120, https://github.com/apache/datafusion/issues/21795. ## Findings from building the integration branch A local branch `leaf-pushdown-integration` holds all of the PRs above merged on top of `main` (https://github.com/apache/datafusion/pull/25412, https://github.com/apache/datafusion/pull/25445, https://github.com/apache/datafusion/pull/25456, https://github.com/apache/datafusion/pull/25455, https://github.com/apache/datafusion/pull/25453, https://github.com/apache/datafusion/pull/25452, https://github.com/apache/datafusion/pull/25458). Facts that matter for review order: - https://github.com/apache/datafusion/pull/25456 deletes `merge_would_duplicate_kept_expr`, so the unit test that https://github.com/apache/datafusion/pull/25452 adds for that function must be dropped when both land. The other two tests in that PR apply unchanged. - https://github.com/apache/datafusion/pull/25455 changes `is_pure_extraction_projection` to take `&[Expr]` and shares it with `PushDownFilter`. https://github.com/apache/datafusion/pull/25456 changes the signature of `try_push_input`. The two merge with a small manual resolution in `push_extraction_pairs`. - With https://github.com/apache/datafusion/pull/25456 merged, one snapshot in https://github.com/apache/datafusion/pull/25412 changes: the extracted expression is spelled `leaf_udf(test.a, ...)` instead of `leaf_udf(a, ...)`. That is the qualified spelling the input schema holds and it removes the bare-vs-qualified gap that https://github.com/apache/datafusion/issues/25447 describes for that path. - `[profile.ci.package."*"]` sets `debug-assertions = false`, so `#[cfg(debug_assertions)]` code in `datafusion-optimizer` does not run when the crate is a dependency of the sqllogictest binary. The existing `check_invariants(InvariantLevel::Executable)` call in the optimizer loop is therefore not exercised by the sqllogictest suite today. This needs its own issue. - `datafusion/core/tests/fuzz_cases` runs only in the merge-queue `extended_tests` job. A fuzz test there does not run on pull requests. ## Not filed yet - `EXPLAIN` prints a nested alias inside a `CAST` (`CAST(character_length(x) AS length(x) AS Int64)`). Pre-existing, display only, seen while working on https://github.com/apache/datafusion/pull/25455. - `optimize_projections` re-inlines a CSE column for `file_row_index()` in the logical plan; the physical planner collapses it again, so there is no wrong result. Seen with the invariant in https://github.com/apache/datafusion/pull/25458. -- 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]
