attilapiros commented on code in PR #37824:
URL: https://github.com/apache/spark/pull/37824#discussion_r965247849
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -563,7 +564,8 @@ case class Subtract(
case class Multiply(
left: Expression,
right: Expression,
- evalMode: EvalMode.Value = EvalMode.fromSQLConf(SQLConf.get)) extends
BinaryArithmetic {
+ evalMode: EvalMode.Value = EvalMode.fromSQLConf(SQLConf.get)) extends
BinaryArithmetic with
Review Comment:
Nit: indentation the `with` should be moved to the next line
##########
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:
I have some doubts regarding this change.
What about a tree where a commutative child has a non-commutative parent
which is not listed above for example `abs('a + 'b)`? As with this change the
recursive call stops at `abs`...
can you extend the test cases which such an example?
--
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]
