[SPARK-18467][SQL] Extracts method for preparing arguments from StaticInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when `needNullCheck == true`. #15901

ueshin · 2016-11-16T06:30:41Z

What changes were proposed in this pull request?

This pr extracts method for preparing arguments from StaticInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when propageteNull == true.

The steps are as follows:

Introduce InvokeLike to extract common logic from StaticInvoke, Invoke and NewInstance to prepare arguments.
StaticInvoke and Invoke had a risk to exceed 64kb JVM limit to prepare arguments but after this patch they can handle them because they share the preparing code of NewInstance, which handles the limit well.
Remove unneeded null checking and fix nullability of NewInstance.
Avoid some of nullabilty checking which are not needed because the expression is not nullable.
Modify to short circuit if arguments have null when needNullCheck == true.
If needNullCheck == true, preparing arguments can be skipped if we found one of them is null, so modified to short circuit in the case.

How was this patch tested?

Existing tests.

…Invoke` and `NewInstance`.

…Instance`.

…true.

rxin · 2016-11-16T07:28:21Z

So the refactoring for simplification adds more code than it deletes? Did you introduce new features as well?

ueshin · 2016-11-16T07:44:26Z

@rxin I introduced features as follows:

StaticInvoke and Invoke had a risk to exceed 64kb JVM limit to prepare arguments but after this patch they can handle them because they share the preparing code of NewInstance, which handles the limit well.
Avoid some of nullabilty checking which are not needed because the expression is not nullable.
If progagateNull == true, preparing arguments can be skipped if we found one of them is null, so modified to short circuit in the case.

rxin · 2016-11-16T08:01:10Z

cc @cloud-fan

viirya · 2016-11-16T08:40:18Z

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

