[SPARK-22705][SQL] Case, Coalesce, and In use less global variables #19901

kiszk · 2017-12-05T17:45:26Z

What changes were proposed in this pull request?

This PR accomplishes the following two items.

Reduce # of global variables from two to one for generated code of Case and Coalesce and remove global variables for generated code of In.
Make lifetime of global variable local within an operation

Item 1. reduces # of constant pool entries in a Java class. Item 2. ensures that an variable is not passed to arguments in a method split by CodegenContext.splitExpressions(), which is addressed by #19865.

How was this patch tested?

Added new tests into PredicateSuite, NullExpressionsSuite, and ConditionalExpressionSuite.

SparkQA · 2017-12-05T17:49:11Z

Test build #84497 has finished for PR 19901 at commit ae50d97.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-05T19:29:43Z

Test build #84504 has finished for PR 19901 at commit 8420c02.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-05T22:30:53Z

Test build #84505 has finished for PR 19901 at commit bcbe82c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2017-12-06T01:00:19Z

@cloud-fan @viirya @mgaido91 could you please review this?

cloud-fan · 2017-12-06T01:40:31Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

    val conditionMet = ctx.freshName("caseWhenConditionMet")
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
+    val value = ctx.freshName("value")


caseWhenTmpResult?

cloud-fan · 2017-12-06T02:18:50Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

-    // It is initialized to `false` and it is set to `true` when the first condition which
-    // evaluates to `true` is met and therefore is not needed to go on anymore on the computation
+    // This variable represents whether the condition is met with true/false or not met.
+    // It is initialized to -1 and it is set to 0 or 1 when the condition which evaluates to


This variable represents whether the evaluated result is null or not. It's a byte value instead of boolean because it carries an extra information about if the case-when condition is met or not. It is initialized to `-1`, which means the condition is not met yet and the result is unknown. When the first condition is met, it is set to `1` if result is null, or `0` if result is not null. We won't go on anymore on the computation if it's set to `1` or `0`.

cloud-fan · 2017-12-06T02:20:52Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

+    // It is initialized to -1 and it is set to 0 or 1 when the condition which evaluates to
+    // `false` or `true` is met and therefore is not needed to go on anymore on the computation
    // of the following conditions.
    val conditionMet = ctx.freshName("caseWhenConditionMet")


nit: resultIsNull

cloud-fan · 2017-12-06T02:23:50Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

+          """
+      },
+      foldFunctions = { funcCalls =>
+        funcCalls.map { funcCall =>


can you keep the previous code style?

cloud-fan · 2017-12-06T02:24:51Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

-           |  $func
-           |} while (false);
-           |return $conditionMet;
-         """.stripMargin,


this multiline style is now the preferred one.

cloud-fan · 2017-12-06T02:26:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala

  override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
+    val isNull = ctx.freshName("isNull")


coalesceTmpIsNull

viirya · 2017-12-06T02:28:45Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

-      ${ev.isNull} = true;
-      ${ev.value} = ${ctx.defaultValue(dataType)};
-      ${ctx.JAVA_BOOLEAN} $conditionMet = false;
+      ${ctx.JAVA_BYTE} $conditionMet = -1;


Why turn it from boolean to byte?

cloud-fan · 2017-12-06T02:29:30Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala

