Skip to content

fix: keep a projection's schema in step with its simplified expressions - #24321

Draft
zhuqi-lucas wants to merge 1 commit into
apache:mainfrom
zhuqi-lucas:projection-schema-invariant
Draft

fix: keep a projection's schema in step with its simplified expressions#24321
zhuqi-lucas wants to merge 1 commit into
apache:mainfrom
zhuqi-lucas:projection-schema-invariant

Conversation

@zhuqi-lucas

Copy link
Copy Markdown
Contributor

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_expressions replaces a projection's expressions while keeping its existing schema, so SimplifyExpressions can 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 OptimizeProjections rebuilds the projections it touches with Projection::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.slt the projection feeds COPY (SELECT ...) TO ... STORED AS PARQUET, so the stale nullability is written into the parquet file.

What changes are included in this PR?

SimplifyExpressions derives 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_schema would 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 in datafusion-substrait and schema_evolution_nested.slt pass 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:

SELECT DISTINCT COALESCE(+cor0.col1, cor0.col1, 2, cor0.col1) FROM tab0 AS cor0 GROUP BY cor0.col1
Internal error: Physical input schema should be the same as the one converted from logical input schema. Differences:
- field nullability at index 0 [coalesce(cor0.col1,cor0.col1,Int64(2),cor0.col1)]: (physical) true vs (logical) false.

coalesce's return_field_from_args is "non-null if any argument is non-null", and the literal 2 is non-null, so false is the accurate answer. On main the query runs because the projection's schema still holds the pre-simplification true, 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 OptimizeProjections should 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 OptimizeProjections from 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, and schema_evolution_nested.slt); the sqlite corpus is what turned up the coalesce case, and CI on this PR should show whether there are more.

`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-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.13%. Comparing base (c08832d) to head (5cd969b).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...timizer/src/simplify_expressions/simplify_exprs.rs 93.75% 0 Missing and 1 partial ⚠️
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.
📢 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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants