mustafasrepo commented on code in PR #11536:
URL: https://github.com/apache/datafusion/pull/11536#discussion_r1684015535
##########
datafusion/physical-plan/src/union.rs:
##########
@@ -431,7 +431,12 @@ impl ExecutionPlan for InterleaveExec {
self: Arc<Self>,
children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>> {
- Ok(Arc::new(InterleaveExec::try_new(children)?))
+ // New children may not be able to interleave; in that case, we fall
back to UnionExec.
Review Comment:
> I see how this particular change is beneficial as now the plan runs where
it didn't before, so that is hard to argue with and I would be ok merging it in
>
> However, I feel like this might be silently masking some bug in the
planner. My personal preference would be to make this an error
>
> ```rust
> if !can_interleave(children.iter()) {
> return internal_err!("Can not create InterleaveExec: new children can
not be interleaved");
> }
> ```
>
> And then track down / fix whatever optimizer pass is causing the children
to no longer be interleavable
>
> FYI @mustafasrepo
This makes sense, invariant should be handled outside the operator during
planning. This checks the whether assumptions met. Thanks for this.
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
