[SPARK-22500][SQL] Fix 64KB JVM bytecode limit problem with cast #19730

kiszk · 2017-11-12T19:06:15Z

What changes were proposed in this pull request?

This PR changes cast code generation to place generated code for expression for fields of a structure into separated methods if these size could be large.

How was this patch tested?

Added new test cases into CastSuite

mgaido91 · 2017-11-12T19:15:21Z

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

@@ -1015,7 +1015,9 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
    }
    val rowClass = classOf[GenericInternalRow].getName
    val result = ctx.freshName("result")
+    ctx.addMutableState(s"$rowClass", result, "")


I think that since we not assigning it, we might keep it as a local variable and pass it to the generated methods. In this way we can avoid to introduce new global variables. Have you tried that?

mgaido91 · 2017-11-12T19:15:36Z

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

    val tmpRow = ctx.freshName("tmpRow")
+    ctx.addMutableState("InternalRow", tmpRow, "")


SparkQA · 2017-11-12T22:01:02Z

Test build #83750 has finished for PR 19730 at commit 9013671.

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

mgaido91 · 2017-11-13T16:10:28Z

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

@@ -1039,13 +1039,19 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
          }
        }
       """
-    }.mkString("\n")
+    }
+    val fieldsEvalCodes = if (ctx.INPUT_ROW != null && ctx.currentVars == null) {


shouldn't be ctx.currentVars != null?

If ctx.currentVars != null, we need to use mkString("\n").

SparkQA · 2017-11-13T18:35:09Z

Test build #83790 has finished for PR 19730 at commit 94dfde2.

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

SparkQA · 2017-11-13T18:57:53Z

Test build #83792 has finished for PR 19730 at commit 39ffaa9.

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

cloud-fan · 2017-11-16T17:06:08Z

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

+    }
+    val fieldsEvalCodes = if (ctx.INPUT_ROW != null && ctx.currentVars == null) {
+      ctx.splitExpressions(fieldsEvalCode, "castStruct",
+        ("InternalRow", ctx.INPUT_ROW) :: (rowClass, result) :: ("InternalRow", tmpRow) :: Nil)


how about inner struct? We also need to pass in the ctx.INPUT_ROW?

Inner struct case in the following code and existing cases in CastSuite works well. Tomorrow, I will add similar test case with many fields.

val struct = Literal.create( Row( UTF8String.fromString("123.4"), Seq("456", "true", "78.9"), Row(7)), StructType(Seq( StructField("i", StringType, nullable = true), StructField("a", ArrayType(StringType, containsNull = false), nullable = true), StructField("s", StructType(Seq( StructField("i", IntegerType, nullable = true))))))) val ret = cast(struct, StructType(Seq( StructField("d", DoubleType, nullable = true), StructField("a", ArrayType(IntegerType, containsNull = true), nullable = true), StructField("s", StructType(Seq( StructField("l", LongType, nullable = true))))))) assert(ret.resolved === true) checkEvaluation(ret, Row(123.4, Seq(456, null, 78), Row(7L)))

I mean, we don't need to pass in ctx.INPUT_ROW to the split functions.

cloud-fan · 2017-11-17T16:10:30Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+    checkEvaluation(cast(Literal.create(input1, from1), to1), output1)
+
+    val from2 = new StructType(
+      (1 to N).map(i => StructField(s"a$i", ArrayType(StringType, containsNull = false))).toArray)


I'd expect something like

val from2 = new StructType( (1 to N).map(i => StructField(s"s$i", from1)).toArray)

or just test this case.

SparkQA · 2017-11-17T17:25:33Z

Test build #83970 has finished for PR 19730 at commit 83fef40.

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

cloud-fan · 2017-11-20T20:28:52Z

ping @kiszk

kiszk · 2017-11-21T01:56:27Z

Working for this. I met another problem. Since this code creates a lot of global variable, I met the problem due to 64K constant pool entries.
Since UTF8String.IntWrapper can be reused, I am writing the following code.

      val wrapper = "intWrapper"
      if (!ctx.mutableStates.exists(s => s._1 == wrapper)) {
        ctx.addMutableState("UTF8String.IntWrapper", wrapper,
          s"$wrapper = new UTF8String.IntWrapper();")
      }

gatorsmile · 2017-11-21T06:38:21Z

Yeah. We can fix them in this PR.

BTW, could you check all the other calls of addMutableState and fix them too?

cloud-fan · 2017-11-21T07:13:49Z

I think this is a different issue and should be fixed with another PR. @kiszk how about we change the test to cast int to long to avoid this issue?

kiszk · 2017-11-21T07:16:46Z

@cloud-fan I see, I will create another PR to fix this global variable issue.
@gatorsmile I will check other calls.

cloud-fan · 2017-11-21T09:47:20Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+      (1 to M).map(i => StructField(s"s$i", toInner)).toArray)
+    val inputOuter = Row.fromSeq((1 to M).map(_ => inputInner))
+    val outputOuter = Row.fromSeq((1 to M).map(_ => outputInner))
+    checkEvaluation(cast(Literal.create(inputOuter, fromOuter), toOuter), outputOuter)


I think this case is good enough to cover all the above cases?

SparkQA · 2017-11-21T12:14:18Z

Test build #84064 has finished for PR 19730 at commit c46ef5c.

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

SparkQA · 2017-11-21T20:18:28Z

Test build #84079 has finished for PR 19730 at commit 574691f.

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

cloud-fan · 2017-11-21T21:46:42Z

thanks, merging to master/2.2!

This PR changes `cast` code generation to place generated code for expression for fields of a structure into separated methods if these size could be large. Added new test cases into `CastSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes #19730 from kiszk/SPARK-22500. (cherry picked from commit ac10171) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

… whole stage codegen ## What changes were proposed in this pull request? A followup of #19730, we can split the code for casting struct even with whole stage codegen. This PR also has some renaming to make the code easier to read. ## How was this patch tested? existing test Author: Wenchen Fan <wenchen@databricks.com> Closes #19891 from cloud-fan/cast.

This PR changes `cast` code generation to place generated code for expression for fields of a structure into separated methods if these size could be large. Added new test cases into `CastSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes apache#19730 from kiszk/SPARK-22500. (cherry picked from commit ac10171) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

initial commit

9013671

mgaido91 reviewed Nov 12, 2017

View reviewed changes

kiszk added 2 commits November 13, 2017 15:45

address review comment

94dfde2

make changes smaller

39ffaa9

mgaido91 reviewed Nov 13, 2017

View reviewed changes

cloud-fan reviewed Nov 16, 2017

View reviewed changes

address review comment

83fef40

cloud-fan reviewed Nov 17, 2017

View reviewed changes

address review comments

c46ef5c

cloud-fan reviewed Nov 21, 2017

View reviewed changes

address review comment

574691f

asfgit closed this in ac10171 Nov 21, 2017

kiszk mentioned this pull request Nov 24, 2017

[SPARK-22595][SQL] fix flaky test: CastSuite.SPARK-22500: cast for struct should not generate codes beyond 64KB #19806

Closed

cloud-fan mentioned this pull request Dec 5, 2017

[SPARK-22500][SQL][followup] cast for struct can split code even with whole stage codegen #19891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22500][SQL] Fix 64KB JVM bytecode limit problem with cast #19730

[SPARK-22500][SQL] Fix 64KB JVM bytecode limit problem with cast #19730

kiszk commented Nov 12, 2017

mgaido91 Nov 12, 2017

mgaido91 Nov 12, 2017

SparkQA commented Nov 12, 2017

mgaido91 Nov 13, 2017

kiszk Nov 13, 2017

SparkQA commented Nov 13, 2017

SparkQA commented Nov 13, 2017

cloud-fan Nov 16, 2017

kiszk Nov 16, 2017 •

edited

Loading

cloud-fan Nov 17, 2017

cloud-fan Nov 17, 2017

cloud-fan Nov 17, 2017

SparkQA commented Nov 17, 2017

cloud-fan commented Nov 20, 2017

kiszk commented Nov 21, 2017 •

edited

Loading

gatorsmile commented Nov 21, 2017

cloud-fan commented Nov 21, 2017

kiszk commented Nov 21, 2017

cloud-fan Nov 21, 2017

SparkQA commented Nov 21, 2017

SparkQA commented Nov 21, 2017

cloud-fan commented Nov 21, 2017 •

edited

Loading

		val tmpRow = ctx.freshName("tmpRow")
		ctx.addMutableState("InternalRow", tmpRow, "")

[SPARK-22500][SQL] Fix 64KB JVM bytecode limit problem with cast #19730

[SPARK-22500][SQL] Fix 64KB JVM bytecode limit problem with cast #19730

Conversation

kiszk commented Nov 12, 2017

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 13, 2017

SparkQA commented Nov 13, 2017

Choose a reason for hiding this comment

kiszk Nov 16, 2017 • 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, 2017

cloud-fan commented Nov 20, 2017

kiszk commented Nov 21, 2017 • edited Loading

gatorsmile commented Nov 21, 2017

cloud-fan commented Nov 21, 2017

kiszk commented Nov 21, 2017

Choose a reason for hiding this comment

SparkQA commented Nov 21, 2017

SparkQA commented Nov 21, 2017

cloud-fan commented Nov 21, 2017 • edited Loading

kiszk Nov 16, 2017 •

edited

Loading

kiszk commented Nov 21, 2017 •

edited

Loading

cloud-fan commented Nov 21, 2017 •

edited

Loading