ahshahid commented on code in PR #37824:
URL: https://github.com/apache/spark/pull/37824#discussion_r966162761
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Canonicalize.scala:
##########
@@ -65,6 +65,6 @@ object Canonicalize {
val newChildren = orderCommutative(l, { case Least(children) => children
})
Least(newChildren)
- case _ => e.withNewChildren(e.children.map(reorderCommutativeOperators))
+ case _ => e
Review Comment:
That should not be an issue. The children traversal is not needed, because
the precanonicalize on children would ensure that all the elements of subtree
have the precanonicalize invoked, which would mean that any commutative
expression's precanonicalize is also invoked, and for commutative expressions,
the precanonicalize & canonicalize are identical & involves reordering.
I will add tests for that too,.
Also I think now there is no need for precanonicalize variable at all.
whatever code is in precanonicalize can directly be assigned to canonicalize
variable.
The main issue was tree traversal at each level ( & mostly redundant)
caused by reorderCall. that is taken care of by not dwelving into children and
reorder is being called only for commutative expressions.
I will also measure the perf impact. that should not change.
That will simplify the code too.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
