You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2026/04/24 07:48:07 UTC
[PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
zhuqi-lucas opened a new pull request, #21827:
URL: https://github.com/apache/datafusion/pull/21827
## Which issue does this PR close?
Closes #21826
## Rationale for this change
`InterleaveExec::with_new_children` panics with an assertion error when children's output partitioning diverges after optimizer rewrites. This happens in practice when multiple physical optimizer passes modify the plan between when `InterleaveExec` is created and when `with_new_children` is called.
We hit this in production with a complex UNION ALL query (5 subqueries with JOINs across partitioned tables, single-file tables, and materialized views). Our custom optimizer chain runs `EnforceDistribution` twice — the first pass creates `InterleaveExec`, later optimizers modify children's partitioning, then the second pass triggers the assertion panic.
## What changes are included in this PR?
`InterleaveExec::with_new_children` now gracefully falls back to `UnionExec` when children are no longer interleavable, instead of panicking. A warning is logged when the fallback occurs.
Correctness is preserved — `EnforceDistribution` will add `RepartitionExec` as needed. Only the interleave optimization is lost.
## Are these changes tested?
Yes, two unit tests added:
- `test_interleave_fallback_to_union_on_partitioning_mismatch` — verifies fallback to UnionExec when one child's partitioning changes from Hash to RoundRobin
- `test_interleave_preserved_when_partitioning_matches` — verifies InterleaveExec is preserved when children's Hash partitioning remains consistent
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #21827:
URL: https://github.com/apache/datafusion/pull/21827#issuecomment-4326298274
> > I agree in principle, but in practice the optimizer rule never gets that chance. Here's the call chain:
>
> Thanks @zhuqi-lucas for this elaboration. For this `transform_up` style traversal, it seems we can only fall back. But I still think optimizer rewrites shouldn’t panic if an error occurs in the middle.
>
> > Falling back to UnionExec inside with_new_children is safe because UnionExec is always a valid replacement
>
> I have no doubt about this. My concern is that this fallback might be a bit aggressive. It could potentially hide bugs similar to #11536 by switch err to warn log, and it could also lead to optimization loss in some other scenarios where keeping InterleaveExec would be a better choice.
>
> Let’s wait to hear more opinions from other reviewers.
It makes sense, thanks @jonahgao for clarify, let's wait more reviewers to take a look.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #21827:
URL: https://github.com/apache/datafusion/pull/21827#issuecomment-4332168780
Thanks for the feedback @alamb @jonahgao. Makes sense — fixing this at the EnforceDistribution callsite is cleaner than silently falling back in with_new_children. We've addressed this internally in our fork for now. Will revisit if we find the right approach for EnforceDistribution.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #21827:
URL: https://github.com/apache/datafusion/pull/21827#discussion_r3149953966
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -612,13 +612,21 @@ impl ExecutionPlan for InterleaveExec {
self: Arc<Self>,
children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>> {
- // New children are no longer interleavable, which might be a bug of optimization rewrite.
- assert_or_internal_err!(
- can_interleave(children.iter()),
- "Can not create InterleaveExec: new children can not be interleaved"
- );
- check_if_same_properties!(self, children);
- Ok(Arc::new(InterleaveExec::try_new(children)?))
+ // After optimizer rewrites (e.g. join_selection changing join modes,
+ // or additional EnforceDistribution passes), children may no longer
+ // have consistent hash partitioning. Fall back to UnionExec instead
+ // of panicking — correctness is preserved, only the interleave
+ // optimization is lost.
+ if can_interleave(children.iter()) {
+ check_if_same_properties!(self, children);
+ Ok(Arc::new(InterleaveExec::try_new(children)?))
+ } else {
+ warn!(
Review Comment:
I agree with @jonahgao that flagging this with a warning only is non ideal because:
1. It will likely get ignored by many people
2. If someone sees this warning in their logs they can't do anything about it (it is likely a bug in DataFusion)
I would personally suggest:
1. Revert this change (so the error is returned)
2. At the callsite where we may change an InterleaveExec so it's chidlren are no longer interleavable (aka in EnforceDistribution) rewrite any InterleaveExec back to union
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #21827:
URL: https://github.com/apache/datafusion/pull/21827#discussion_r3136910403
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -612,13 +612,21 @@ impl ExecutionPlan for InterleaveExec {
self: Arc<Self>,
children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>> {
- // New children are no longer interleavable, which might be a bug of optimization rewrite.
Review Comment:
As this comment says, I think another solution is to address it in the optimizer.
This might be the root cause. Once identified, we can decide the best option: either avoiding certain rewrites on InterleaveExec or falling back to UnionExec.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #21827:
URL: https://github.com/apache/datafusion/pull/21827#issuecomment-4323945220
> `assert_or_internal_err` only returns an `internal_err`, and it seems like the optimizer didn't propagate the error correctly, causing a panic.
The error is propagated correctly — it bubbles up through `transform_up` and fails the entire optimizer rule. The problem is that `with_new_children` is called internally by `transform_up` during tree traversal, so the optimizer rule has no opportunity to catch the error and fall back. The error propagates all the way up and kills the optimization pass.
> I think it is appropriate to return an error here. It should be up to the user (e.g., the optimizer rule) to decide whether to fall back
I agree in principle, but in practice the optimizer rule never gets that chance. Here's the call chain:
```
optimizer_rule.optimize(plan)
→ plan.transform_up(closure)
→ node.with_new_children(modified_children) // error here
→ Err propagates up through transform_up
→ Err propagates up, entire rule fails
```
There's no hook point where the optimizer rule can intercept the error from `with_new_children` and decide to fall back. The `transform_up` machinery doesn't expose per-node error handling.
Falling back to `UnionExec` inside `with_new_children` is safe because `UnionExec` is always a valid replacement — it's semantically correct, just without the interleave optimization. `EnforceDistribution` will add `RepartitionExec` as needed downstream.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #21827:
URL: https://github.com/apache/datafusion/pull/21827#issuecomment-4323877292
> However, the assertion in `with_new_children` is still fragile — any `transform_up` based optimizer that changes a child's output partitioning will trigger a panic.
`assert_or_internal_err` only returns an `internal_err`, and it seems like the optimizer didn’t propagate the error correctly, causing a panic.
I think it is appropriate to return an error here. It should be up to the user (e.g., the optimizer rule) to decide whether to fall back or to preserve the partitioning properties of InterleaveExec’s children.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas closed pull request #21827: fix: InterleaveExec fallback to UnionExec when children partitioning diverges
URL: https://github.com/apache/datafusion/pull/21827
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: InterleaveExec fallback to UnionExec when children partitioning diverges [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #21827:
URL: https://github.com/apache/datafusion/pull/21827#issuecomment-4325549995
> I agree in principle, but in practice the optimizer rule never gets that chance. Here's the call chain:
Thanks @zhuqi-lucas for this elaboration. For this `transform_up` style traversal, it seems we can only fall back.
But I still think optimizer rewrites shouldn’t panic if an error occurs in the middle.
> Falling back to UnionExec inside with_new_children is safe because UnionExec is always a valid replacement
I have no doubt about this. My concern is that this fallback might be a bit aggressive. It could potentially hide bugs similar to #11536 by switch err to warn log, and it could also lead to optimization loss in some other scenarios where keeping InterleaveExec would be a better choice.
Let’s wait to hear more opinions from other reviewers.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org