Skip to content

perf: reuse projection schema in OptimizeProjections instead of recomputing it - #24281

Open
zhuqi-lucas wants to merge 3 commits into
apache:mainfrom
zhuqi-lucas:optimize-projections-reuse-schema
Open

perf: reuse projection schema in OptimizeProjections instead of recomputing it#24281
zhuqi-lucas wants to merge 3 commits into
apache:mainfrom
zhuqi-lucas:optimize-projections-reuse-schema

Conversation

@zhuqi-lucas

@zhuqi-lucas zhuqi-lucas commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Which issue does this close?

Closes #24264. Answers #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 already sitting in proj.schema.

Reusing it turned out not to be safe as-is, which is what the first revision of this PR got wrong and what #24284 is about.

LogicalPlan::map_expressions replaces a projection's expressions while keeping its existing schema, so SimplifyExpressions can leave the two out of step. Constant folding turns arrow_cast([...], 'LargeList(...)'), a function call whose field the planner derived as nullable, into a non-null literal whose field is not, while the schema keeps the pre-folding answer. OptimizeProjections calling try_new was quietly normalising that back, so whether a stale schema survived into the final plan depended on which rules happened to fire.

So this PR fixes that first, then does the optimisation.

What changes are included in this PR?

1. SimplifyExpressions derives the projection schema after rewriting (simplify_exprs.rs), and only when the expressions actually changed.

Final plans are unchanged: the normalisation OptimizeProjections was performing simply happens earlier now. Nothing in the tree needed updating, no snapshot and no expected plan, which is the clearest evidence this is an equivalence rather than a behaviour change.

