You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "haohuaijin (via GitHub)" <gi...@apache.org> on 2023/12/02 10:23:58 UTC
[PR] fix transforming `LogicalPlan::Explain` use `TreeNode::transform` fails [arrow-datafusion]
haohuaijin opened a new pull request, #8400:
URL: https://github.com/apache/arrow-datafusion/pull/8400
## Which issue does this PR close?
Closes #8396
## Rationale for this change
## What changes are included in this PR?
make `LogicalPlan::Explain` can transform use `TreeNode::transform`
## Are these changes tested?
yes, add one test
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix transforming `LogicalPlan::Explain` use `TreeNode::transform` fails [arrow-datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #8400:
URL: https://github.com/apache/arrow-datafusion/pull/8400#issuecomment-1837117706
cc @trueleo , does this fix work for you?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix transforming `LogicalPlan::Explain` use `TreeNode::transform` fails [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8400:
URL: https://github.com/apache/arrow-datafusion/pull/8400
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix transforming `LogicalPlan::Explain` use `TreeNode::transform` fails [arrow-datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #8400:
URL: https://github.com/apache/arrow-datafusion/pull/8400#discussion_r1412778322
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -877,19 +877,20 @@ impl LogicalPlan {
input: Arc::new(inputs[0].clone()),
}))
}
- LogicalPlan::Explain(_) => {
+ LogicalPlan::Explain(e) => {
Review 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`
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix transforming `LogicalPlan::Explain` use `TreeNode::transform` fails [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8400:
URL: https://github.com/apache/arrow-datafusion/pull/8400#discussion_r1413068716
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -877,19 +877,20 @@ impl LogicalPlan {
input: Arc::new(inputs[0].clone()),
}))
}
- LogicalPlan::Explain(_) => {
+ LogicalPlan::Explain(e) => {
// Explain should be handled specially in the optimizers;
- // If this check cannot pass it means some optimizer pass is
- // trying to optimize Explain directly
- if expr.is_empty() {
- return plan_err!("Invalid EXPLAIN command. Expression is empty");
- }
-
- if inputs.is_empty() {
- return plan_err!("Invalid EXPLAIN command. Inputs are empty");
- }
-
- Ok(self.clone())
+ assert!(
Review Comment:
I recommend we make these checks return internal_err (so they error, rather than panic in case of bug), but I also think we could do this as a follow on PR
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -877,19 +877,20 @@ impl LogicalPlan {
input: Arc::new(inputs[0].clone()),
}))
}
- LogicalPlan::Explain(_) => {
+ LogicalPlan::Explain(e) => {
// Explain should be handled specially in the optimizers;
Review Comment:
```suggestion
```
I think this comment is entirely outdated now so we could remove it
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix transforming `LogicalPlan::Explain` use `TreeNode::transform` fails [arrow-datafusion]
Posted by "trueleo (via GitHub)" <gi...@apache.org>.
trueleo commented on PR #8400:
URL: https://github.com/apache/arrow-datafusion/pull/8400#issuecomment-1837119880
@haohuaijin Thank you so much for the fix. I'll test this out. Fix seemed simple to me but i got confused about the comment
```
// Explain should be handled specially in the optimizers;
// If this check cannot pass it means some optimizer pass is
// trying to optimize Explain directly
```
Maybe @alamb can confirm that this change is not breaking anything else.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix transforming `LogicalPlan::Explain` use `TreeNode::transform` fails [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8400:
URL: https://github.com/apache/arrow-datafusion/pull/8400#discussion_r1413068716
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -877,19 +877,20 @@ impl LogicalPlan {
input: Arc::new(inputs[0].clone()),
}))
}
- LogicalPlan::Explain(_) => {
+ LogicalPlan::Explain(e) => {
// Explain should be handled specially in the optimizers;
- // If this check cannot pass it means some optimizer pass is
- // trying to optimize Explain directly
- if expr.is_empty() {
- return plan_err!("Invalid EXPLAIN command. Expression is empty");
- }
-
- if inputs.is_empty() {
- return plan_err!("Invalid EXPLAIN command. Inputs are empty");
- }
-
- Ok(self.clone())
+ assert!(
Review 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
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix transforming `LogicalPlan::Explain` use `TreeNode::transform` fails [arrow-datafusion]
Posted by "trueleo (via GitHub)" <gi...@apache.org>.
trueleo commented on PR #8400:
URL: https://github.com/apache/arrow-datafusion/pull/8400#issuecomment-1837918437
> 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
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 of an optimization is then added to Explain's `stringified_plans`.
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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org