alamb commented on code in PR #4842:
URL: https://github.com/apache/arrow-datafusion/pull/4842#discussion_r1064014394
##########
datafusion/optimizer/src/optimizer.rs:
##########
@@ -309,17 +309,11 @@ impl Optimizer {
}
log_plan(&format!("Optimized plan (pass {i})"), &new_plan);
- // TODO this is an expensive way to see if the optimizer did
anything and
- // it would be better to change the OptimizerRule trait to return
an Option
- // instead
- let new_plan_str = format!("{}", new_plan.display_indent());
- if plan_str == new_plan_str {
+ if !plan_is_changed {
Review Comment:
I agree with @jackwener that that returning `Ok(Some(plan))` may not
actually have changed the plan
Perhaps we need to have a `impl PartialEq` for `LogicalPlan` that does the
correct deep equality check?
https://github.com/apache/arrow-datafusion/blob/3cc607de4ce6e9e1fd537091e471858c62f58653/datafusion/expr/src/logical_plan/plan.rs#L50
I remember at some point we tried to `#[derive(PartialEq)]` for
`LogicalPlan` but we hit some problem 🤔 maybe it has been resolved. Or maybe
we can manually derive `PartialEq` 🤔
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
