Skip to content

[SPARK-12018][SQL] Refactor common subexpression elimination code #10009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
viirya wants to merge 4 commits into from
Closed

[SPARK-12018][SQL] Refactor common subexpression elimination code #10009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4876ce0
Refactor common subexpression elimination.
viirya Nov 26, 2015
b3cf6a8
Remove unnecessary variables.
viirya Nov 27, 2015
8e937d3
For comments.
viirya Nov 27, 2015
b162ba5
For comments.
viirya Nov 27, 2015
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
10 changes: 3 additions & 7 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,9 @@ abstract class Expression extends TreeNode[Expression] {
def gen(ctx: CodeGenContext): GeneratedExpressionCode = {
ctx.subExprEliminationExprs.get(this).map { subExprState =>
// This expression is repeated meaning the code to evaluated has already been added
// as a function, `subExprState.fnName`. Just call that.
val code =
s"""
|/* $this */
|${subExprState.fnName}(${ctx.INPUT_ROW});
""".stripMargin.trim
GeneratedExpressionCode(code, subExprState.code.isNull, subExprState.code.value)
// as a function and called in advance. Just use it.
val code = s"/* $this */"
GeneratedExpressionCode(code, subExprState.isNull, subExprState.value)
}.getOrElse {
val isNull = ctx.freshName("isNull")
val primitive = ctx.freshName("primitive")
Expand Down
34 changes: 9 additions & 25 deletions ...lyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,16 +104,13 @@ class CodeGenContext {
val equivalentExpressions: EquivalentExpressions = new EquivalentExpressions

// State used for subexpression elimination.
case class SubExprEliminationState(
isLoaded: String,
code: GeneratedExpressionCode,
fnName: String)
case class SubExprEliminationState(isNull: String, value: String)

// Foreach expression that is participating in subexpression elimination, the state to use.
val subExprEliminationExprs = mutable.HashMap.empty[Expression, SubExprEliminationState]

// The collection of isLoaded variables that need to be reset on each row.
val subExprIsLoadedVariables = mutable.ArrayBuffer.empty[String]
// The collection of sub-exression result resetting methods that need to be called on each row.
val subExprResetVariables = mutable.ArrayBuffer.empty[String]

final val JAVA_BOOLEAN = "boolean"
final val JAVA_BYTE = "byte"
Expand Down Expand Up @@ -408,7 +405,6 @@ class CodeGenContext {
val commonExprs = equivalentExpressions.getAllEquivalentExprs.filter(_.size > 1)
commonExprs.foreach(e => {
val expr = e.head
val isLoaded = freshName("isLoaded")
val isNull = freshName("isNull")
val value = freshName("value")
val fnName = freshName("evalExpr")
Expand All @@ -417,18 +413,12 @@ class CodeGenContext {
val code = expr.gen(this)
val fn =
s"""
|private void $fnName(InternalRow ${INPUT_ROW}) {
| if (!$isLoaded) {
| ${code.code.trim}
| $isLoaded = true;
| $isNull = ${code.isNull};
| $value = ${code.value};
| }
|private void $fnName(InternalRow $INPUT_ROW) {
| ${code.code.trim}
| $isNull = ${code.isNull};
| $value = ${code.value};
|}
""".stripMargin
Copy link
Contributor

@nongli nongli Nov 30, 2015

Choose a reason for hiding this comment

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

Am I right in saying you've picked a different approach to do this? You removed isLoaded and just load it every time. I think this is scary because it does not allow shortcircuiting. This can be bad if it is expr1 AND expr2 and expr1 is selective or expr2 is expensive.

Copy link
Member Author

@viirya viirya Nov 30, 2015

Choose a reason for hiding this comment

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

I don't understand your point. $fnName will be called only once for each row. This patch doesn't change that.

Copy link
Contributor

@nongli nongli Nov 30, 2015

Choose a reason for hiding this comment

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

Nvm, what I said doesn't apply for this operator.

I was thinking for Filter you wouldn't want to call fnName once for each row. It can be called less than once.

LGTM

code.code = fn
code.isNull = isNull
code.value = value

addNewFunction(fnName, fn)

Expand All @@ -448,18 +438,12 @@ class CodeGenContext {
// 2. Less code.
// 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.

// Maintain the loaded value and isNull as member variables. This is necessary if the codegen
// function is split across multiple functions.
// TODO: maintaining this as a local variable probably allows the compiler to do better
// optimizations.
addMutableState("boolean", isLoaded, s"$isLoaded = false;")
addMutableState("boolean", isNull, s"$isNull = false;")
addMutableState(javaType(expr.dataType), value,
s"$value = ${defaultValue(expr.dataType)};")
subExprIsLoadedVariables += isLoaded

val state = SubExprEliminationState(isLoaded, code, fnName)
subExprResetVariables += s"$fnName($INPUT_ROW);"
val state = SubExprEliminationState(isNull, value)
e.foreach(subExprEliminationExprs.put(_, state))
})
}
Expand Down
4 changes: 2 additions & 2 deletions ...in/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ object GenerateUnsafeProjection extends CodeGenerator[Seq[Expression], UnsafePro
val holderClass = classOf[BufferHolder].getName
ctx.addMutableState(holderClass, bufferHolder, s"this.$bufferHolder = new $holderClass();")

// Reset the isLoaded flag for each row.
val subexprReset = ctx.subExprIsLoadedVariables.map { v => s"${v} = false;" }.mkString("\n")
// Reset the subexpression values for each row.
val subexprReset = ctx.subExprResetVariables.mkString("\n")

val code =
s"""
Expand Down