[GitHub] [spark] viirya commented on a change in pull request #32980: [SPARK-35829][SQL] Clean up evaluates subexpressions and add more flexibility to evaluate particular subexpressoin

[GitBox <gi...@apache.org>]

viirya commented on a change in pull request #32980:
URL: https://github.com/apache/spark/pull/32980#discussion_r655164773



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
##########
@@ -76,24 +76,35 @@ object ExprCode {
 /**
  * State used for subexpression elimination.
  *
+ * @param code The sequence of statements required to evaluate the subexpression.
  * @param isNull A term that holds a boolean value representing whether the expression evaluated
  *               to null.
  * @param value A term for a value of a common sub-expression. Not valid if `isNull`
  *              is set to `true`.
+ * @param childrenSubExprs The sequence of subexpressions as the children expressions. Before
+ *                         evaluating this subexpression, we should evaluate all children
+ *                         subexpressions first. This is used if we want to selectively evaluate
+ *                         particular subexpressions, instead of all at once. In the case, we need
+ *                         to make sure we evaluate all children subexpressions too.
  */
-case class SubExprEliminationState(isNull: ExprValue, value: ExprValue)
+case class SubExprEliminationState(
+  var code: Block,
+  isNull: ExprValue,
+  value: ExprValue,
+  childrenSubExprs: Seq[SubExprEliminationState] = Seq.empty)
 
 /**
  * Codes and common subexpressions mapping used for subexpression elimination.
  *
- * @param codes Strings representing the codes that evaluate common subexpressions.
+ * @param codes all `SubExprEliminationState` representing the codes that evaluate common
+ *              subexpressions.

Review comment:
       The're the same `SubExprEliminationState`. `states` is used as map when we look for subexpressions to replace in an expression. `codes` are all values in the map, and they are in the sequence when we create them.
   
   Now I'm thinking it more, maybe we don't need to keep the sequence (`codes`). As this PR cleans up child subexpressions during evaluation. The order of evaluation seems not important anymore.




-- 
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org