2. rewrite_projection_given_requirements derives the pruned schema by selecting the already-computed fields from the existing projection schema (project_schema_by_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.

Cost goes from O(exprs * width) to O(k), and to O(1) when nothing is pruned. Unlike making the recompute cheaper, this removes the work rather than speeding it up: no to_field call, no field allocation, no cache and no heuristics.

Correctness

With (1) in place, proj.schema is in step with proj.expr, so slicing it at the retained indices produces exactly what projection_schema would recompute: field i corresponds to expression i, and RequiredIndices yields a sorted, deduplicated subset.

project_schema_by_indices_matches_recompute asserts that, for a mixed expression list (plain column, computed binary expr, alias, NULL literal, qualified column) and every representative index subset, the sliced schema matches projection_schema on fields, qualifiers, schema metadata and functional dependencies, and that the identity subset reuses the same Arc.

The two failures the first revision of this PR introduced are fixed by (1), not worked around:

  • roundtrip_literal_list, roundtrip_literal_struct, roundtrip_literal_named_struct, roundtrip_literal_renamed_struct in datafusion-substrait, which compare plan schemas across a roundtrip
  • schema_evolution_nested.slt, where the projection feeds COPY (SELECT ...) TO ... STORED AS PARQUET, so a stale nullability reached the written file and DESCRIBE reported YES instead of NO

Full local runs: datafusion-substrait 49 + 200 + 3, datafusion-optimizer 760 + 26 + 5, datafusion-expr 248 + 55, datafusion-common 547, datafusion-sql 88 + 572 + 12, and schema_evolution_nested.slt 1/1. All green, with no test or snapshot modified.

Are there any user-facing changes?

No. Optimized plans are unchanged.

Copilot AI lite review requested due to automatic review settings August 12, 2026 07:51
@github-actions github-actions Bot added the optimizer Optimizer rules label Aug 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves OptimizeProjections performance by avoiding repeated recomputation of projection output schemas when pruning projection expressions, instead reusing/slicing the already-computed Projection.schema.

Changes:

  • Update rewrite_projection_given_requirements to build pruned projections via Projection::try_new_with_schema using a sliced schema from the existing projection schema.
  • Add project_schema_by_indices helper to project fields + functional dependencies while reusing schema metadata.
  • Add a unit test validating that sliced schemas match projection_schema recomputation across representative index subsets.
Suppressed comments (1)

datafusion/optimizer/src/optimize_projections/mod.rs:1285

  • project_schema_by_indices also projects functional dependencies and preserves schema-level metadata, but the test currently only compares fields and qualifiers. Adding assertions for functional dependencies and schema metadata will better protect the behavior this PR relies on.
            // Output fields (name, data type, nullability, field metadata) must
            // match the from-scratch computation exactly.
            assert_eq!(
                reused.fields(),
                recomputed.fields(),
                "fields differ for indices {indices:?}"
            );

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +1249 to +1252
binary_expr(col("b"), Operator::Plus, col("c")),
col("c").alias("c_alias"),
lit(1_i64).alias("one"),
Expr::Column(Column::new(Some(TableReference::bare("test")), "b")),
…puting it

`rewrite_projection_given_requirements` rebuilt the pruned projection with
`Projection::try_new`, which recomputes the output schema from scratch via
`projection_schema`: it calls `Expr::to_field` for every retained expression,
and column resolution (`DFSchema::field_from_column`) is a linear scan, so
recomputing a projection's schema is O(exprs * schema_width) and runs on every
projection on every optimizer pass. This is especially costly for wide
`SELECT *`-style projections over wide schemas.

The retained expressions are a subset of the projection's original expressions,
so their output fields are unchanged by pruning unreferenced sibling columns.
Select those fields from the existing projection schema and construct the pruned
projection with `try_new_with_schema`, mirroring the schema reuse already done
in `merge_consecutive_projections`. When nothing is pruned the schema Arc is
reused as-is. This turns the per-projection schema cost from O(exprs * width)
into O(k).

Behavior-preserving: the sliced schema is identical to the recomputed one. Adds
`project_schema_by_indices_matches_recompute` asserting that equivalence across
expression subsets; the full datafusion-optimizer suite still passes.
Addresses review feedback on the schema-reuse test:

- The comment claimed a nullable literal, but lit(1_i64) is non-nullable,
  so nullability propagation was never actually exercised. Swapped it for a
  NULL Int64 literal and added assertions pinning the premise that the
  literal is nullable while the input columns are not.
- project_schema_by_indices also carries schema-level metadata and projects
  functional dependencies through the kept indices, but the test only
  compared fields and qualifiers. Both are now asserted against the
  from-scratch computation for every subset.
`LogicalPlan::map_expressions` replaces a projection's expressions while
keeping its existing schema, so `SimplifyExpressions` could leave the two
out of step: constant folding turns a function call, whose field the
planner derived as nullable, into a non-null literal, whose field is not,
and the schema keeps the pre-folding answer.

That was invisible because `OptimizeProjections` rebuilds the projections
it touches with `Projection::try_new`, deriving the schema again and
normalising it back. Which meant whether a stale schema reached the final
plan depended on which rules happened to fire, and it blocked deriving a
pruned projection's schema by reuse rather than recomputation.

Derive the schema here instead, only when the expressions actually
changed. The final plans are unchanged, since the normalisation that
`OptimizeProjections` was doing simply happens earlier now: no snapshot
or expected plan in the tree needed updating.
@zhuqi-lucas
zhuqi-lucas marked this pull request as ready for review August 13, 2026 07:12
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.62500% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.13%. Comparing base (c08832d) to head (8d4ece9).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...tafusion/optimizer/src/optimize_projections/mod.rs 90.00% 0 Missing and 8 partials ⚠️
...timizer/src/simplify_expressions/simplify_exprs.rs 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24281      +/-   ##
==========================================
- Coverage   81.29%   81.13%   -0.16%     
==========================================
  Files        1110     1112       +2     
  Lines      385205   386802    +1597     
  Branches   385205   386802    +1597     
==========================================
+ Hits       313145   313835     +690     
- Misses      53580    54485     +905     
- Partials    18480    18482       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OptimizeProjections: projection schema construction is O(exprs × width)

3 participants