[SPARK-22498][SQL] Fix 64KB JVM bytecode limit problem with concat #19728

kiszk · 2017-11-12T17:01:12Z

What changes were proposed in this pull request?

This PR changes concat code generation to place generated code for expression for arguments into separated methods if these size could be large.
This PR resolved the case of concat with a lot of argument

How was this patch tested?

Added new test cases into StringExpressionsSuite

SparkQA · 2017-11-12T19:59:24Z

Test build #83745 has finished for PR 19728 at commit 16ddb2f.

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

SparkQA · 2017-11-12T20:02:56Z

Test build #83746 has finished for PR 19728 at commit 756254b.

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

SparkQA · 2017-11-12T20:19:11Z

Test build #83747 has finished for PR 19728 at commit bc97d41.

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

viirya · 2017-11-13T03:17:05Z

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

+    ctx.addMutableState("UTF8String[]", args, "")
+
+    val inputs = evals.zipWithIndex.map { case (eval, index) =>
+      if (eval.isNull != "true") {


If eval.isNull is not a pre-evaluated constant?

If eval.isNull is not a pre-evaluated constant, I expect that the following code at lines 73-74 will be generated. Only when we ensure it is true, we can avoid assigning a value (use default null value).

viirya · 2017-11-13T03:17:41Z

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

-      ev.copy(evals.map(_.code).mkString("\n") + s"""
-        UTF8String ${ev.value} = UTF8String.concatWs($inputs);
+      val inputs = evals.tail.zipWithIndex.map { case (eval, index) =>
+        if (eval.isNull != "true") {


Same issue as https://github.com/apache/spark/pull/19728/files#r150441287.

viirya · 2017-11-13T03:18:11Z

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

      }.unzip

-      ev.copy(evals.map(_.code).mkString("\n") +
+      // ev.copy(evals.map(_.code).mkString("\n") +


Forget to remove commented code?

viirya · 2017-11-13T04:05:25Z

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

+      if (eval.isNull != "true") {
+        s"""
+          ${eval.code}
+          $args[$index] = ${eval.value};


If eval.isNull is evaluated to null at runtime, eval.value is useless. We should assign null in that case.

viirya · 2017-11-13T04:06:22Z

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

+        if (eval.isNull != "true") {
+          s"""
+           ${eval.code}
+           $args[$index] = ${eval.value};


Same as https://github.com/apache/spark/pull/19728/files#r150444902.

viirya · 2017-11-13T07:14:48Z

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

-      if (${ev.value} == null) {
-        ${ev.isNull} = true;
+    val argNums = evals.length
+    val args = ctx.freshName("argLen")


argLen? I think it's not length of args?

viirya · 2017-11-13T07:20:57Z

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

+          ${eval.code}
+          if (!${eval.isNull}) {
+            $args[$index] = ${eval.value};
+          }


I think we are better to assign null too if eval.isNull == null, because args is global variable and we need to override all values for previous row.

Thanks, while args is global, UTF8String[] is allocated before executing these assignments. Thus, we can ensure all of elements in args are null.
What do you think?

That is guaranteed before the first row processed. After we process rows, args are updated for each row. E.g., args[0] can be updated and assigned with a string for row 0. In next row, if eval.isNull is evaluated to true, we don't override back to null, so arg[0] is still the string value for row 0.

When the next row is processed, $args = new UTF8String[$argNums] is executed again in apply() method. In other words, I think that current implementation does not reuse UTF8String[] between different rows.
Do you mean UTF8String[] is reused between different rows?

Oh, I see. I didn't notice that args is not globally initialized.

Thank you for taking your time for my code.
It is not easy to imagine the generated code. I may have to put the generated code.

SparkQA · 2017-11-13T08:05:02Z

Test build #83770 has finished for PR 19728 at commit 0ed8d16.

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

SparkQA · 2017-11-13T11:16:13Z

Test build #83779 has finished for PR 19728 at commit 6fdb451.

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

mgaido91 · 2017-11-13T11:51:19Z

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

-        ${ev.isNull} = true;
+    val argNums = evals.length
+    val args = ctx.freshName("args")
+    ctx.addMutableState("UTF8String[]", args, "")


I think we can avoid defining this as a global variable, what do you think?

mgaido91 · 2017-11-13T11:51:52Z

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

-        UTF8String ${ev.value} = UTF8String.concatWs($inputs);
+      val argNums = evals.length
+      val args = ctx.freshName("argLen")
+      ctx.addMutableState("UTF8String[]", args, "")


this also can be kept method local, IMHO

mgaido91 · 2017-11-13T15:08:36Z

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

+      val inputs = evals.tail.zipWithIndex.map { case (eval, index) =>
+        if (eval.isNull != "true") {
+          s"""
+           ${eval.code}


nit: in this and the next 4 lines an indentation space is missing if I am not wrong.

mgaido91 · 2017-11-13T15:17:30Z

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

-
-      ev.copy(evals.map(_.code).mkString("\n") + s"""
-        UTF8String ${ev.value} = UTF8String.concatWs($inputs);
+      val argNums = evals.length


this should be evals.length -1, or even better, I'd suggest to declare two variables: sep and strings (or the name you prefer) to hold head and tail. This would help readability too IMHO.

I am sorry that I cannot understand your suggestion.
argNums is referred as $argNums to allocate an array. Are you suggesting to allocate an array as new UTF8String[$argNums + 1]?

no, I am saying the opposite. Let's consider an example, maybe I better can explain what I mean in this way. Let's assume that we are running concat_ws(',', 'a', 'b'). Then, evals would contain 3 elements. So here your argNums would be 3. But here you would be using only $args[0] and $args[1], because the first element (',', the separator) is handled differently.
Thus, I would suggest to have something like:

val sep = evals.head val strings = evals.tail val argNums = strings.length // note that this is evals.length -1 ...

I think that in this way the code would be much clearer (other than fixing this little bug).

Thank you for your kindly explanation. I totally agree with you.

SparkQA · 2017-11-13T17:51:42Z

Test build #83787 has finished for PR 19728 at commit e6b73ab.

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

SparkQA · 2017-11-13T18:43:01Z

Test build #83791 has finished for PR 19728 at commit cd20410.

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

SparkQA · 2017-11-13T19:11:17Z

Test build #83795 has finished for PR 19728 at commit e668b98.

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

cloud-fan · 2017-11-16T17:09:41Z

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

-      UTF8String ${ev.value} = UTF8String.concat($inputs);
-      if (${ev.value} == null) {
-        ${ev.isNull} = true;
+    val argNums = evals.length


nit: numArgs.

cloud-fan · 2017-11-16T17:10:47Z

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

+    val args = ctx.freshName("args")
+
+    val inputs = evals.zipWithIndex.map { case (eval, index) =>
+      if (eval.isNull != "true") {


please do not mix in optimizations with bug fix

I see. I will remove this.

SparkQA · 2017-11-16T20:38:20Z

Test build #83944 has finished for PR 19728 at commit 555575e.

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

cloud-fan · 2017-11-17T17:09:23Z

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

-      if (${ev.value} == null) {
-        ${ev.isNull} = true;
-      }
+    val numArgs = evals.length


nit: we can inline it

cloud-fan · 2017-11-17T17:16:39Z

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

-        UTF8String ${ev.value} = UTF8String.concatWs($inputs);
+      val separator = evals.head
+      val strings = evals.tail
+      val argNums = strings.length


nit: numArgs

cloud-fan · 2017-11-17T17:18:51Z

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

+      ev.copy(s"""
+        UTF8String[] $args = new UTF8String[$argNums];
+        $codes
+        UTF8String ${ev.value} = UTF8String.concatWs(${separator.value}, $args);


nit:

val code = if (ctx.INPUT_ROW != null && ctx.currentVars == null) ... ev.copy(code = s""" UTF8String[] $args = new UTF8String[$argNums]; ${separator.code} ... """)

cloud-fan · 2017-11-17T18:52:28Z

can you split it into 3 PRs? The approaches for these 3 expressions are quite different.

SparkQA · 2017-11-17T20:32:20Z

Test build #83974 has finished for PR 19728 at commit 22019b1.

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

kiszk · 2017-11-18T04:15:39Z

Sure done.
#19777 for concat_ws. #19778 for elt

SparkQA · 2017-11-18T07:08:04Z

Test build #83983 has finished for PR 19728 at commit b9ca4ff.

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

cloud-fan · 2017-11-18T18:40:36Z

thanks, merging to master/2.2!

## What changes were proposed in this pull request? This PR changes `concat` code generation to place generated code for expression for arguments into separated methods if these size could be large. This PR resolved the case of `concat` with a lot of argument ## How was this patch tested? Added new test cases into `StringExpressionsSuite` Author: Kazuaki Ishizaki <[email protected]> Closes #19728 from kiszk/SPARK-22498. (cherry picked from commit d54bfec) Signed-off-by: Wenchen Fan <[email protected]>

## What changes were proposed in this pull request? This PR changes `concat` code generation to place generated code for expression for arguments into separated methods if these size could be large. This PR resolved the case of `concat` with a lot of argument ## How was this patch tested? Added new test cases into `StringExpressionsSuite` Author: Kazuaki Ishizaki <[email protected]> Closes apache#19728 from kiszk/SPARK-22498. (cherry picked from commit d54bfec) Signed-off-by: Wenchen Fan <[email protected]>

kiszk force-pushed the SPARK-22498 branch from 16ddb2f to 756254b Compare November 12, 2017 17:05

initial commit

bc97d41

kiszk force-pushed the SPARK-22498 branch from 756254b to bc97d41 Compare November 12, 2017 17:18

viirya reviewed Nov 13, 2017

View reviewed changes

address review comment

0ed8d16

viirya reviewed Nov 13, 2017

View reviewed changes

kiszk changed the title ~~[SPARK-22498][SQL] Fix 64KB JVM bytecode limit problem with concat and concat_ws~~ [SPARK-22498][SQL] Fix 64KB JVM bytecode limit problem with concat, concat_ws, and elt Nov 13, 2017

kiszk added 2 commits November 13, 2017 08:09

support elt

366f721

address review comment

6fdb451

mgaido91 reviewed Nov 13, 2017

View reviewed changes

address review comment

e6b73ab

mgaido91 reviewed Nov 13, 2017

View reviewed changes

kiszk added 2 commits November 13, 2017 15:48

address review comment

cd20410

address review comment

e668b98

cloud-fan reviewed Nov 16, 2017

View reviewed changes

address review comment

555575e

cloud-fan reviewed Nov 17, 2017

View reviewed changes

address review comments

22019b1

drop changes for concat_ws and elt

b9ca4ff

kiszk changed the title ~~[SPARK-22498][SQL] Fix 64KB JVM bytecode limit problem with concat, concat_ws, and elt~~ [SPARK-22498][SQL] Fix 64KB JVM bytecode limit problem with concat Nov 18, 2017

asfgit closed this in d54bfec Nov 18, 2017

GulajavaMinistudio mentioned this pull request Nov 19, 2017

Update upstream GulajavaMinistudio/spark#220

Merged

Uh oh!

[SPARK-22498][SQL] Fix 64KB JVM bytecode limit problem with concat #19728

[SPARK-22498][SQL] Fix 64KB JVM bytecode limit problem with concat #19728

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Nov 12, 2017

Uh oh!

SparkQA commented Nov 12, 2017

Uh oh!

SparkQA commented Nov 12, 2017

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 Nov 13, 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!

SparkQA commented Nov 13, 2017

Uh oh!

SparkQA commented Nov 13, 2017

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!

SparkQA commented Nov 13, 2017

Uh oh!

SparkQA commented Nov 13, 2017

Uh oh!

SparkQA commented Nov 13, 2017

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 commented Nov 12, 2017 •

edited

Loading

kiszk Nov 13, 2017 •

edited

Loading

cloud-fan commented Nov 17, 2017 •

edited

Loading