Skip to content

[SPARK-22668][SQL] Ensure no global variables in arguments of method split by CodegenContext.splitExpressions() #20021

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
wants to merge 5 commits into from
Closed

[SPARK-22668][SQL] Ensure no global variables in arguments of method split by CodegenContext.splitExpressions() #20021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18ec598
Ensure no global variables in arguments of method split by CodegenCon…
cloud-fan Dec 19, 2017
f1afb92
fix stack
cloud-fan Dec 19, 2017
3d44195
address comments
cloud-fan Dec 20, 2017
95d0c1b
Merge remote-tracking branch 'origin/master' into codegen
cloud-fan Dec 21, 2017
900f246
only check in test
cloud-fan Dec 21, 2017
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
18 changes: 8 additions & 10 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,13 +602,13 @@ case class Least(children: Seq[Expression]) extends Expression {

override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
val tmpIsNull = ctx.addMutableState(ctx.JAVA_BOOLEAN, "leastTmpIsNull")
ev.isNull = ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
val evals = evalChildren.map(eval =>
s"""
|${eval.code}
|if (!${eval.isNull} && ($tmpIsNull ||
|if (!${eval.isNull} && (${ev.isNull} ||
| ${ctx.genGreater(dataType, ev.value, eval.value)})) {
| $tmpIsNull = false;
| ${ev.isNull} = false;
| ${ev.value} = ${eval.value};
|}
""".stripMargin
Expand All @@ -628,10 +628,9 @@ case class Least(children: Seq[Expression]) extends Expression {
foldFunctions = _.map(funcCall => s"${ev.value} = $funcCall;").mkString("\n"))
ev.copy(code =
s"""
|$tmpIsNull = true;
|${ev.isNull} = true;
|${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
|$codes
|final boolean ${ev.isNull} = $tmpIsNull;
""".stripMargin)
}
}
Expand Down Expand Up @@ -682,13 +681,13 @@ case class Greatest(children: Seq[Expression]) extends Expression {

override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
val tmpIsNull = ctx.addMutableState(ctx.JAVA_BOOLEAN, "greatestTmpIsNull")
ev.isNull = ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
val evals = evalChildren.map(eval =>
s"""
|${eval.code}
|if (!${eval.isNull} && ($tmpIsNull ||
|if (!${eval.isNull} && (${ev.isNull} ||
| ${ctx.genGreater(dataType, eval.value, ev.value)})) {
| $tmpIsNull = false;
| ${ev.isNull} = false;
| ${ev.value} = ${eval.value};
|}
""".stripMargin
Expand All @@ -708,10 +707,9 @@ case class Greatest(children: Seq[Expression]) extends Expression {
foldFunctions = _.map(funcCall => s"${ev.value} = $funcCall;").mkString("\n"))
ev.copy(code =
s"""
|$tmpIsNull = true;
|${ev.isNull} = true;
|${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
|$codes
|final boolean ${ev.isNull} = $tmpIsNull;
""".stripMargin)
}
}
32 changes: 27 additions & 5 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 @@ -128,7 +128,7 @@ class CodegenContext {
* `currentVars` to null, or set `currentVars(i)` to null for certain columns, before calling
* `Expression.genCode`.
*/
final var INPUT_ROW = "i"
var INPUT_ROW = "i"

/**
* Holding a list of generated columns as input of current operator, will be used by
Expand All @@ -146,22 +146,30 @@ class CodegenContext {
* as a member variable
*
* They will be kept as member variables in generated classes like `SpecificProjection`.
*
* Exposed for tests only.
*/
val inlinedMutableStates: mutable.ArrayBuffer[(String, String)] =
private[catalyst] val inlinedMutableStates: mutable.ArrayBuffer[(String, String)] =
mutable.ArrayBuffer.empty[(String, String)]

/**
* The mapping between mutable state types and corrseponding compacted arrays.
* The keys are java type string. The values are [[MutableStateArrays]] which encapsulates
* the compacted arrays for the mutable states with the same java type.
*
* Exposed for tests only.
*/
val arrayCompactedMutableStates: mutable.Map[String, MutableStateArrays] =
private[catalyst] val arrayCompactedMutableStates: mutable.Map[String, MutableStateArrays] =
mutable.Map.empty[String, MutableStateArrays]

// An array holds the code that will initialize each state
val mutableStateInitCode: mutable.ArrayBuffer[String] =
// Exposed for tests only.
private[catalyst] val mutableStateInitCode: mutable.ArrayBuffer[String] =
mutable.ArrayBuffer.empty[String]

// Tracks the names of all the mutable states.
private val mutableStateNames: mutable.HashSet[String] = mutable.HashSet.empty

/**
* This class holds a set of names of mutableStateArrays that is used for compacting mutable
* states for a certain type, and holds the next available slot of the current compacted array.
Expand All @@ -172,7 +180,11 @@ class CodegenContext {

private[this] var currentIndex = 0

private def createNewArray() = arrayNames.append(freshName("mutableStateArray"))
private def createNewArray() = {
val newArrayName = freshName("mutableStateArray")
mutableStateNames += newArrayName
arrayNames.append(newArrayName)
}

def getCurrentIndex: Int = currentIndex

Expand Down Expand Up @@ -241,6 +253,7 @@ class CodegenContext {
val initCode = initFunc(varName)
inlinedMutableStates += ((javaType, varName))
mutableStateInitCode += initCode
mutableStateNames += varName
varName
} else {
val arrays = arrayCompactedMutableStates.getOrElseUpdate(javaType, new MutableStateArrays)
Expand Down Expand Up @@ -930,6 +943,15 @@ class CodegenContext {
// inline execution if only one block
blocks.head
} else {
if (Utils.isTesting) {
Copy link
Member

@viirya viirya Dec 20, 2017

Choose a reason for hiding this comment

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

Do we only do the assert in testing? Because passing global variables won't raise compile error, if we have any global variables passed in when not in testing, the codegen still work and may lead to wrong result.

Copy link
Contributor

@mgaido91 mgaido91 Dec 20, 2017

Choose a reason for hiding this comment

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

as you said, it may lead, but likely it doesn't. Then I do think that the best option is to assert it only in testing, where this might help finding potential bugs. In production it is an overkill to throw an exception for a situation which most likely is not a problem IMHO.

Copy link
Member

@kiszk kiszk Dec 20, 2017

Choose a reason for hiding this comment

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

Here is a discussion about this testing.

// Passing global variables to the split method is dangerous, as any mutating to it is
// ignored and may lead to unexpected behavior.
arguments.foreach { case (_, name) =>
assert(!mutableStateNames.contains(name),
s"split function argument $name cannot be a global variable.")
}
}

val func = freshName(funcName)
val argString = arguments.map { case (t, name) => s"$t $name" }.mkString(", ")
val functions = blocks.zipWithIndex.map { case (body, i) =>
Expand Down
8 changes: 3 additions & 5 deletions ...yst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ case class CaseWhen(
// It is initialized to `NOT_MATCHED`, and if it's set to `HAS_NULL` or `HAS_NONNULL`,
// We won't go on anymore on the computation.
val resultState = ctx.freshName("caseWhenResultState")
val tmpResult = ctx.addMutableState(ctx.javaType(dataType), "caseWhenTmpResult")
ev.value = ctx.addMutableState(ctx.javaType(dataType), ev.value)

// these blocks are meant to be inside a
// do {
Expand All @@ -205,7 +205,7 @@ case class CaseWhen(
|if (!${cond.isNull} && ${cond.value}) {
| ${res.code}
| $resultState = (byte)(${res.isNull} ? $HAS_NULL : $HAS_NONNULL);
| $tmpResult = ${res.value};
| ${ev.value} = ${res.value};
| continue;
|}
""".stripMargin
Expand All @@ -216,7 +216,7 @@ case class CaseWhen(
s"""
|${res.code}
|$resultState = (byte)(${res.isNull} ? $HAS_NULL : $HAS_NONNULL);
|$tmpResult = ${res.value};
|${ev.value} = ${res.value};
""".stripMargin
}

Expand Down Expand Up @@ -264,13 +264,11 @@ case class CaseWhen(
ev.copy(code =
s"""
|${ctx.JAVA_BYTE} $resultState = $NOT_MATCHED;
|$tmpResult = ${ctx.defaultValue(dataType)};
|do {
| $codes
|} while (false);
|// TRUE if any condition is met and the result is null, or no any condition is met.
|final boolean ${ev.isNull} = ($resultState != $HAS_NONNULL);
|final ${ctx.javaType(dataType)} ${ev.value} = $tmpResult;
""".stripMargin)
}
}
Expand Down
9 changes: 4 additions & 5 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,15 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
}

override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val tmpIsNull = ctx.addMutableState(ctx.JAVA_BOOLEAN, "coalesceTmpIsNull")
ev.isNull = ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)

// all the evals are meant to be in a do { ... } while (false); loop
val evals = children.map { e =>
val eval = e.genCode(ctx)
s"""
|${eval.code}
|if (!${eval.isNull}) {
| $tmpIsNull = false;
| ${ev.isNull} = false;
| ${ev.value} = ${eval.value};
| continue;
|}
Expand All @@ -103,7 +103,7 @@ case class Coalesce(children: Seq[Expression]) extends Expression {
foldFunctions = _.map { funcCall =>
s"""
|${ev.value} = $funcCall;
|if (!$tmpIsNull) {
|if (!${ev.isNull}) {
| continue;
|}
""".stripMargin
Expand All @@ -112,12 +112,11 @@ case class Coalesce(children: Seq[Expression]) extends Expression {

ev.copy(code =
s"""
|$tmpIsNull = true;
|${ev.isNull} = true;
|$resultType ${ev.value} = ${ctx.defaultValue(dataType)};
|do {
| $codes
|} while (false);
|final boolean ${ev.isNull} = $tmpIsNull;
""".stripMargin)
}
}
Expand Down
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ case class In(value: Expression, list: Seq[Expression]) extends Predicate {
|${valueGen.code}
|byte $tmpResult = $HAS_NULL;
|if (!${valueGen.isNull}) {
| $tmpResult = 0;
| $tmpResult = $NOT_MATCHED;
| $javaDataType $valueArg = ${valueGen.value};
| do {
| $codes
Expand Down