viirya commented on a change in pull request #33082:
URL: https://github.com/apache/spark/pull/33082#discussion_r659221432
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
##########
@@ -1766,19 +1766,20 @@ object CodeGenerator extends Logging {
argSet += JavaCode.variable(ctx.INPUT_ROW, classOf[InternalRow])
}
+ val stack = mutable.Stack[Expression](expr)
+
// Collects local variables from a given `expr` tree
- val collectLocalVariable = (ev: ExprValue) => ev match {
+ val collectLocalVariable = (ev: ExprValue, e: Expression) => ev match {
case vv: VariableValue => argSet += vv
- case _ =>
+ case _ => stack.pushAll(e.children)
}
- val stack = mutable.Stack[Expression](expr)
while (stack.nonEmpty) {
stack.pop() match {
case e if subExprs.contains(e) =>
val SubExprEliminationState(isNull, value) = subExprs(e)
- collectLocalVariable(value)
- collectLocalVariable(isNull)
+ collectLocalVariable(value, e)
+ collectLocalVariable(isNull, e)
Review comment:
Hmm, after second look, it looks not root cause. A subexpr should be
evaluated before using it.
So technically all its children expressions are not needed.
I found the root cause. `PromotePrecision` overwrites `Expression.genCode`
where is subexpression replace happens. So `PromotePrecision` skips
subexpression elimination. I will submit another PR and include your test as
co-author there. Thanks.
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
