fix: normalize InterleaveExec to UnionExec before enforcing distribution - #24959
Conversation
…ution enforcement
|
@zhuqi-lucas WDYT about converting the interleave into a union like this? Are there concerns I'm missing? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24959 +/- ##
==========================================
- Coverage 81.62% 81.62% -0.01%
==========================================
Files 1124 1124
Lines 412462 412474 +12
Branches 412462 412474 +12
==========================================
- Hits 336684 336682 -2
- Misses 55965 55973 +8
- Partials 19813 19819 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zhuqi-lucas
left a comment
There was a problem hiding this comment.
Thanks @jayzhan211 — approach matches what #21827 landed on.
One scope note: the invariant itself is unchanged. On your branch, rebuilding an interleave whose children lost their partitioning (what any transform_up does) still fails:
Internal error: Assertion failed: can_interleave(children.iter())
EnsureRequirements is safe because Phase 0 strips interleaves before anything rebuilds them, but other rules aren't. JoinSelection runs at optimizer.rs:101, before this rule at :136, so the case reported in #21826 — a custom chain running EnforceDistribution twice, JoinSelection then walking an interleave the earlier pass built — isn't covered here. Same goes for the ~10 rules after :136 that rebuild the tree while an interleave is live.
Not blocking — the idempotency fix is worth having. Just maybe Part of #21826 rather than Closes.
|
Let me find another solution to close #21827 |
Which issue does this PR close?
Rationale for this change
EnsureRequirementsfails with an internal error when the plan it is givenalready contains an
InterleaveExecwhose children no longer share a hash(or range) partitioning:
InterleaveExecdeclares no distribution requirement of its own. It is onlyvalid while its children happen to share the same partitioning, which is why
ensure_distributioncreates it from aUnionExecin the first place. Onceit exists, several rewrites can take that partitioning away: this rule strips
RepartitionExecs that nothing requires, join key reordering changes the hashexpressions,
JoinSelectionside swaps change output partitioning, and so on.PlanContext::with_new_childrenrebuilds every parent from its updatedchildren while walking the tree, so the interleave is rebuilt over the changed
children before the rule's closure sees the node, and the rebuild fails.
This affects any pipeline that runs the rule more than once, re-optimizes a
deserialized plan, or runs
EnforceDistributionafter other rewrites (thesetup reported in #21826). It also breaks the idempotency the rule advertises.
The discussion on #21827 concluded that the fix belongs at the
EnforceDistributioncall site rather than as a silent fallback inInterleaveExec::with_new_children; this PR is that fix.What changes are included in this PR?
replace_interleave_with_unionhelper inensure_requirements/enforce_distribution.rs, which turns anInterleaveExecback into the equivalentUnionExec.EnsureRequirements::optimizeruns it as a top-down "Phase 0" before joinkey reordering. Phase 2 then re-derives interleaves from unions wherever the
children still qualify, exactly as it already re-derives
RepartitionExec,CoalescePartitionsExecandSortPreservingMergeExec.change its output partitioning and make the parent interleave fail to
rebuild.
A minimal reproducer that fails on
mainand passes here:What is the testing strategy for this PR?
Three new tests in
datafusion/core/tests/physical_optimizer/enforce_distribution.rs, each runthrough the existing multi-pass harness (
DISTRIB_DISTRIB_SORTandSORT_DISTRIB_DISTRIB) so idempotency is checked as well:interleave_falls_back_to_union_when_children_lose_partitioning: thereproducer above; the result is a
UnionExecover the scans.interleave_fallback_still_satisfies_parent_hash_requirement: same inputunder a
FinalPartitionedaggregate; the aggregate gets a single hashrepartition above the union.
existing_interleave_is_kept_when_children_stay_interleavable: aninterleave over hash partitioned aggregates is re-derived as an interleave,
and stays a union with
prefer_existing_union = true.The test harness's tree-node integrity block now applies the same pre-pass so
it mirrors the rule. Existing
union_to_interleaveandunion_not_to_interleavetests are unchanged, andunion.sltpasses.Are there any user-facing changes?
No API changes. One behavioral note: with
datafusion.optimizer.prefer_existing_union = true, anInterleaveExecpresent in the input plan is now emitted as a
UnionExec. The rule nevercreates interleaves under that setting, and plans produced by the default
pipeline are unaffected because
EnsureRequirementsis the only creator ofInterleaveExecand runs once.