fix: keep a projection's schema in step with its simplified expressions - #24321
Draft
zhuqi-lucas wants to merge 1 commit into
Draft
fix: keep a projection's schema in step with its simplified expressions#24321zhuqi-lucas wants to merge 1 commit into
zhuqi-lucas wants to merge 1 commit into
Conversation
`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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24321 +/- ##
==========================================
- Coverage 81.29% 81.13% -0.16%
==========================================
Files 1110 1112 +2
Lines 385205 386726 +1521
Branches 385205 386726 +1521
==========================================
+ Hits 313145 313769 +624
- Misses 53580 54482 +902
+ Partials 18480 18475 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 close?
Works towards #24284. Opened as a draft on purpose: the change itself is small, but it enforces an invariant the tree does not currently maintain, and I would like CI to enumerate the full fallout before anyone spends review time on it. See "What this is really asking" below.
Rationale for this change
LogicalPlan::map_expressionsreplaces a projection's expressions while keeping its existing schema, soSimplifyExpressionscan 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.Today that is invisible because
OptimizeProjectionsrebuilds the projections it touches withProjection::try_new, deriving the schema again and normalising it back. So whether a stale schema reaches the final plan depends on which rules happen to fire.Where it does reach the final plan it is observable, not just cosmetic. In
schema_evolution_nested.sltthe projection feedsCOPY (SELECT ...) TO ... STORED AS PARQUET, so the stale nullability is written into the parquet file.What changes are included in this PR?
SimplifyExpressionsderives the projection schema after rewriting, and only when the expressions actually changed.What this is really asking
Enforcing "a projection's schema equals what
projection_schemawould derive from its expressions" surfaces places where the tree does not currently hold that. Two so far:1. Stale nullability after constant folding. Fixed by this PR. With it, the four
roundtrip_literal_*tests indatafusion-substraitandschema_evolution_nested.sltpass without touching any test or snapshot, which is what makes me think the invariant is the right one.2. Logical and physical nullability disagree for
coalesce. Surfaced by this PR, not caused by it:coalesce'sreturn_field_from_argsis "non-null if any argument is non-null", and the literal2is non-null, sofalseis the accurate answer. Onmainthe query runs because the projection's schema still holds the pre-simplificationtrue, which happens to agree with the physical side. Making the logical side accurate is what puts the two out of step.From
datafusion-testing/data/sqlite/random/groupby/slt_good_1.slt:33179, so it only shows up under--include-sqlite.That is the question for #24284: is the invariant something the tree should hold, in which case the logical/physical asymmetry is a separate bug worth fixing, or is a projection's schema a declared output that expression rewrites must not change, in which case
OptimizeProjectionsshould stop silently re-deriving it and this PR is the wrong direction.I have no stake in which answer; I ran into this while trying to stop
OptimizeProjectionsfrom recomputing projection schemas (#24264, #24281) and would rather settle the semantics than work around them. Happy to implement either direction.Are there any user-facing changes?
Where a stale schema previously reached the final plan, that plan's nullability changes to match its expressions. No test or snapshot in the tree needed updating for the suites I can run locally (
datafusion-optimizer,-expr,-common,-sql,-substrait, andschema_evolution_nested.slt); the sqlite corpus is what turned up thecoalescecase, and CI on this PR should show whether there are more.