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
cloud-fan 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 1 commit
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)
}
}
12 changes: 12 additions & 0 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 @@ -930,6 +930,18 @@ 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.
// We don't need to check `arrayCompactedMutableStates` here, as it results to array access
Copy link
Contributor

@mgaido91 mgaido91 Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

what if we declare a variable with the same name of an arrayCompactedMutableStates ? Let's say that we have:

public class Foo {
 private Object[] ourArray;
 // ....
 private void ourMethod() {
   Object[] ourArray = new Object[1];
   ourSplitFunction(ourArray);
 }
 private void ourSplitFunction(Object[] ourArray) {
  ourArray[0] = null;
 }
 // ...
}

Copy link
Contributor Author

@cloud-fan cloud-fan Dec 19, 2017

Choose a reason for hiding this comment

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

is there any place we would do that? ctx.addMutableState returns an array access code, I can't image a caller would extract the array name from it and use it as parameters...

Copy link
Contributor

@mgaido91 mgaido91 Dec 19, 2017

Choose a reason for hiding this comment

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

I don't think so, but currently there is also no place which creates the problem for which this assertion is being introduces. Of course this case is very very unlikely, but since we are introducing the check, I think that the effort to ensure also this very remote corner case is very low...

Copy link
Member

@kiszk kiszk Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is no place now, but there could be a place. If smj_value16 is replaced with an array element by ctx.addMutableState, the array element was passed.

This PR fixed this issue by eliminating usage of mutable state.

// code and will raise compile error if we use it in parameter list.
val mutableStateNames = inlinedMutableStates.map(_._2).toSet
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