physical-plan: coerce UNION/INTERLEAVE schema mismatches at plan time - #24094
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24094 +/- ##
==========================================
- Coverage 80.99% 80.98% -0.01%
==========================================
Files 1106 1106
Lines 383352 383495 +143
Branches 383352 383495 +143
==========================================
+ Hits 310488 310581 +93
- Misses 54544 54578 +34
- Partials 18320 18336 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8c2409c to
eab8fcc
Compare
Follow-up to apache#23861 (issue apache#15394). Moves the schema re-stamping for nullability-mismatched UNION ALL / INTERLEAVE inputs out of `execute()` and into plan construction, via a new `CoerceSchemaExec` node inserted by `UnionExec::try_new`/`InterleaveExec::try_new` whenever a child's own schema disagrees with the computed union schema. The node is now visible in `EXPLAIN` output, is transparent for statistics/pushdown/proto purposes, and adds no measurable overhead versus inline re-stamping.
c987b39 to
2bba020
Compare
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The plan-visible CoerceSchemaExec approach looks good, and I like that it removes the schema restamping from UnionExec::execute() and InterleaveExec::execute() while keeping the coercion explicit in the plan.
I left one non-blocking suggestion for additional protobuf round-trip coverage. Otherwise, this looks good to me.
Addresses review feedback on apache#24094 (kosiew): verify that `CoerceSchemaExec::try_to_proto`'s wrapper-erasure trick is correctly undone by `UnionExec`/`InterleaveExec::try_from_proto` re-inserting the wrapper via `try_new`, and that the decoded plan's emitted batches actually carry the coerced nullable schema, not just its EXPLAIN string.
I wonder if instead of using an entirely new exec, we could use the existing ProjectionExec operator with Cast exprs? In theory it should already be capable of doing the schema tranformation (assuming the physical CastExpr can have a field -- not just a DataType) Thank you for follow up on this @dariocurr and @kosiew |
Per alamb's suggestion on apache#24094: CastExpr::new_with_target_field already lets a cast carry an explicit target Field (not just a DataType), and the cast kernel has a same-type fast path (Arc::clone, no data copy), so a nullability-only coercion is just a same-type cast. UnionExec/InterleaveExec now build a ProjectionExec with a CastExpr (or a plain Column when a leg's field already matches exactly) instead of a hand-rolled ExecutionPlan. This deletes the hand-rolled node's ~150 lines of trait boilerplate (statistics/pushdown/proto plumbing) and, since ProjectionExec has an ordinary protobuf message, removes the wrapper-erasure trick entirely -- there's no more invisible reinsertion on proto decode to reason about. It also gets the existing projection-collapsing optimizer pass for free: when a coerced leg's own top node is already a ProjectionExec, the two fuse into one instead of stacking. Also fixes two narrow gaps this surfaced in ProjectionExec's statistics propagation (datafusion-physical-expr's project_column_statistics_through_expr): a CastExpr whose source values are already of the target DataType is a value-preserving relabeling, so unlike a real type-changing cast, sum_value and byte_size should carry over unchanged rather than degrading to Absent.
|
Made this change. A few side effects:
|
# Conflicts: # datafusion/proto/tests/cases/roundtrip_physical_plan.rs
|
I merged up to resolve some conflicts |
alamb
left a comment
There was a problem hiding this comment.
Thank you @dariocurr and @kosiew
…apache#24094) ## Which issue does this PR close? Follow-up to apache#23861 (issue apache#15394). Not closing a new issue. ## Rationale for this change apache#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 apache#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. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Follow-up to #23861 (issue #15394). Not closing a new issue.
Rationale for this change
#23861 fixed
UNION ALLbatches carrying the wrong nullability when one legis
NOT NULLand another isn't, by re-stamping each batch's schema insideUnionExec/InterleaveExec's ownexecute(). In review, @alamb noted: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?
CoerceSchemaExec, a single-child passthroughExecutionPlannode.UnionExec::try_new/InterleaveExec::try_newinsert it above any childwhose own output schema disagrees with the computed union schema (in
practice, only nullability differs --
UnionExec::try_newalready rejectsreal data-type mismatches via
calculate_union).SchemaConformingStream) is unchanged;it just lives under
CoerceSchemaExec::execute()now instead of beingcalled directly from
UnionExec/InterleaveExec::execute().CoerceSchemaExecimplements the fullExecutionPlansurface a pure 1:1 passthrough needs to stay transparentto the optimizer: statistics passthrough, filter/limit pushdown,
benefits_from_input_partitioning() -> false(so it doesn't trigger aspurious repartition), and proto (de)serialization -- the node erases
itself on encode and is reconstructed by
try_newon decode, so noprotobuf schema change was needed.
rejected eagerly at plan-build time (via
EquivalenceProperties:: with_new_schema) rather than lazily atexecute().Are these changes tested?
test_union_partition_statistics_with_mismatched_nullabilityin
union.rs, proving statistics aren't poisoned toAbsentthrough thenew node.
union_nullable/union_nullable_spillregression tests fromfix: UnionExec now conforms each batch to the union's declared schema #23861 continue to pass unchanged.
sqllogictestgolden file (union.slt) whereEXPLAINoutput now shows the new node for pre-existing nullability-mismatched
UNION ALLcases.performance difference in either the coerced or matched-schema case
(differences were within run-to-run noise).
Are there any user-facing changes?
EXPLAINoutput for aUNION ALL/interleaved plan with a nullabilitymismatch across legs will now show a
CoerceSchemaExecnode that wasn'tthere before. No behavioral or correctness change.