You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/06/28 00:44:47 UTC
[GitHub] [spark] maropu commented on a change in pull request #32980: [SPARK-35829][SQL] Clean up evaluates subexpressions and add more flexibility to evaluate particular subexpressoin
maropu commented on a change in pull request #32980:
URL: https://github.com/apache/spark/pull/32980#discussion_r659400977
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
##########
@@ -1049,17 +1095,25 @@ class CodegenContext extends Logging {
// elimination.
val commonExprs = equivalentExpressions.getAllEquivalentExprs(1)
- val nonSplitExprCode = {
+ val nonSplitCode = {
+ val allStates = mutable.ArrayBuffer.empty[SubExprEliminationState]
commonExprs.map { exprs =>
- val eval = withSubExprEliminationExprs(localSubExprEliminationExprsForNonSplit.toMap) {
+ withSubExprEliminationExprs(localSubExprEliminationExprsForNonSplit.toMap) {
val eval = exprs.head.genCode(this)
- // Generate the code for this expression tree.
- val state = SubExprEliminationState(eval.isNull, eval.value)
+ // Collects other subexpressions from the children.
+ val childrenSubExprs = mutable.ArrayBuffer.empty[SubExprEliminationState]
+ exprs.head.foreach {
+ case e if subExprEliminationExprs.contains(e) =>
+ childrenSubExprs += subExprEliminationExprs(e)
+ case _ =>
+ }
+ val state = SubExprEliminationState(eval, childrenSubExprs.toSeq.reverse)
Review comment:
btw, how about moving `.reverse` into the SubExprEliminationState side if we always need to sort it;
```
object SubExprEliminationState {
def apply(eval: ExprCode, children: Seq[SubExprEliminationState]): SubExprEliminationState = {
new SubExprEliminationState(eval, children.reverse)
}
}
```
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
##########
@@ -1030,11 +1032,55 @@ class CodegenContext extends Logging {
}
/**
- * Checks and sets up the state and codegen for subexpression elimination. This finds the
- * common subexpressions, generates the code snippets that evaluate those expressions and
- * populates the mapping of common subexpressions to the generated code snippets. The generated
- * code snippets will be returned and should be inserted into generated codes before these
- * common subexpressions actually are used first time.
+ * Evaluates a sequence of `SubExprEliminationState` which represent subexpressions. After
+ * evaluating a subexpression, this method will clean up the code block to avoid duplicate
+ * evaluation.
+ */
+ def evaluateSubExprEliminationState(subExprStates: Iterable[SubExprEliminationState]): String = {
+ val code = new StringBuilder()
+
+ subExprStates.foreach { state =>
+ val currentCode = evaluateSubExprEliminationState(state.children) + "\n" + state.eval.code
+ code.append(currentCode + "\n")
+ state.eval.code = EmptyBlock
+ }
+
+ code.toString()
+ }
+
+ /**
+ * Checks and sets up the state and codegen for subexpression elimination in whole-stage codegen.
+ *
+ * This finds the common subexpressions, generates the code snippets that evaluate those
+ * expressions and populates the mapping of common subexpressions to the generated code snippets.
+ *
+ * The generated code snippet for subexpression is wrapped in `SubExprEliminationState`, which
+ * contains a `ExprCode` and the children `SubExprEliminationState` if any. The `ExprCode`
Review comment:
nit: `a ExprCode` -> `an ExprCode`
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
##########
@@ -1049,17 +1095,25 @@ class CodegenContext extends Logging {
// elimination.
val commonExprs = equivalentExpressions.getAllEquivalentExprs(1)
- val nonSplitExprCode = {
+ val nonSplitCode = {
+ val allStates = mutable.ArrayBuffer.empty[SubExprEliminationState]
commonExprs.map { exprs =>
- val eval = withSubExprEliminationExprs(localSubExprEliminationExprsForNonSplit.toMap) {
+ withSubExprEliminationExprs(localSubExprEliminationExprsForNonSplit.toMap) {
val eval = exprs.head.genCode(this)
- // Generate the code for this expression tree.
- val state = SubExprEliminationState(eval.isNull, eval.value)
+ // Collects other subexpressions from the children.
+ val childrenSubExprs = mutable.ArrayBuffer.empty[SubExprEliminationState]
+ exprs.head.foreach {
+ case e if subExprEliminationExprs.contains(e) =>
+ childrenSubExprs += subExprEliminationExprs(e)
+ case _ =>
+ }
+ val state = SubExprEliminationState(eval, childrenSubExprs.toSeq.reverse)
Review comment:
`childrenSubExprs.toSeq.reverse` -> `childrenSubExprs.reverse`?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
##########
@@ -1049,17 +1095,25 @@ class CodegenContext extends Logging {
// elimination.
val commonExprs = equivalentExpressions.getAllEquivalentExprs(1)
- val nonSplitExprCode = {
+ val nonSplitCode = {
+ val allStates = mutable.ArrayBuffer.empty[SubExprEliminationState]
commonExprs.map { exprs =>
- val eval = withSubExprEliminationExprs(localSubExprEliminationExprsForNonSplit.toMap) {
+ withSubExprEliminationExprs(localSubExprEliminationExprsForNonSplit.toMap) {
val eval = exprs.head.genCode(this)
- // Generate the code for this expression tree.
- val state = SubExprEliminationState(eval.isNull, eval.value)
+ // Collects other subexpressions from the children.
+ val childrenSubExprs = mutable.ArrayBuffer.empty[SubExprEliminationState]
+ exprs.head.foreach {
+ case e if subExprEliminationExprs.contains(e) =>
+ childrenSubExprs += subExprEliminationExprs(e)
Review comment:
Q: Is it difficult to add some tests for this new behaviour?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
##########
@@ -1030,11 +1032,55 @@ class CodegenContext extends Logging {
}
/**
- * Checks and sets up the state and codegen for subexpression elimination. This finds the
- * common subexpressions, generates the code snippets that evaluate those expressions and
- * populates the mapping of common subexpressions to the generated code snippets. The generated
- * code snippets will be returned and should be inserted into generated codes before these
- * common subexpressions actually are used first time.
+ * Evaluates a sequence of `SubExprEliminationState` which represent subexpressions. After
+ * evaluating a subexpression, this method will clean up the code block to avoid duplicate
+ * evaluation.
+ */
+ def evaluateSubExprEliminationState(subExprStates: Iterable[SubExprEliminationState]): String = {
Review comment:
nit: `Iterable` -> `Seq`?
--
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-unsubscribe@spark.apache.org
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