Projection schema can go stale after SimplifyExpressions; OptimizeProjections silently normalizes it
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
### 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:
- `LogicalPlan::map_expressions` (used by `SimplifyExpressions`) replaces `expr` and **keeps the existing `schema`**:
```rust
LogicalPlan::Projection(Projection { expr, input, schema }) =>
expr.map_elements(f)?.update_data(|expr| {
LogicalPlan::Projection(Projection { expr, input, schema })
})
```
- `LogicalPlan::with_new_exprs` **recomputes** it:
```rust
LogicalPlan::Projection(Projection { .. }) => {
let input = self.only_input(inputs)?;
Projection::try_new(expr, Arc::new(input)).map(LogicalPlan::Projection)
}
```
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
```sql
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:
1. 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
2. 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.
Contributor guide
Research direction
Trace LogicalPlan::map_expressions in simplify_exprs.rs alongside Projection::try_new, with_new_exprs, and rewrite_projection_given_requirements. Reproduce the SQL case with Optimizer::optimize's observer and compare stored projection schemas with projection_schema(input, &expr). Use the four named substrait roundtrip tests to verify the chosen schema convention and its consistency.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100