viirya commented on a change in pull request #32699:
URL: https://github.com/apache/spark/pull/32699#discussion_r642314197
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
##########
@@ -1068,11 +1072,19 @@ class CodegenContext extends Logging {
}.unzip
val splitThreshold = SQLConf.get.methodSplitThreshold
- val codes = if (commonExprVals.map(_.code.length).sum > splitThreshold) {
+
+ val (codes, subExprsMap, exprCodes) = if
(nonSplitExprCode.map(_.length).sum > splitThreshold) {
if
(inputVarsForAllFuncs.map(calculateParamLengthFromExprValues).forall(isValidParamLength))
{
- commonExprs.zipWithIndex.map { case (exprs, i) =>
+ val localSubExprEliminationExprs =
+ mutable.HashMap.empty[Expression, SubExprEliminationState]
+
+ val splitCodes = commonExprs.zipWithIndex.map { case (exprs, i) =>
val expr = exprs.head
- val eval = commonExprVals(i)
+ val eval =
withSubExprEliminationExprs(localSubExprEliminationExprs.toMap) {
+ Seq(expr.genCode(this))
+ }.head
+
+ val value = addMutableState(javaType(expr.dataType), "subExprValue")
Review comment:
Use the example in the description to explain. For the two
subexpressions:
1. `simpleUDF($"id")`
2. `functions.length(simpleUDF($"id"))`
Previously we evaluate them independently, i.e.,
```
String subExpr1 = simpleUDF($"id");
Int subExpr2 = functions.length(simpleUDF($"id"));
```
Now we remove redundant evaluation of nested subexpressions:
```
String subExpr1 = simpleUDF($"id");
Int subExpr2 = functions.length(subExpr1);
```
If we need to split the functions, when we evaluate `functions.length`, it
needs access of `subExpr1`. We have two choices. One is to add `subExpr1` to
the function parameter list of the split function for `functions.length`.
Another one is to use mutable state.
To add it to parameter list will complicate the way we compute parameter
length. That's said we need to link nested subexpression relations and get the
correct parameters. Seems to me it is not worth doing that.
Currently I choose the simpler approach that is to use mutable state.
--
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