-         """.stripMargin
-      }.mkString)
-
+      extraArguments = (ctx.javaType(dataType), ev.value) :: Nil,


we don't need to pass it as a parameter, just create a local variable at the beginning of the generated method:

${ctx.javaType(dataType)} ${ev.value} = default... do { $func } while (false); return ${ev.value};

cloud-fan · 2017-12-06T02:29:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala

+          """.stripMargin
+      },
+      foldFunctions = { funcCalls =>
+        funcCalls.map { funcCall =>


ditto, please keep the previous code style.

cloud-fan · 2017-12-06T02:30:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.value)
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
+    // inValue  -1:isNull, 0:false, 1:true
+    val inValue = ctx.freshName("value")


nit: inTmpResult?

// inValue -1 indicates at lease one expr in list is evaluated to null. 0 means no matches found. 1 means the expr in list matches the given value expr.

viirya · 2017-12-06T02:31:10Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

        $codes
-      } while (false);""")
+      } while (false);
+      boolean ${ev.isNull} = ($conditionMet != 0); // TRUE if -1 or 1


It looks weird that we have two possible values for TRUE.

I see. nvm.

Maybe // TRUE if any condition is met and the result is not null, or no any condition is met.

viirya · 2017-12-06T03:00:55Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

-         |if (!${ev.isNull}) {
+         |byte $inValue = (byte)(${valueGen.isNull} ? -1 : 0); // isNull or FALSE
+         |if ($inValue != -1) {
         |  $javaDataType $valueArg = ${valueGen.value};


Can't we just pass ${valueGen.value}? Why we need another local variable for it?

${valueGen.value} may have an constant 1.0D. It is not easy to pass it to an argument by using spilitExpressions().

SparkQA · 2017-12-06T08:05:02Z

Test build #84534 has finished for PR 19901 at commit c81d795.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-12-06T08:07:49Z

retest this please.

cloud-fan · 2017-12-06T08:15:08Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

+           """.stripMargin
+      },
+      foldFunctions = { funcCalls =>
+        funcCalls.map { funcCall =>


can you keep the previous code style? i.e. _.map { funcCall =>

cloud-fan · 2017-12-06T08:15:39Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

-         """.stripMargin
-      }.mkString)
+      returnType = ctx.JAVA_BYTE,
+      makeSplitFunction = {


ditto, makeSplitFunction = func =>

viirya · 2017-12-06T08:16:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

-         |${ev.value} = false;
-         |${ev.isNull} = ${valueGen.isNull};
-         |if (!${ev.isNull}) {
+         |// TRUE if any condition is met and the result is not null, or no any condition is met.


This suggested comment I think is for CaseWhen?

Oh , sorry for stupid mistake.

cloud-fan · 2017-12-06T08:16:36Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

+         |  $codes
+         |} while (false);
+         |boolean ${ev.isNull} = ($resultIsNull != 0); // TRUE if -1 or 1
+         |${ctx.javaType(dataType)} ${ev.value} = $tmpResult;


nit: final

cloud-fan · 2017-12-06T08:17:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala

  override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
+    val coalesceTmpIsNull = ctx.freshName("coalesceTmpIsNull")


nit: val tmpIsNull = ctx.freshName("coalesceTmpIsNull"), to be consistent with https://github.com/apache/spark/pull/19901/files#diff-a966ee88604a834221e82916ec051d7dR190

cloud-fan · 2017-12-06T08:17:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala

-
+      returnType = ctx.javaType(dataType),
+      makeSplitFunction = {
+        func =>


ditto, keep previous code style.

Ah, got it about makeSplitFunction = func =>. I thought about multline style. Sorry for my misunderstanding.

cloud-fan · 2017-12-06T08:20:23Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala

         |do {
         |  $codes
         |} while (false);
+         |boolean ${ev.isNull} = $coalesceTmpIsNull;


nit: final

cloud-fan · 2017-12-06T08:20:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
+    // inTmpResult -1 indicates at lease one expr in list is evaluated to null.
+    // 0 means no matches found. 1 means the expr in list matches the given value expr.
+    val inTmpResult = ctx.freshName("inTmpResult")


nit: val tmpResult = ctx.freshName("inTmpResult")

also here I think there is no need for this and we can use the same approach used in coalesce.

cloud-fan · 2017-12-06T13:58:58Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

-    val conditionMet = ctx.freshName("caseWhenConditionMet")
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
+    // This variable represents whether the evaluated result is null or not. It's a byte value


We should also update the comment:

This variable holds the state of the result, and has 3 possible values: -1 means the condition is not met yet and the result is unknown. 0 means the condition is met and result is not null. 1 means the condition is met and result is null. It is initialized to `-1`, and if it's set to `1` or `0`, We won't go on anymore on the computation.

cloud-fan · 2017-12-06T14:00:57Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

    // This generates code like:
-    //   conditionMet = caseWhen_1(i);
-    //   if(conditionMet) {
+    //   caseWhenResultIsNull = caseWhen_1(i);


these need to be updated

cloud-fan · 2017-12-06T14:10:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

    val listGen = list.map(_.genCode(ctx))
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.value)
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
+    // inTmpResult -1 indicates at lease one expr in list is evaluated to null.


to be more clear:

inTmpResult has 3 possible values: -1 means no matches found and there is at least one value in the list evaluated to null 0 means no matches found and all values in the list are not null 1 means one value in the list is matched, and the value is saved to `tmpResult`.

cloud-fan · 2017-12-06T14:11:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

    // The evaluation of variables can be stopped when we find a matching value.
    val listCode = listGen.map(x =>
      s"""
+         |$tmpResult = 0;


well, this against what we've discussed before...

Sorry, this is my mistake for debugging.

cloud-fan · 2017-12-06T14:13:01Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

-         |${ev.isNull} = ${valueGen.isNull};
-         |if (!${ev.isNull}) {
+         |byte $tmpResult = (byte)(${valueGen.isNull} ? -1 : 0);
+         |if ($tmpResult != -1) {


I feel it's more readable to write

if (!{ev.isNull}) { byte $tmpResult = 0; ... }

cloud-fan · 2017-12-06T14:14:00Z

LGTM except some minor comments

mgaido91 · 2017-12-06T15:42:43Z

LGTM too

SparkQA · 2017-12-06T15:56:50Z

Test build #84557 has finished for PR 19901 at commit 6a14e16.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-06T17:27:29Z

Test build #84561 has finished for PR 19901 at commit 740f1a0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-12-06T22:43:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

+    // inTmpResult has 3 possible values:
+    //   -1 means no matches found and there is at least one value in the list evaluated to null
+    //   0 means no matches found and all values in the list are not null
+    //   1 means one value in the list is matched


Could we define some constants with meaningful names in doGenCode?

// -1 means no matches found and there is at least one value in the list evaluated final val HAS_NULL = -1 // 0 means no matches found and all values in the list are not null final val NOT_MATCHED = 0 // 1 means one value in the list is matched final val MATCHED = 1

gatorsmile · 2017-12-06T22:44:15Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

+    // This variable holds the state of the result:
+    //   -1 means the condition is not met yet and the result is unknown.
+    //   0 means the condition is met and result is not null.
+    //   1 means the condition is met and result is null.


The same here.

gatorsmile · 2017-12-06T22:46:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

         |${x.code}
         |if (${x.isNull}) {
-         |  ${ev.isNull} = true;
+         |  $tmpResult = -1; // isNull = true


How about updating the comments to

// ${ev.isNull} = true;

gatorsmile · 2017-12-06T22:46:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

         |} else if (${ctx.genEqual(value.dataType, valueArg, x.value)}) {
-         |  ${ev.isNull} = false;
-         |  ${ev.value} = true;
+         |  $tmpResult = 1; // value = TRUE


How about updating the comments to

// ${ev.isNull} = false; ${ev.value} = true;

gatorsmile · 2017-12-06T22:47:31Z

LGTM except a few comments about code readability

viirya · 2017-12-07T00:41:49Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

+    val HAS_NONNULL = 0
+    // 1 means the condition is met and result is null.
+    val HAS_NULL = 1
+    // It is initialized to `-1`, and if it's set to `1` or `0`, We won't go on anymore on the


nit: since you define the constants above, why not use them in this comment?

It is initialized to `NOT_MATCHED`, and if it's set to `HAS_NULL` or `HAS_NONNULL`...

viirya · 2017-12-07T00:44:57Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

+         |do {
+         |  $codes
+         |} while (false);
+         |// TRUE if any condition is met and the result is not null, or no any condition is met.


Seems wrong here. TRUE if any condition is met and the result is null...

viirya · 2017-12-07T00:50:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.value)
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
+    // inTmpResult has 3 possible values:
+    // -1 means no matches found and there is at least one value in the list evaluated


... evaluated to null

viirya · 2017-12-07T00:53:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

         |${x.code}
         |if (${x.isNull}) {
-         |  ${ev.isNull} = true;
+         |  $tmpResult = $HAS_NULL; // ${ev.isNull} = true;


// ${ev.isNull} = true; looks not necessary comment for me.

oh, seems suggested by other reviewer. Then fine for me.

viirya · 2017-12-07T00:53:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

         |} else if (${ctx.genEqual(value.dataType, valueArg, x.value)}) {
-         |  ${ev.isNull} = false;
-         |  ${ev.value} = true;
+         |  $tmpResult = $MATCHED; // ${ev.isNull} = false; ${ev.value} = true;


viirya · 2017-12-07T00:55:20Z

Few minor comments for code comment, LGTM.

SparkQA · 2017-12-07T02:44:17Z

Test build #84584 has finished for PR 19901 at commit 31ab853.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-07T04:37:34Z

Test build #84588 has finished for PR 19901 at commit c4691b6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-12-07T12:50:18Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

-    val conditionMet = ctx.freshName("caseWhenConditionMet")
-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
+    // This variable holds the state of the result:


resultState holds the state of the result, which has 3 possible values:

cloud-fan · 2017-12-07T12:56:05Z

thanks, merging to master! you can address the minor comment in your next PR.

initial commit

8420c02

kiszk force-pushed the SPARK-22705 branch from ae50d97 to 8420c02 Compare December 5, 2017 19:27

fix scala style error

bcbe82c

cloud-fan reviewed Dec 6, 2017

View reviewed changes

viirya reviewed Dec 6, 2017

View reviewed changes

cloud-fan reviewed Dec 6, 2017

View reviewed changes

viirya reviewed Dec 6, 2017

View reviewed changes

address review comment

c81d795

cloud-fan reviewed Dec 6, 2017

View reviewed changes

viirya reviewed Dec 6, 2017

View reviewed changes

cloud-fan reviewed Dec 6, 2017

View reviewed changes

address review comments

740f1a0

gatorsmile reviewed Dec 6, 2017

View reviewed changes

address review comments

31ab853

viirya reviewed Dec 7, 2017

View reviewed changes

address review comments

c4691b6

cloud-fan reviewed Dec 7, 2017

View reviewed changes

asfgit closed this in ea2fbf4 Dec 7, 2017

[SPARK-22705][SQL] Case, Coalesce, and In use less global variables #19901

[SPARK-22705][SQL] Case, Coalesce, and In use less global variables #19901

Uh oh!

Conversation

kiszk commented Dec 5, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Dec 5, 2017

Uh oh!

SparkQA commented Dec 5, 2017

Uh oh!

SparkQA commented Dec 5, 2017

Uh oh!

kiszk commented Dec 6, 2017

Uh oh!

cloud-fan Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 6, 2017

Uh oh!

viirya commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 6, 2017 •

edited

Loading

cloud-fan Dec 6, 2017 •

edited

Loading

kiszk Dec 6, 2017 •

edited

Loading

viirya Dec 6, 2017 •

edited

Loading

cloud-fan Dec 6, 2017 •

edited

Loading