Describe the bug
A Projection's stored schema can disagree with what projection_schema(input, &expr) would recompute from its own expressions. Today this is invisible because OptimizeProjections happens to rebuild every projection it touches with Projection::try_new, which recomputes the schema and silently normalizes it back.
There are two conflicting conventions in the codebase:
Preserving the schema looks deliberate on the simplify side: simplify_exprs.rs explicitly uses Aggregate::try_new_with_schema(input, group_expr, aggr_expr, schema) for the aggregate case, i.e. simplification is not meant to change a node's output schema.
The net effect is that constant folding can leave a projection whose stored nullability no longer matches its expressions, and whether that survives to the final plan depends on whether OptimizeProjections later rebuilds the node.
To Reproduce
SELECT STRUCT(1, true, CAST(NULL AS STRING)) FROM data
Walking the plan after each optimizer rule (via Optimizer::optimize's observer) and comparing each Projection's stored schema against projection_schema(input, &expr):
- initial plan from the SQL planner: consistent
- after
simplify_expressions: stale, and it stays stale for every subsequent rule
Concretely, struct(1, true, CAST(NULL AS Utf8View)) folds to a single non-null Struct literal. The folded literal recomputes to nullable: false, while the projection's stored field remains nullable: true from before folding.
Expected behavior
Either:
- A
Projection's schema is an invariant that always equals projection_schema(input, &expr), in which case SimplifyExpressions (and anything else going through map_expressions) should recompute it after rewriting expressions; or
- The schema is explicitly allowed to be a "declared" output schema that expression rewrites must not change, in which case
OptimizeProjections should stop silently recomputing it, and the recompute in with_new_exprs is the inconsistent one.
Right now both conventions coexist and the outcome depends on which rules happen to fire.
Additional context
Found while working on #24264 / #24281. That PR avoids the O(exprs * schema_width) schema recompute in rewrite_projection_given_requirements by slicing the existing projection schema instead of calling Projection::try_new. That removes the accidental normalization, and four substrait roundtrip tests then fail because the stale nullability survives into the final plan:
cases::roundtrip_logical_plan::roundtrip_literal_list
cases::roundtrip_logical_plan::roundtrip_literal_named_struct
cases::roundtrip_logical_plan::roundtrip_literal_renamed_struct
cases::roundtrip_logical_plan::roundtrip_literal_struct
They assert plan.schema() == plan2.schema() across a substrait roundtrip. On main both sides end up nullable: false because OptimizeProjections recomputed both. With the recompute removed, the original side keeps the stale nullable: true while the substrait side (rebuilt with Projection::try_new) is false. So the substrait consumer is not at fault here; it is the only side that ends up canonical.
I have parked #24281 as a draft until this is settled, since the answer decides whether that optimization is sound as written. Happy to implement whichever direction maintainers prefer.
Describe the bug
A
Projection's storedschemacan disagree with whatprojection_schema(input, &expr)would recompute from its own expressions. Today this is invisible becauseOptimizeProjectionshappens to rebuild every projection it touches withProjection::try_new, which recomputes the schema and silently normalizes it back.There are two conflicting conventions in the codebase:
LogicalPlan::map_expressions(used bySimplifyExpressions) replacesexprand keeps the existingschema:LogicalPlan::with_new_exprsrecomputes it:Preserving the schema looks deliberate on the simplify side:
simplify_exprs.rsexplicitly usesAggregate::try_new_with_schema(input, group_expr, aggr_expr, schema)for the aggregate case, i.e. simplification is not meant to change a node's output schema.The net effect is that constant folding can leave a projection whose stored nullability no longer matches its expressions, and whether that survives to the final plan depends on whether
OptimizeProjectionslater rebuilds the node.To Reproduce
Walking the plan after each optimizer rule (via
Optimizer::optimize's observer) and comparing eachProjection's stored schema againstprojection_schema(input, &expr):simplify_expressions: stale, and it stays stale for every subsequent ruleConcretely,
struct(1, true, CAST(NULL AS Utf8View))folds to a single non-nullStructliteral. The folded literal recomputes tonullable: false, while the projection's stored field remainsnullable: truefrom before folding.Expected behavior
Either:
Projection'sschemais an invariant that always equalsprojection_schema(input, &expr), in which caseSimplifyExpressions(and anything else going throughmap_expressions) should recompute it after rewriting expressions; orOptimizeProjectionsshould stop silently recomputing it, and the recompute inwith_new_exprsis the inconsistent one.Right now both conventions coexist and the outcome depends on which rules happen to fire.
Additional context
Found while working on #24264 / #24281. That PR avoids the
O(exprs * schema_width)schema recompute inrewrite_projection_given_requirementsby slicing the existing projection schema instead of callingProjection::try_new. That removes the accidental normalization, and four substrait roundtrip tests then fail because the stale nullability survives into the final plan:They assert
plan.schema() == plan2.schema()across a substrait roundtrip. Onmainboth sides end upnullable: falsebecauseOptimizeProjectionsrecomputed both. With the recompute removed, the original side keeps the stalenullable: truewhile the substrait side (rebuilt withProjection::try_new) isfalse. So the substrait consumer is not at fault here; it is the only side that ends up canonical.I have parked #24281 as a draft until this is settled, since the answer decides whether that optimization is sound as written. Happy to implement whichever direction maintainers prefer.