zhuqi-lucas opened a new pull request, #24321: URL: https://github.com/apache/datafusion/pull/24321
## 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: ```sql 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. -- 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]