+
+  def propagateNull: Boolean
+
+  def prepareArguments(ctx: CodegenContext, ev: ExprCode): (String, String, String) = {


Add comment for this method? It is not simple enough to skip the comment.

viirya · 2016-11-16T08:41:53Z

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

+  def prepareArguments(ctx: CodegenContext, ev: ExprCode): (String, String, String) = {
+
+    val argsHaveNull =
+      if (propagateNull && arguments.exists(_.nullable)) {


propagateNull && arguments.exists(_.nullable) is used many times, define a variable for it?

SparkQA · 2016-11-16T08:50:36Z

Test build #68700 has finished for PR 15901 at commit 2f30f53.

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

viirya · 2016-11-16T09:02:43Z

This looks good overall. The more codes added are mostly used to deal with short circuit cases of nullability.

viirya · 2016-11-16T09:05:09Z

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

@@ -109,7 +171,7 @@ case class Invoke(
    functionName: String,
    dataType: DataType,
    arguments: Seq[Expression] = Nil,
-    propagateNull: Boolean = true) extends Expression with NonSQLExpression {
+    propagateNull: Boolean = true) extends Expression with InvokeLike with NonSQLExpression {


The comment of Invoke doesn't have explanation for the param propagateNull. Can you also add it too?

viirya · 2016-11-16T09:16:11Z

The title is not well explained this pr and can be updated to reflect its purpose.

The explanation of the features you added can be moved to the pr description.

cloud-fan · 2016-11-16T09:18:09Z

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

+
+  def prepareArguments(ctx: CodegenContext, ev: ExprCode): (String, String, String) = {
+
+    val argsHaveNull =


this variable name looks like a boolean flag, can you think of a better name?

viirya · 2016-11-16T09:21:36Z

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

@@ -33,6 +33,73 @@ import org.apache.spark.sql.catalyst.util.{ArrayBasedMapData, GenericArrayData}
 import org.apache.spark.sql.types._

 /**
+ * Common base class for [[StaticInvoke]], [[Invoke]], and [[NewInstance]].
+ */
+trait InvokeLike extends Expression {


Does it need to extend Expression?

…end it.

ueshin · 2016-11-17T04:38:26Z

@viirya @cloud-fan I addressed your comments and updated pr title and description.

cloud-fan · 2016-11-17T04:47:27Z

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

+   * Prepares codes for arguments.
+   *
+   * - generate codes for argument.
+   * - use ctx.splitExpressions() not to exceed 64kb JVM limit while preparing arguments.


nit: not to -> to not

Thanks, fixed.

cloud-fan · 2016-11-17T04:54:08Z

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

+   * - use ctx.splitExpressions() not to exceed 64kb JVM limit while preparing arguments.
+   * - avoid some of nullabilty checking which are not needed because the expression is not
+   *   nullable.
+   * - when progagateNull == true, short circuit if we found one of arguments is null because


progagateNull -> propagatingNull? Actually can we think of better name than propagatingNull? how about needNullCheck ?

Thanks, I used needNullCheck for now.

cloud-fan · 2016-11-17T05:03:30Z

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

+      val reset = s"$containsNullInArguments = false;"
+      val argCodes = arguments.zipWithIndex.map { case (e, i) =>
+        val expr = e.genCode(ctx)
+        s"""


how about:

val updateContainsNull = if (e.nullable) { "$containsNull = ${expr.isNull};" } else { "" } s""" if (!$containsNull) { ${expr.code} $updateContainsNull ${argValues(i)} = ${expr.value}; } """

Thanks, I'll use it.

cloud-fan · 2016-11-17T05:06:31Z

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

@@ -50,7 +132,7 @@ case class StaticInvoke(
    dataType: DataType,
    functionName: String,
    arguments: Seq[Expression] = Nil,
-    propagateNull: Boolean = true) extends Expression with NonSQLExpression {
+    propagateNull: Boolean = true) extends Expression with InvokeLike with NonSQLExpression {


why not make InvokeLike extends Expression and NonSQLExpression, then we can just extends InvokeLike here?

Makes sense, I'll modify them.

cloud-fan · 2016-11-17T05:09:04Z

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

-         if ($argIsNulls[idx]) { $isNull = true; break; }
-       }
-     """
+    val prepareIsNull = if (propagateNull && arguments.exists(_.nullable)) {


use propogatingNull?

Oops, I missed it, fixed.

SparkQA · 2016-11-17T15:04:33Z

Test build #68777 has finished for PR 15901 at commit 2c52d91.

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

SparkQA · 2016-11-17T15:08:52Z

Test build #68778 has finished for PR 15901 at commit 43d2693.

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

viirya · 2016-11-18T02:20:49Z

LGTM cc @cloud-fan for another check.

cloud-fan · 2016-11-18T02:46:33Z

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

+   * @param ev an [[ExprCode]] with unique terms.
+   * @return (code to prepare arguments, argument string, result of argument null check)
+   */
+  def prepareArguments(ctx: CodegenContext, ev: ExprCode): (String, String, String) = {


ev is not used.

Thanks, removed.

cloud-fan · 2016-11-18T03:00:33Z

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

+      if (!${obj.isNull}) {
+        $argCode
+      }
+      boolean ${ev.isNull} = ${obj.isNull} || $resultIsNull;


this assumes resultIsNull is a member variable not local variable, can we avoid doing this? We can make the code clearer and more robust like:

javaType ${ev.value} = defaultValue; boolean ${ev.isNull} = true; if (!${obj.isNull}) { $argCode ${ev.isNull} = $resultIsNull ... $postNullCheck }

I'm afraid not because if evaluating arguments is split to some methods, resultIsNull can't be referred from the split methods.

I know resultIsNull must be a member variable now, but in the future we may make it a local variable if the argument code is not so long to exceed the 64kb limit. Here why not we assume resultIsNull can be local variable and make us more robust?

I modified the code as you mentioned.

But I'm sorry, I didn't understand what you want to do.

You are assuming that the argument code is not so long, so use local variable without splitting evaluations? (could also make the argument variable as local variable)

I think the argument code would easily become large enough to exceed the 64kb limit.

You want to modify only resultIsNull to local variable and pass it and return the value to the split methods?

We need to extend the method ctx.splitExpressions() to be able to pass and return value and handle it as [SPARK-16845][SQL] GeneratedClass$SpecificOrdering grows beyond 64 KB #15480 is trying.

You want to switch local variable or member variable by whether split or not?

We need to know whether split or not but we can't now. Modify ctx.splitExpressions() to return split or not?

Could you let me know if you have other thoughts?

You are assuming that the argument code is not so long, so use local variable without splitting evaluations? (could also make the argument variable as local variable)

This is what I mean. We can generate code first and see if it exceeds the 64kb limitation, so logically we do have a chance to use local variables.

I see. Let's see if it exceeds the limit or not, using local variables.

cloud-fan · 2016-11-18T03:06:04Z

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

+      $prepareIsNull
+    """ +
+      (if (needNullCheck) {
+        s"final $javaType ${ev.value} = $isNull ? ${ctx.defaultValue(javaType)} : $constructorCall;"


is it necessary? I'm pretty sure javac can optimize ... = false ? a : b to ... = b for us.

Do we use janino instead of javac?

sorry I mean "java compiler"... I think janino is smart enough about it.

Yeah. I go to check janino source code. It has the optimization: https://github.com/janino-compiler/janino/blob/22a7787e62037049e337cb1ce4064c29a4856022/janino/src/main/java/org/codehaus/janino/UnitCompiler.java#L4247.

I see, updated. Thanks.

cloud-fan · 2016-11-18T03:11:21Z

LGTM except some minor comments, thanks for working on it.

SparkQA · 2016-11-18T10:17:55Z

Test build #68833 has finished for PR 15901 at commit 1baac55.

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

cloud-fan · 2016-11-18T12:15:37Z

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

      $javaType ${ev.value} = ${ctx.defaultValue(dataType)};
-      if (!${ev.isNull}) {
-        $evaluate
+      if (!${obj.isNull}) {


Anyway, I think this new code looks clearer than before, what do you think? @ueshin

Yes, I agree with you. Thank you for your suggestion.

cloud-fan · 2016-11-18T12:18:41Z

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

-       }
-     """
+    val prepareIsNull = if (needNullCheck) {
+      s"boolean $isNull = $resultIsNull;"


one more optimization: we can just write ev.isNull = resultIsNull, then if resultIsNull is false, the final evaluation will become ... = false ? aaa : bbb and get optimized by janino.

Ah, I see what you mean, modified. Thanks.

ueshin · 2016-11-18T16:10:06Z

f8acda6 is a test to see if it exceeds the 64kb JVM limit or not, using only local variables.
I'll revert it if some test failed.

SparkQA · 2016-11-18T17:48:31Z

Test build #68858 has finished for PR 15901 at commit f8acda6.

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

This reverts commit f8acda6.

SparkQA · 2016-11-19T02:45:01Z

Test build #68877 has finished for PR 15901 at commit fe2871c.

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

viirya · 2016-11-19T03:09:26Z

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

-      ${argGen.map(_.code).mkString("\n")}
-      $setIsNull
+      $argCode
+      boolean ${ev.isNull} = $resultIsNull;
      final $javaType ${ev.value} = ${ev.isNull} ? ${ctx.defaultValue(dataType)} : $callFunc;


The ${ev.isNull} at line 153 can be $resultIsNull. So the optimization for false ? ${ctx.defaultValue(dataType)} : $callFunc; can work here too?

Yes, that's right. I modified. Thanks.

SparkQA · 2016-11-19T05:53:11Z

Test build #68881 has finished for PR 15901 at commit c88a1ff.

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

viirya · 2016-11-21T02:12:23Z

LGTM cc @cloud-fan for another look.

cloud-fan · 2016-11-21T04:05:50Z

thanks, merging to master/2.1!

…cInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when `needNullCheck == true`. ## What changes were proposed in this pull request? This pr extracts method for preparing arguments from `StaticInvoke`, `Invoke` and `NewInstance` and modify to short circuit if arguments have `null` when `propageteNull == true`. The steps are as follows: 1. Introduce `InvokeLike` to extract common logic from `StaticInvoke`, `Invoke` and `NewInstance` to prepare arguments. `StaticInvoke` and `Invoke` had a risk to exceed 64kb JVM limit to prepare arguments but after this patch they can handle them because they share the preparing code of NewInstance, which handles the limit well. 2. Remove unneeded null checking and fix nullability of `NewInstance`. Avoid some of nullabilty checking which are not needed because the expression is not nullable. 3. Modify to short circuit if arguments have `null` when `needNullCheck == true`. If `needNullCheck == true`, preparing arguments can be skipped if we found one of them is `null`, so modified to short circuit in the case. ## How was this patch tested? Existing tests. Author: Takuya UESHIN <[email protected]> Closes #15901 from ueshin/issues/SPARK-18467. (cherry picked from commit 6585479) Signed-off-by: Wenchen Fan <[email protected]>

…cInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when `needNullCheck == true`. ## What changes were proposed in this pull request? This pr extracts method for preparing arguments from `StaticInvoke`, `Invoke` and `NewInstance` and modify to short circuit if arguments have `null` when `propageteNull == true`. The steps are as follows: 1. Introduce `InvokeLike` to extract common logic from `StaticInvoke`, `Invoke` and `NewInstance` to prepare arguments. `StaticInvoke` and `Invoke` had a risk to exceed 64kb JVM limit to prepare arguments but after this patch they can handle them because they share the preparing code of NewInstance, which handles the limit well. 2. Remove unneeded null checking and fix nullability of `NewInstance`. Avoid some of nullabilty checking which are not needed because the expression is not nullable. 3. Modify to short circuit if arguments have `null` when `needNullCheck == true`. If `needNullCheck == true`, preparing arguments can be skipped if we found one of them is `null`, so modified to short circuit in the case. ## How was this patch tested? Existing tests. Author: Takuya UESHIN <[email protected]> Closes apache#15901 from ueshin/issues/SPARK-18467.

ueshin added 3 commits November 16, 2016 15:01

Introduce InvokeLike to extract common logic from StaticInvoke, `…

9bf9aa9

…Invoke` and `NewInstance`.

Refactor to remove unneeded null checking and fix nullability of `New…

28f6200

…Instance`.

Modify to short circuit if arguments have null when propageteNull == …

2f30f53

…true.

viirya reviewed Nov 16, 2016

View reviewed changes

cloud-fan reviewed Nov 16, 2016

View reviewed changes

viirya reviewed Nov 16, 2016

View reviewed changes

ueshin added 5 commits November 17, 2016 12:40

Add a comment for prepareArguments().

9ac3f28

Define a variable for propagateNull && arguments.exists(_.nullable).

5ad6966

Add a missing param comment for propagateNull.

a0ac177

Rename variable from argsHaveNull to containsNullInArguments.

757d33e

Modify InvokeLike not to extend Expression because no need to ext…

240fde4

…end it.

ueshin changed the title ~~[SPARK-18467][SQL] Refactor StaticInvoke, Invoke and NewInstance.~~ [SPARK-18467][SQL] Extracts method for preparing arguments from StaticInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when propageteNull == true. Nov 17, 2016

cloud-fan reviewed Nov 17, 2016

View reviewed changes

ueshin added 4 commits November 17, 2016 14:31

Fix comments.

6f6e0b3

Modify for readability.

2bd6e50

Make InvokeLike extend Expression and NonSQLExpression.

bcb93db

Use propagatingNull.

d448b60

cloud-fan reviewed Nov 18, 2016

View reviewed changes

ueshin added 3 commits November 18, 2016 16:00

Remove unused argument.

bd9c09f

Remove unneeded if.

501095f

Modify generated code of Invoke.

1baac55

cloud-fan reviewed Nov 18, 2016

View reviewed changes

ueshin added 3 commits November 19, 2016 00:31

Use resultIsNull as ev.isNull in NewInstance.

831c521

Remove unneeded zipWithIndex.

0b210f8

Use local variables for resultIsNulls and argValues.

f8acda6

Revert "Use local variables for resultIsNulls and argValues."

fe2871c

This reverts commit f8acda6.

viirya reviewed Nov 19, 2016

View reviewed changes

Optimize a code in StaticInvoke.

c88a1ff

asfgit closed this in 6585479 Nov 21, 2016

ueshin mentioned this pull request Nov 21, 2016

[SPARK-18398][SQL] Fix nullabilities of MapObjects and ExternalMapToCatalyst. #15840

Closed


		def propagateNull: Boolean

		def prepareArguments(ctx: CodegenContext, ev: ExprCode): (String, String, String) = {


		def prepareArguments(ctx: CodegenContext, ev: ExprCode): (String, String, String) = {

		val argsHaveNull =

[SPARK-18467][SQL] Extracts method for preparing arguments from StaticInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when needNullCheck == true. #15901

[SPARK-18467][SQL] Extracts method for preparing arguments from StaticInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when needNullCheck == true. #15901

Conversation

ueshin commented Nov 16, 2016 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

rxin commented Nov 16, 2016

ueshin commented Nov 16, 2016

rxin commented Nov 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 16, 2016

viirya commented Nov 16, 2016

Choose a reason for hiding this comment

viirya commented Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 17, 2016

SparkQA commented Nov 17, 2016

viirya commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Nov 18, 2016

SparkQA commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin commented Nov 18, 2016 • edited Loading

SparkQA commented Nov 18, 2016

SparkQA commented Nov 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 19, 2016

viirya commented Nov 21, 2016

cloud-fan commented Nov 21, 2016

[SPARK-18467][SQL] Extracts method for preparing arguments from StaticInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when `needNullCheck == true`. #15901

[SPARK-18467][SQL] Extracts method for preparing arguments from StaticInvoke, Invoke and NewInstance and modify to short circuit if arguments have null when `needNullCheck == true`. #15901

ueshin commented Nov 16, 2016 •

edited

Loading

viirya commented Nov 16, 2016 •

edited

Loading

cloud-fan Nov 17, 2016 •

edited

Loading

cloud-fan Nov 18, 2016 •

edited

Loading

ueshin commented Nov 18, 2016 •

edited

Loading