[SPARK-21413][SQL] Fix 64KB JVM bytecode limit problem in multiple projections with CASE WHEN #18641

kiszk · 2017-07-15T08:40:19Z

What changes were proposed in this pull request?

This PR changes Case When code generation to place condition and then expressions' generated code into separated methods if these size could be large. When the method is newly generated, variables for isNull and value are declared as an instance variable to pass these values (e.g. isNull1409 and value1409) to the callers of the generated method.

This PR resolved three cases:

large code size of condition or then expression
large code size of else expression
large code size of total conditional branches

Case 1 before this PR

/* 005 */ class SpecificMutableProjection extends org.apache.spark.sql.catalyst.expressions.codegen.BaseMutableProjection {
...
/* 034 */   public java.lang.Object apply(java.lang.Object _i) {
/* 035 */     InternalRow i = (InternalRow) _i;
/* 036 */
/* 037 */
/* 038 */
/* 039 */     boolean isNull = true;
/* 040 */     int value = -1;
/* 041 */
/* 042 */
/* 043 */     boolean isNull1 = true;
/* 044 */     boolean value1 = false;
/* 045 */
/* 046 */     boolean isNull2 = true;
/* 047 */     int value2 = -1;
/* 048 */
/* 049 */
/* 050 */     boolean isNull3 = true;
/* 051 */     boolean value3 = false;
/* 052 */
/* 053 */     boolean isNull4 = true;
/* 054 */     int value4 = -1;
/* 055 */
/* 056 */
/* 057 */     boolean isNull5 = true;
/* 058 */     boolean value5 = false;
/* 059 */
/* 060 */     boolean isNull6 = true;
/* 061 */     int value6 = -1;
/* 062 */
/* 063 */
/* 064 */     boolean isNull7 = true;
/* 065 */     boolean value7 = false;
/* 066 */
/* 067 */     boolean isNull8 = true;
/* 068 */     int value8 = -1;
/* 069 */
/* 070 */
/* 071 */     boolean isNull9 = true;
/* 072 */     boolean value9 = false;
/* 073 */
/* 074 */     boolean isNull10 = true;
/* 075 */     int value10 = -1;
/* 076 */
/* 077 */
/* 078 */     boolean isNull11 = true;
/* 079 */     boolean value11 = false;
/* 080 */
/* 081 */     boolean isNull12 = true;
/* 082 */     int value12 = -1;
/* 083 */
/* 084 */
/* 085 */     boolean isNull13 = true;
/* 086 */     boolean value13 = false;
/* 087 */
/* 088 */     boolean isNull14 = true;
/* 089 */     int value14 = -1;
/* 090 */
/* 091 */
/* 092 */     boolean isNull15 = true;
/* 093 */     boolean value15 = false;
/* 094 */
/* 095 */     boolean isNull16 = true;
/* 096 */     int value16 = -1;
/* 097 */
/* 098 */
/* 099 */     boolean isNull17 = true;
/* 100 */     boolean value17 = false;
/* 101 */
/* 102 */     boolean isNull18 = true;
/* 103 */     int value18 = -1;
/* 104 */
/* 105 */
/* 106 */     boolean isNull19 = true;
/* 107 */     boolean value19 = false;
/* 108 */
/* 109 */     boolean isNull20 = i.isNullAt(0);
/* 110 */     int value20 = isNull20 ? -1 : (i.getInt(0));
/* 111 */     if (!isNull20) {
/* 112 */
/* 113 */
/* 114 */       isNull19 = false; // resultCode could change nullability.
/* 115 */       value19 = value20 == 0;
/* 116 */
/* 117 */     }
/* 118 */     if (!isNull19 && value19) {
/* 119 */
/* 120 */       isNull18 = false;
/* 121 */       value18 = -1;
/* 122 */     }
/* 123 */
/* 124 */     else {
/* 125 */
/* 126 */
/* 127 */       boolean isNull23 = i.isNullAt(0);
/* 128 */       int value23 = isNull23 ? -1 : (i.getInt(0));
/* 129 */       isNull18 = isNull23;
/* 130 */       value18 = value23;
/* 131 */     }
...

Case 2 after this PR

/* 005 */ class SpecificMutableProjection extends org.apache.spark.sql.catalyst.expressions.codegen.BaseMutableProjection {
...
/* 263 */   private boolean isNull1409;
/* 264 */   private boolean value1409;
...
/* 519 */   private boolean isNull2815;
/* 520 */   private boolean value2815;
...
/* 1073 */   public java.lang.Object apply(java.lang.Object _i) {
/* 1074 */     InternalRow i = (InternalRow) _i;
/* 1075 */
/* 1076 */
/* 1077 */
/* 1078 */     boolean isNull = true;
/* 1079 */     int value = -1;
/* 1080 */
/* 1081 */     caseWhenCondExpr255(i);
/* 1082 */     if (!isNull2815 && value2815) {
/* 1083 */
/* 1084 */       isNull = false;
/* 1085 */       value = -1;
/* 1086 */     }
/* 1087 */
/* 1088 */     else {
/* 1089 */
/* 1090 */
/* 1091 */       boolean isNull2816 = true;
/* 1092 */       int value2816 = -1;
/* 1093 */
/* 1094 */       caseWhenCondExpr383(i);
/* 1095 */       if (!isNull4223 && value4223) {
/* 1096 */
/* 1097 */         isNull2816 = false;
/* 1098 */         value2816 = -1;
/* 1099 */       }
...
/* 30171 */   private void caseWhenCondExpr255(InternalRow i) {
/* 30172 */     boolean isNull1 = true;
/* 30173 */     boolean value1 = false;
/* 30174 */
/* 30175 */     boolean isNull2 = true;
/* 30176 */     int value2 = -1;
/* 30177 */
/* 30178 */     caseWhenCondExpr127(i);
/* 30179 */     if (!isNull1409 && value1409) {
/* 30180 */
/* 30181 */       isNull2 = false;
/* 30182 */       value2 = -1;
/* 30183 */     }
/* 30184 */
/* 30185 */     else {
/* 30186 */
/* 30187 */
/* 30188 */       boolean isNull1410 = true;
/* 30189 */       int value1410 = -1;
/* 30190 */
/* 30191 */       caseWhenCondExpr191(i);
/* 30192 */       if (!isNull2113 && value2113) {
/* 30193 */
/* 30194 */         isNull1410 = false;
/* 30195 */         value1410 = -1;
/* 30196 */       }
...
/* 30342 */     if (!isNull2) {
/* 30343 */
/* 30344 */
/* 30345 */       isNull1 = false; // resultCode could change nullability.
/* 30346 */       value1 = value2 == 0;
/* 30347 */
/* 30348 */     }
/* 30349 */     isNull2815 = isNull1;
/* 30350 */     value2815 = value1;
/* 30351 */   }
...

How was this patch tested?

Added new test suites into CodeGenerationSuite and DataFrameSuite

SparkQA · 2017-07-15T10:24:45Z

Test build #79632 has finished for PR 18641 at commit 19ae0dc.

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

SparkQA · 2017-07-15T19:00:41Z

Test build #79636 has finished for PR 18641 at commit acfdf54.

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

kiszk · 2017-07-15T23:08:09Z

@viirya @cloud-fan could you please take a look?

viirya · 2017-07-17T02:35:01Z

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

We can move this condition above and reuse it.

viirya · 2017-07-17T02:35:53Z

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

Shall we check INPUT_ROW too for these three functions?

NVM. I saw there's the check already.

viirya · 2017-07-17T02:40:34Z

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

Is it enough to only consider the generated code of individual condition? If it is less than 512, e.g, 500, but the combination of all conditions can still be large to cause the same issue.

Good catch. Now, I updated it to use the same logic as if.

SparkQA · 2017-07-17T07:04:54Z

Test build #79660 has finished for PR 18641 at commit 164ea83.

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

kiszk · 2017-07-17T12:01:03Z

Jenkins, retest this please

SparkQA · 2017-07-17T14:41:45Z

Test build #79666 has finished for PR 18641 at commit 164ea83.

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

kiszk · 2017-07-18T09:42:29Z

@viirya Are there any additional comments?
cc @cloud-fan

viirya · 2017-07-18T12:52:17Z

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

Actually my previous comment meant the sum of generated codes of all branches.

With latest commit, it is possible that the code size of each branch is in the range, but the sum of all branches still breaks 64KB limit.

Ah, got it. You mean that we have to split super deeply-nested if-then-else statements into multiple methods, too.
I will work for that.

@viirya I worked for possible three cases that may break 64KB limit.

SparkQA · 2017-07-19T10:44:55Z

Test build #79747 has finished for PR 18641 at commit dcd5105.

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

SparkQA · 2017-07-19T12:03:31Z

Test build #79750 has finished for PR 18641 at commit b11888d.

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

SparkQA · 2017-07-19T17:04:23Z

Test build #79762 has finished for PR 18641 at commit 780ae2b.

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

kiszk · 2017-07-25T01:42:23Z

ping @cloud-fan

viirya · 2017-07-26T03:20:08Z

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

This comment is put at wrong place?

good catch. done.

SparkQA · 2017-07-29T18:19:46Z

Test build #80045 has finished for PR 18641 at commit e5431a0.

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

kiszk · 2017-08-07T02:31:52Z

ping @cloud-fan

kiszk · 2017-08-15T18:19:20Z

ping @cloud-fan

kiszk · 2017-08-22T05:38:37Z

ping @cloud-fan

kiszk · 2017-10-12T11:26:15Z

Jenkins, retest this please

SparkQA · 2017-10-12T14:19:29Z

Test build #82677 has finished for PR 18641 at commit e5431a0.

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

kiszk · 2017-10-28T09:55:30Z

Jenkins, retest this please

SparkQA · 2017-10-28T12:46:24Z

Test build #83165 has finished for PR 18641 at commit e5431a0.

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

kiszk · 2017-11-10T16:37:11Z

@cloud-fan would it be possible to review if you have time?

cloud-fan · 2017-11-13T19:56:57Z

can you fix the conflict?

SparkQA · 2017-11-13T22:51:04Z

Test build #83803 has finished for PR 18641 at commit c79e1f5.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

… whole-stage codegen

cloud-fan · 2017-11-14T10:00:00Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

+    val schema = StructType(StructField("a", IntegerType) :: Nil)
+    val df = spark.createDataFrame(sparkContext.parallelize(Seq(Row(1))), schema)
+    val df1 =
+      df.withColumn("a", when($"a" === 0, null).otherwise($"a"))


maybe use a loop here?

Sure, the following code can throw an exception

... var df1 = df for (i <- 1 to 10) { df1 = df1.withColumn("a", when($"a" === 0, null).otherwise($"a")) } ...

cloud-fan · 2017-11-14T10:03:56Z

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

+    val isNull = ctx.freshName("caseWhenIsNull")
+    val value = ctx.freshName("caseWhenValue")
+
    val cases = branches.map { case (condExpr, valueExpr) =>


Can we follow what we did for And and Or, and just check the code length at the beginning? TBH I don't understand your change after several minutes reading.

For CaseWhen, the code bloat occurs in one case class. The CaseWhenCodegen.doGenCode can generate deeply-nested if-then-else statements as above in the comment. Each element in cases has only a if-then. Thus, it is not possible to insert code check here. Since And and Or generates deeply nested if-then-else by calling doGenCode many times, to check code size here works well.

This line generates the nested if-then-else. Thus, after this line, code size check is performed.

What do you think?

We only codegen CASE WHEN if the case branches are less than 20, I think check code size here is good enough.

I see. Do you want to add such a code?

val cases = ... val genCode = if (cases.map(s => s.length).sum <= 1024) { cases.mkString("\nelse {\n") } else { // current code var isGlobalVariable = false ... generatedCode }

SparkQA · 2017-11-14T11:14:23Z

Test build #83842 has finished for PR 18641 at commit 9080500.

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

SparkQA · 2017-11-14T18:10:13Z

Test build #83854 has finished for PR 18641 at commit b6030d9.

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

SparkQA · 2017-11-15T06:20:24Z

Test build #83880 has finished for PR 18641 at commit e69f126.

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

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

Test build #83882 has finished for PR 18641 at commit 5466ef0.

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

HyukjinKwon · 2017-11-15T08:10:01Z

retest this please

SparkQA · 2017-11-15T11:00:29Z

Test build #83885 has finished for PR 18641 at commit 5466ef0.

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

…essions ## What changes were proposed in this pull request? A frequently reported issue of Spark is the Java 64kb compile error. This is because Spark generates a very big method and it's usually caused by 3 reasons: 1. a deep expression tree, e.g. a very complex filter condition 2. many individual expressions, e.g. expressions can have many children, operators can have many expressions. 3. a deep query plan tree (with whole stage codegen) This PR focuses on 1. There are already several patches(apache#15620 apache#18972 apache#18641) trying to fix this issue and some of them are already merged. However this is an endless job as every non-leaf expression has this issue. This PR proposes to fix this issue in `Expression.genCode`, to make sure the code for a single expression won't grow too big. According to maropu 's benchmark, no regression is found with TPCDS (thanks maropu !): https://docs.google.com/spreadsheets/d/1K3_7lX05-ZgxDXi9X_GleNnDjcnJIfoSlSCDZcL4gdg/edit?usp=sharing ## How was this patch tested? existing test Author: Wenchen Fan <[email protected]> Author: Wenchen Fan <[email protected]> Closes apache#19767 from cloud-fan/codegen.

kiszk · 2017-11-27T07:45:05Z

#19752 will cover this solution.

viirya reviewed Jul 17, 2017

View reviewed changes

viirya reviewed Jul 18, 2017

View reviewed changes

viirya reviewed Jul 26, 2017

View reviewed changes

kiszk added 3 commits November 14, 2017 10:49

initial commit

a26aafe

remove end-to-end test since sbt test does not go to fallback path in…

6f0de18

… whole-stage codegen

Changed a decision logic to split it into methods

7c78cf2

kiszk added 6 commits November 14, 2017 10:52

address review comment

2e01427

fix comments

fb62f3c

fix failure of HiveCompatibilitySuite.ppr_allchildsarenull

85bee25

address review comment

3cb95aa

use return value of addNewFunction for caller function name

ca0ddd5

rebase with master

9080500

kiszk force-pushed the SPARK-21413 branch from c79e1f5 to 9080500 Compare November 14, 2017 09:17

cloud-fan reviewed Nov 14, 2017

View reviewed changes

address review comment

b6030d9

fix failures for ImputerSuite

e69f126

fix failures for ImputerSuite

5466ef0

cloud-fan mentioned this pull request Nov 16, 2017

[SPARK-22543][SQL] fix java 64kb compile error for deeply nested expressions #19767

Closed

kiszk mentioned this pull request Nov 19, 2017

[SPARK-22520][SQL] Support code generation for large CaseWhen #19752

Closed

kiszk closed this Nov 27, 2017

[SPARK-21413][SQL] Fix 64KB JVM bytecode limit problem in multiple projections with CASE WHEN #18641

[SPARK-21413][SQL] Fix 64KB JVM bytecode limit problem in multiple projections with CASE WHEN #18641

Uh oh!

Conversation

kiszk commented Jul 15, 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 Jul 15, 2017

Uh oh!

SparkQA commented Jul 15, 2017

Uh oh!

kiszk commented Jul 15, 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!

SparkQA commented Jul 17, 2017

Uh oh!

kiszk commented Jul 17, 2017

Uh oh!

SparkQA commented Jul 17, 2017

Uh oh!

kiszk commented Jul 18, 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!

SparkQA commented Jul 19, 2017

Uh oh!

SparkQA commented Jul 19, 2017

Uh oh!

SparkQA commented Jul 19, 2017

Uh oh!

kiszk commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 29, 2017

Uh oh!

kiszk commented Aug 7, 2017

Uh oh!

kiszk commented Aug 15, 2017

Uh oh!

kiszk commented Aug 22, 2017

Uh oh!

kiszk commented Oct 12, 2017

Uh oh!

SparkQA commented Oct 12, 2017

Uh oh!

kiszk commented Oct 28, 2017

Uh oh!

SparkQA commented Oct 28, 2017

Uh oh!

kiszk commented Nov 10, 2017

Uh oh!

cloud-fan 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

kiszk commented Jul 15, 2017 •

edited

Loading

kiszk Nov 14, 2017 •

edited

Loading