apache / apache/datafusion

Projection schema can go stale after SimplifyExpressions; OptimizeProjections silently normalizes it

Open
#24,284 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.