maropu commented on a change in pull request #30245:
URL: https://github.com/apache/spark/pull/30245#discussion_r519766271
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/EquivalentExpressions.scala
##########
@@ -96,8 +151,9 @@ class EquivalentExpressions {
case other => other.children
}
- if (!skip && !addExpr(expr)) {
- childrenToRecurse.foreach(addExprTree)
+ if (!skip && !addFunc(expr)) {
+ childrenToRecurse.foreach(addExprTree(_, addFunc))
+
commonChildrenToRecurse(expr).filter(_.nonEmpty).foreach(addCommonExprs(_,
addFunc))
Review comment:
Seems like we can merge them like this?
```
// There are some special expressions that we should not recurse into
all of its children.
// 1. CodegenFallback: it's children will not be used to generate code
(call eval() instead)
// 2. If: common subexpressions will always be evaluated at the
beginning, but the true and
// false expressions in `If` may not get accessed, according to
the predicate
// expression. We should only recurse into the predicate
expression.
// 3. CaseWhen: like `If`, the children of `CaseWhen` only get
accessed in a certain
// condition. We should only recurse into the first
condition expression as it
// will always get accessed.
// 4. Coalesce: it's also a conditional expression, we should only
recurse into the first
// children, because others may not get accessed.
lazy val (childrenToRecurse, commonChildrenToRecurse) = expr match {
case _: CodegenFallback => (Nil, Nil)
case i: If => (i.predicate :: Nil, Seq(Seq(i.trueValue, i.falseValue)))
case c: CaseWhen =>
val (Seq(headCondition), otherConditions) =
c.branches.map(_._1).splitAt(1)
val values = c.branches.map(_._2) ++ c.elseValue
(headCondition :: Nil, Seq(otherConditions, values))
case c: Coalesce => (c.children.head :: Nil, Seq(c.children.tail))
case other => (other.children, Nil)
}
```
Then, I think its better to update the comment above for describing how to
handle these "special expressions (If, CaseWhen, and Coalesce)".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
