Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Comment thread
cloud-fan marked this conversation as resolved.
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit hard to understand. what's the difference between SubExprEliminationState here and in states?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param states Foreach expression that is participating in subexpression elimination,
* the state to use.
* @param exprCodesNeedEvaluate Some expression codes that need to be evaluated before
* calling common subexpressions.
*/
case class SubExprCodes(
codes: Seq[String],
codes: Seq[SubExprEliminationState],
states: Map[Expression, SubExprEliminationState],
exprCodesNeedEvaluate: Seq[ExprCode])

Expand Down Expand Up @@ -1029,6 +1040,23 @@ class CodegenContext extends Logging {
genCodes
}

/**
* 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: Seq[SubExprEliminationState]): String = {
val code = new StringBuilder()

subExprStates.foreach { state =>
val currentCode = evaluateSubExprEliminationState(state.childrenSubExprs) + "\n" + state.code
code.append(currentCode + "\n")
state.code = EmptyBlock
}

code.toString()
}

/**
* 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
Expand All @@ -1049,17 +1077,26 @@ 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) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add some comments to explain the assumption: this code works because EquivalentExpressions returns child expressions first.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW collecting child expressions here looks really inefficient, but I don't have a better idea for now ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This is not general expression but special (subexpr) ones, so we don't do collecting child expressions in general but in limited range. Except that if you have many subexpr and they are highly nested.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add some comments to explain the assumption: this code works because EquivalentExpressions returns child expressions first.

As I commented before, I plan to remove the sorting. A better idea is to add SubExprEliminationState first into the map (not codegen yet). Then during codegen, we can look at the map to chain children.

childrenSubExprs += subExprEliminationExprs(e)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Is it difficult to add some tests for this new behaviour?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me add a few tests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new test.

case _ =>
}
val state = SubExprEliminationState(eval.code, eval.isNull, eval.value,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems it's simpler if we define SubExprEliminationState as SubExprEliminationState(eval: ExprValue, children: ...)

@viirya viirya Jun 21, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean SubExprEliminationState(eval: ExprCode, children: ...)?

childrenSubExprs.toSeq.reverse)
exprs.foreach(localSubExprEliminationExprsForNonSplit.put(_, state))
allStates += state
Seq(eval)
}.head
eval.code.toString
}
}
allStates.toSeq
}

// For some operators, they do not require all its child's outputs to be evaluated in advance.
Expand All @@ -1071,9 +1108,8 @@ class CodegenContext extends Logging {
(inputVars.toSeq, exprCodes.toSeq)
}.unzip

val splitThreshold = SQLConf.get.methodSplitThreshold

val (codes, subExprsMap, exprCodes) = if (nonSplitExprCode.map(_.length).sum > splitThreshold) {
val needSplit = nonSplitCode.map(_.code.length).sum > SQLConf.get.methodSplitThreshold
val (codes, subExprsMap, exprCodes) = if (needSplit) {
if (inputVarsForAllFuncs.map(calculateParamLengthFromExprValues).forall(isValidParamLength)) {
val localSubExprEliminationExprs =
mutable.HashMap.empty[Expression, SubExprEliminationState]
Expand Down Expand Up @@ -1111,22 +1147,32 @@ class CodegenContext extends Logging {
|}
""".stripMargin

val state = SubExprEliminationState(isNull, JavaCode.global(value, expr.dataType))
exprs.foreach(localSubExprEliminationExprs.put(_, state))
// 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 inputVariables = inputVars.map(_.variableName).mkString(", ")
s"${addNewFunction(fnName, fn)}($inputVariables);"
val code = code"${addNewFunction(fnName, fn)}($inputVariables);"
val state = SubExprEliminationState(code, isNull, JavaCode.global(value, expr.dataType),
childrenSubExprs.toSeq.reverse)
exprs.foreach(localSubExprEliminationExprs.put(_, state))
state
}
(splitCodes, localSubExprEliminationExprs, exprCodesNeedEvaluate)
} else {
if (Utils.isTesting) {
throw QueryExecutionErrors.failedSplitSubExpressionError(MAX_JVM_METHOD_PARAMS_LENGTH)
} else {
logInfo(QueryExecutionErrors.failedSplitSubExpressionMsg(MAX_JVM_METHOD_PARAMS_LENGTH))
(nonSplitExprCode, localSubExprEliminationExprsForNonSplit, Seq.empty)
(nonSplitCode, localSubExprEliminationExprsForNonSplit, Seq.empty)
}
}
} else {
(nonSplitExprCode, localSubExprEliminationExprsForNonSplit, Seq.empty)
(nonSplitCode, localSubExprEliminationExprsForNonSplit, Seq.empty)
}
SubExprCodes(codes, subExprsMap.toMap, exprCodes.flatten)
}
Comment thread
cloud-fan marked this conversation as resolved.
Expand Down Expand Up @@ -1174,8 +1220,10 @@ class CodegenContext extends Logging {
// Currently, we will do this for all non-leaf only expression trees (i.e. expr trees with
// at least two nodes) as the cost of doing it is expected to be low.

val subExprCode = s"${addNewFunction(fnName, fn)}($INPUT_ROW);"
subexprFunctions += s"${addNewFunction(fnName, fn)}($INPUT_ROW);"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: shall we use subexprFunctions += subExprCode here? otherwise we are calling addNewFunction twice.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, as the functions in class is a map, it will overwrite. But yes, we should use subExprCode. Let me submit a followup.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val state = SubExprEliminationState(
code"$subExprCode",
JavaCode.isNullGlobal(isNull),
JavaCode.global(value, expr.dataType))
subExprEliminationExprs ++= e.map(_ -> state).toMap
Expand Down Expand Up @@ -1776,7 +1824,7 @@ object CodeGenerator extends Logging {
while (stack.nonEmpty) {
stack.pop() match {
case e if subExprs.contains(e) =>
val SubExprEliminationState(isNull, value) = subExprs(e)
val SubExprEliminationState(_, isNull, value, _) = subExprs(e)
collectLocalVariable(value)
collectLocalVariable(isNull)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,7 @@ class CodeGenerationSuite extends SparkFunSuite with ExpressionEvalHelper {
val add1 = Add(ref, ref)
val add2 = Add(add1, add1)
val dummy = SubExprEliminationState(
EmptyBlock,
JavaCode.variable("dummy", BooleanType),
JavaCode.variable("dummy", BooleanType))

Expand All @@ -471,7 +472,7 @@ class CodeGenerationSuite extends SparkFunSuite with ExpressionEvalHelper {
val ctx = new CodegenContext
val e = ref.genCode(ctx)
// before
ctx.subExprEliminationExprs += ref -> SubExprEliminationState(e.isNull, e.value)
ctx.subExprEliminationExprs += ref -> SubExprEliminationState(EmptyBlock, e.isNull, e.value)
assert(ctx.subExprEliminationExprs.contains(ref))
// call withSubExprEliminationExprs
ctx.withSubExprEliminationExprs(Map(add1 -> dummy)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ class SubexpressionEliminationSuite extends SparkFunSuite with ExpressionEvalHel
ctx.withSubExprEliminationExprs(subExprs.states) {
exprs.map(_.genCode(ctx))
}
val subExprsCode = subExprs.codes.mkString("\n")
val subExprsCode = ctx.evaluateSubExprEliminationState(subExprs.codes)

val codeBody = s"""
public java.lang.Object generate(Object[] references) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ case class HashAggregateExec(
bindReferences(updateExprsForOneFunc, inputAttrs)
}
val subExprs = ctx.subexpressionEliminationForWholeStageCodegen(boundUpdateExprs.flatten)
val effectiveCodes = subExprs.codes.mkString("\n")
val effectiveCodes = ctx.evaluateSubExprEliminationState(subExprs.codes)
val bufferEvals = boundUpdateExprs.map { boundUpdateExprsForOneFunc =>
ctx.withSubExprEliminationExprs(subExprs.states) {
boundUpdateExprsForOneFunc.map(_.genCode(ctx))
Expand Down Expand Up @@ -989,7 +989,7 @@ case class HashAggregateExec(
bindReferences(updateExprsForOneFunc, inputAttrs)
}
val subExprs = ctx.subexpressionEliminationForWholeStageCodegen(boundUpdateExprs.flatten)
val effectiveCodes = subExprs.codes.mkString("\n")
val effectiveCodes = ctx.evaluateSubExprEliminationState(subExprs.codes)
val unsafeRowBufferEvals = boundUpdateExprs.map { boundUpdateExprsForOneFunc =>
ctx.withSubExprEliminationExprs(subExprs.states) {
boundUpdateExprsForOneFunc.map(_.genCode(ctx))
Expand Down Expand Up @@ -1035,7 +1035,7 @@ case class HashAggregateExec(
bindReferences(updateExprsForOneFunc, inputAttrs)
}
val subExprs = ctx.subexpressionEliminationForWholeStageCodegen(boundUpdateExprs.flatten)
val effectiveCodes = subExprs.codes.mkString("\n")
val effectiveCodes = ctx.evaluateSubExprEliminationState(subExprs.codes)
val fastRowEvals = boundUpdateExprs.map { boundUpdateExprsForOneFunc =>
ctx.withSubExprEliminationExprs(subExprs.states) {
boundUpdateExprsForOneFunc.map(_.genCode(ctx))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ case class ProjectExec(projectList: Seq[NamedExpression], child: SparkPlan)
val genVars = ctx.withSubExprEliminationExprs(subExprs.states) {
exprs.map(_.genCode(ctx))
}
(subExprs.codes.mkString("\n"), genVars, subExprs.exprCodesNeedEvaluate)
(ctx.evaluateSubExprEliminationState(subExprs.codes), genVars, subExprs.exprCodesNeedEvaluate)
} else {
("", exprs.map(_.genCode(ctx)), Seq.empty)
}
Expand Down