dariocurr opened a new pull request, #24094:
URL: https://github.com/apache/datafusion/pull/24094
## Which issue does this PR close?
Follow-up to #23861 (issue #15394). Not closing a new issue.
## Rationale for this change
#23861 fixed `UNION ALL` batches carrying the wrong nullability when one leg
is `NOT NULL` and another isn't, by re-stamping each batch's schema inside
`UnionExec`/`InterleaveExec`'s own `execute()`. In review, @alamb noted:
> ideally we could coerce the schema at plan time but I don't know how to
> coerce nullability
This PR does that: the coercion becomes an explicit node in the plan tree,
inserted when the plan is built, instead of invisible logic inside
`execute()`.
## What changes are included in this PR?
- Adds `CoerceSchemaExec`, a single-child passthrough `ExecutionPlan` node.
`UnionExec::try_new`/`InterleaveExec::try_new` insert it above any child
whose own output schema disagrees with the computed union schema (in
practice, only nullability differs -- `UnionExec::try_new` already rejects
real data-type mismatches via `calculate_union`).
- The actual batch re-stamping logic (`SchemaConformingStream`) is unchanged;
it just lives under `CoerceSchemaExec::execute()` now instead of being
called directly from `UnionExec`/`InterleaveExec::execute()`.
- Because it's a real plan node, `CoerceSchemaExec` implements the full
`ExecutionPlan` surface a pure 1:1 passthrough needs to stay transparent
to the optimizer: statistics passthrough, filter/limit pushdown,
`benefits_from_input_partitioning() -> false` (so it doesn't trigger a
spurious repartition), and proto (de)serialization -- the node erases
itself on encode and is reconstructed by `try_new` on decode, so no
protobuf schema change was needed.
- A genuine data-type mismatch (as opposed to nullability-only) is now
rejected eagerly at plan-build time (via `EquivalenceProperties::
with_new_schema`) rather than lazily at `execute()`.
## Are these changes tested?
- New unit test `test_union_partition_statistics_with_mismatched_nullability`
in `union.rs`, proving statistics aren't poisoned to `Absent` through the
new node.
- Existing `union_nullable`/`union_nullable_spill` regression tests from
#23861 continue to pass unchanged.
- Updated the `sqllogictest` golden file (`union.slt`) where `EXPLAIN`
output now shows the new node for pre-existing nullability-mismatched
`UNION ALL` cases.
- Benchmarked against the previous (inline) approach: no measurable
performance difference in either the coerced or matched-schema case
(differences were within run-to-run noise).
## Are there any user-facing changes?
`EXPLAIN` output for a `UNION ALL`/interleaved plan with a nullability
mismatch across legs will now show a `CoerceSchemaExec` node that wasn't
there before. No behavioral or correctness change.
--
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]
