fix transforming LogicalPlan::Explain use TreeNode::transform fails
#8400
Conversation
| @@ -877,19 +877,20 @@ impl LogicalPlan { | |||
| input: Arc::new(inputs[0].clone()), | |||
| })) | |||
| } | |||
| LogicalPlan::Explain(_) => { | |||
| LogicalPlan::Explain(e) => { | |||
There was a problem hiding this comment.
I find we can correct transform LogicalPlan::Analyze in below
https://github.com/apache/arrow-datafusion/blob/340ecfdfe0e6667d2c9f528a60d8ee7fa5c34805/datafusion/expr/src/logical_plan/plan.rs#L871-L878
so I used the same approach in LogicalPlan::Explain
|
cc @trueleo , does this fix work for you? |
|
@haohuaijin Thank you so much for the fix. I'll test this out. (edit: This is working for the example code i presented in the issue) I am still bit puzzled about the comment which was there. Maybe @alamb can confirm that this change is not breaking anything else. |
There was a problem hiding this comment.
Thank you @haohuaijin -- this makes sense to me and I think it is a nice fix 👏
I am still bit puzzled about the comment which was there.
// Explain should be handled specially in the optimizers; // If this check cannot pass it means some optimizer pass is // trying to optimize Explain directlyMaybe @alamb can confirm that this change is not breaking anything else.
I think this comment is from a very long time ago when the optimizers were structured quite differently than they are today.
The change in this PR I think is correct and make sense to me
| } | ||
|
|
||
| Ok(self.clone()) | ||
| assert!( |
There was a problem hiding this comment.
I recommend we make these checks return internal_err (so they error, rather than panic in case of bug), but this is consistent with the rest of the code in this file, so this is good
Co-authored-by: Andrew Lamb <[email protected]>
I think i get the issue now. When running optimizer code there is matching on Explain so that the child plan of explain is optimized and output from the optimizations are then pushed to Explain's Thus transforming explain on it's own cannot make any changes to stringified_plans. It is left upto the caller to make the right decision. |
…ls (apache#8400) * fix transforming LogicalPlan::Explain use TreeNode::transform fails * Update datafusion/expr/src/logical_plan/plan.rs Co-authored-by: Andrew Lamb <[email protected]> --------- Co-authored-by: Andrew Lamb <[email protected]>
Which issue does this PR close?
Closes #8396
Rationale for this change
What changes are included in this PR?
make
LogicalPlan::Explaincan transform useTreeNode::transformAre these changes tested?
yes, add one test
Are there any user-facing changes?