[SPARK-21413][SQL] Fix 64KB JVM bytecode limit problem in multiple projections with CASE WHEN by kiszk · Pull Request #18641 · apache/spark

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 */classSpecificMutableProjectionextendsorg.apache.spark.sql.catalyst.expressions.codegen.BaseMutableProjection {
...
/* 034 */publicjava.lang.Objectapply(java.lang.Object_i) {
/* 035 */InternalRowi = (InternalRow) _i;
/* 036 *//* 037 *//* 038 *//* 039 */booleanisNull = true;
/* 040 */intvalue = -1;
/* 041 *//* 042 *//* 043 */booleanisNull1 = true;
/* 044 */booleanvalue1 = false;
/* 045 *//* 046 */booleanisNull2 = true;
/* 047 */intvalue2 = -1;
/* 048 *//* 049 *//* 050 */booleanisNull3 = true;
/* 051 */booleanvalue3 = false;
/* 052 *//* 053 */booleanisNull4 = true;
/* 054 */intvalue4 = -1;
/* 055 *//* 056 *//* 057 */booleanisNull5 = true;
/* 058 */booleanvalue5 = false;
/* 059 *//* 060 */booleanisNull6 = true;
/* 061 */intvalue6 = -1;
/* 062 *//* 063 *//* 064 */booleanisNull7 = true;
/* 065 */booleanvalue7 = false;
/* 066 *//* 067 */booleanisNull8 = true;
/* 068 */intvalue8 = -1;
/* 069 *//* 070 *//* 071 */booleanisNull9 = true;
/* 072 */booleanvalue9 = false;
/* 073 *//* 074 */booleanisNull10 = true;
/* 075 */intvalue10 = -1;
/* 076 *//* 077 *//* 078 */booleanisNull11 = true;
/* 079 */booleanvalue11 = false;
/* 080 *//* 081 */booleanisNull12 = true;
/* 082 */intvalue12 = -1;
/* 083 *//* 084 *//* 085 */booleanisNull13 = true;
/* 086 */booleanvalue13 = false;
/* 087 *//* 088 */booleanisNull14 = true;
/* 089 */intvalue14 = -1;
/* 090 *//* 091 *//* 092 */booleanisNull15 = true;
/* 093 */booleanvalue15 = false;
/* 094 *//* 095 */booleanisNull16 = true;
/* 096 */intvalue16 = -1;
/* 097 *//* 098 *//* 099 */booleanisNull17 = true;
/* 100 */booleanvalue17 = false;
/* 101 *//* 102 */booleanisNull18 = true;
/* 103 */intvalue18 = -1;
/* 104 *//* 105 *//* 106 */booleanisNull19 = true;
/* 107 */booleanvalue19 = false;
/* 108 *//* 109 */booleanisNull20 = i.isNullAt(0);
/* 110 */intvalue20 = 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 */booleanisNull23 = i.isNullAt(0);
/* 128 */intvalue23 = isNull23 ? -1 : (i.getInt(0));
/* 129 */isNull18 = isNull23;
/* 130 */value18 = value23;
/* 131 */ }
...

Case 2 after this PR

/* 005 */classSpecificMutableProjectionextendsorg.apache.spark.sql.catalyst.expressions.codegen.BaseMutableProjection {
...
/* 263 */privatebooleanisNull1409;
/* 264 */privatebooleanvalue1409;
...
/* 519 */privatebooleanisNull2815;
/* 520 */privatebooleanvalue2815;
...
/* 1073 */publicjava.lang.Objectapply(java.lang.Object_i) {
/* 1074 */InternalRowi = (InternalRow) _i;
/* 1075 *//* 1076 *//* 1077 *//* 1078 */booleanisNull = true;
/* 1079 */intvalue = -1;
/* 1080 *//* 1081 */caseWhenCondExpr255(i);
/* 1082 */if (!isNull2815 && value2815) {
/* 1083 *//* 1084 */isNull = false;
/* 1085 */value = -1;
/* 1086 */ }
/* 1087 *//* 1088 */else {
/* 1089 *//* 1090 *//* 1091 */booleanisNull2816 = true;
/* 1092 */intvalue2816 = -1;
/* 1093 *//* 1094 */caseWhenCondExpr383(i);
/* 1095 */if (!isNull4223 && value4223) {
/* 1096 *//* 1097 */isNull2816 = false;
/* 1098 */value2816 = -1;
/* 1099 */ }
...
/* 30171 */privatevoidcaseWhenCondExpr255(InternalRowi) {
/* 30172 */booleanisNull1 = true;
/* 30173 */booleanvalue1 = false;
/* 30174 *//* 30175 */booleanisNull2 = true;
/* 30176 */intvalue2 = -1;
/* 30177 *//* 30178 */caseWhenCondExpr127(i);
/* 30179 */if (!isNull1409 && value1409) {
/* 30180 *//* 30181 */isNull2 = false;
/* 30182 */value2 = -1;
/* 30183 */ }
/* 30184 *//* 30185 */else {
/* 30186 *//* 30187 *//* 30188 */booleanisNull1410 = true;
/* 30189 */intvalue1410 = -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

We can move this condition above and reuse it.

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

Shall we check INPUT_ROW too for these three functions?

NVM. I saw there's the check already.

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

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

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

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

+ 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

+ 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 <wenchen@databricks.com> Author: Wenchen Fan <cloud0fan@gmail.com> Closesapache#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 9080500Compare 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

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

Uh oh!

Choose a reason for hiding this comment

kiszk commented Jul 15, 2017•
edited
Loading

kiszkNov 14, 2017•
edited
Loading