Repository navigation
bench: add stacked wide projection cases to sql_planner - #26841
Merged
Merged
Conversation
adriangb
approved these changes
Oct 28, 2026
Contributor
|
Thanks @zhuqi-lucas @adriangb ! |
adriangb
pushed a commit
to adriangb/datafusion
that referenced
this pull request
Nov 2, 2026
…omputing it (apache#24281) ## Which issue does this close? Closes apache#24264. Answers apache#24284 in the process. ## Rationale for this change `rewrite_projection_given_requirements`, the core of `OptimizeProjections`, prunes a projection's expressions to the subset actually required and then rebuilds it with `Projection::try_new`. `try_new` recomputes the output schema via `projection_schema`, calling `Expr::to_field` for every retained expression; column resolution is a linear scan over the input schema, so this is `O(exprs * schema_width)` per projection, per pass. The retained expressions are a subset of the ones the projection already has, so the answer is usually already sitting in `proj.schema`. Reusing it is only safe if that schema is still what the expressions derive. `OptimizeProjections` recomputing every schema was quietly fixing up the cases where it is not, so this PR first makes those cases explicit. ## What changes are included in this PR? A stored projection schema can go stale in three ways. Each is handled where it happens: **1. A rule rewrites the projection's own expressions.** - `LogicalPlan::map_expressions` now derives the projection schema again when the expressions were rewritten, and keeps it otherwise. This covers in-tree rules such as `SimplifyExpressions` (constant folding can turn a nullable function call into a non-null literal) and downstream rules registered with `add_optimizer_rule`, which previously relied on the next `OptimizeProjections` pass to fix the schema up. This is the root cause apache#24284 points at. - `CommonSubexprEliminate` builds its projection itself, so it derives the schema there when it changed something (`common_subexpr_eliminate.rs`). Splitting a CASE guard such as `WHEN x IS NOT NULL THEN x` into two common subexpressions means the rewritten CASE can no longer be proven non-null, while the stored schema still said non-null. This is the sqlite sqllogictest failure the previous revision hit (`slt_good_1.slt:33179`); a minimal version is now in `cse.slt`. **2. The projection contains a subquery.** A subquery is optimized on its own, so the field it produces can change after the outer projection's schema was derived. `OptimizeProjections` recomputes the schema for these. **3. Rewriting the input changes a field the projection reads**, for example an aggregate rebuilt after constant folding made a group column non-null. After the input is rewritten, `OptimizeProjections` compares the `k` fields the projection reads before and after (`read_fields_unchanged`). If any differ, it recomputes. **Otherwise it reuses the schema.** `project_schema_by_indices` selects the already-computed fields at the retained indices and builds with `Projection::try_new_with_schema`; when nothing is pruned, the existing `Arc` is reused as-is. Functional dependencies are projected through the kept indices; they can be weaker than recomputed ones, never stronger. The checks cost `O(k)` with no name lookups, so the common case goes from `O(exprs * width)` to `O(k)`. ## Benchmarks The existing `sql_planner` cases barely exercise this: their projections are narrow or plain columns, so recomputing the schema is cheap there. apache#26841 adds `optimizer_wide_projection_stack_{all,half}`: four nested projections of 200 computed expressions each over a 200-column table, as produced by layered views. Locally (Apple M4 Pro), with that benchmark applied to both sides: | case | main | this PR | |---|---|---| | `optimizer_wide_projection_stack_all` | 11.28 ms | 6.24 ms (-45%) | | `optimizer_wide_projection_stack_half` | 5.57 ms | 3.81 ms (-32%) | On a downstream production query with a ~60-column projection, `OptimizeProjections` went from 18.1 ms to 9.2 ms (-49%). ## Are these changes tested? To check that nothing else can go stale, I temporarily logged every reused schema that differs from `projection_schema` and ran all 542 sqllogictest files. Before (2) and (3) it logged 13 cases (12 scalar subqueries, 1 aggregate); with them it logs none. So for everything the test suite covers, the reused fields are exactly what `try_new` would compute, and final plans are unchanged. Tests, each verified to fail without its fix: - `projection_schema_follows_rule_rewrite` (a downstream-style rule using `map_expressions`), `projection_schema_kept_without_rule_rewrite` (unchanged expressions keep the same schema `Arc`), `projection_schema_follows_rewritten_exprs` (CSE unit test) and the `cse.slt` coalesce case for (1) - `projection_schema_follows_subquery` for (2) - `projection_schema_follows_rewritten_input` for (3) - `project_schema_by_indices_matches_recompute`: the sliced schema matches `projection_schema` on fields, qualifiers, metadata and functional dependencies, and the identity subset reuses the same `Arc` No existing test or snapshot needed updating. Local runs on current `main`: `datafusion-optimizer` 958 + 29 + 5, `datafusion-expr`, and all 542 sqllogictest files. ## Are there any user-facing changes? Optimized plans are unchanged. `LogicalPlan::map_expressions` now returns a projection with a freshly derived schema when it rewrote the projection's expressions, so custom rules no longer need to rebuild it with `Projection::try_new` themselves. API change: because the schema is derived from the new expressions, a rewrite that renames outputs (e.g. `unalias`) now changes the returned `schema()`, and a rewrite whose new expressions reference columns of an input installed only afterwards now fails inside `map_expressions` on a `Projection`. The 56.0.0 upgrade guide describes both and the migration.
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
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.
Which issue does this PR close?
Part of #24264.
Rationale for this change
sql_plannerhas no case where a projection holds many computed expressions over a wide input, which is what layered views produce. Recomputing projection schemas in the optimizer is expensive exactly there, so changes such as #24281 show no signal on the existing cases. With these cases, #24281 measures 11.28 ms -> 6.24 ms (all) and 5.57 ms -> 3.81 ms (half) locally.What changes are included in this PR?
Adds
optimizer_wide_projection_stack_allandoptimizer_wide_projection_stack_half: four nested projections of 200 computed expressions each over a 200-column table, with the outer query selecting all or half of the columns. Each expression reads its input column twice so the layers are not merged.Are these changes tested?
Benchmark only. Run with
cargo bench --bench sql_planner -- optimizer_wide_projection.Are there any user-facing changes?
No.