[SPARK-22226][SQL] splitExpression can create too many method calls in the outer class #19480

mgaido91 · 2017-10-12T08:21:38Z

What changes were proposed in this pull request?

SPARK-18016 introduced NestedClass to avoid that the many methods generated by splitExpressions contribute to the outer class' constant pool, making it growing too much. Unfortunately, despite their definition is stored in the NestedClass, they all are invoked in the outer class and for each method invocation, there are two entries added to the constant pool: a Methodref and a Utf8 entry (you can easily check this compiling a simple sample class with janinoc and looking at its Constant Pool). This limits the scalability of the solution with very large methods which are split in a lot of small ones. This means that currently we are generating classes like this one:

class SpecificUnsafeProjection extends org.apache.spark.sql.catalyst.expressions.UnsafeProjection {
...
  public UnsafeRow apply(InternalRow i) {
     rowWriter.zeroOutNullBytes();
     apply_0(i);
     apply_1(i);
...
    nestedClassInstance.apply_862(i);
    nestedClassInstance.apply_863(i);
...
    nestedClassInstance1.apply_1612(i);
    nestedClassInstance1.apply_1613(i);
...
  }
...
  private class NestedClass {
    private void apply_862(InternalRow i) { ... }
    private void apply_863(InternalRow i) { ... }
...
  }
  private class NestedClass1 {
    private void apply_1612(InternalRow i) { ... }
    private void apply_1613(InternalRow i) { ... }
...
  }
}

This PR reduce the Constant Pool size of the outer class by adding a new method to each nested class: in this method we invoke all the small methods generated by splitExpression in that nested class. In this way, in the outer class there is only one method invocation per nested class, reducing by orders of magnitude the entries in its constant pool because of method invocations. This means that after the patch the generated code becomes:

class SpecificUnsafeProjection extends org.apache.spark.sql.catalyst.expressions.UnsafeProjection {
...
  public UnsafeRow apply(InternalRow i) {
     rowWriter.zeroOutNullBytes();
     apply_0(i);
     apply_1(i);
     ...
     nestedClassInstance.apply(i);
     nestedClassInstance1.apply(i);
     ...
  }
...
  private class NestedClass {
    private void apply_862(InternalRow i) { ... }
    private void apply_863(InternalRow i) { ... }
...
    private void apply(InternalRow i) {
      apply_862(i);
      apply_863(i);
      ...
    }
  }
  private class NestedClass1 {
    private void apply_1612(InternalRow i) { ... }
    private void apply_1613(InternalRow i) { ... }
...
    private void apply(InternalRow i) {
      apply_1612(i);
      apply_1613(i);
      ...
    }
  }
}

How was this patch tested?

Added UT and existing UTs

viirya · 2017-10-12T09:23:51Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

    // The number of named constants that can exist in the class is limited by the Constant Pool
    // limit, 65,536. We cannot know how many constants will be inserted for a class, so we use a
-    // threshold of 1600k bytes to determine when a function should be inlined to a private, nested
+    // threshold of 1000k bytes to determine when a function should be inlined to a private, nested


Any reason to change this?

Is it for the possibly to be add method grouping all split methods? If yes, we can add the method into an inner class (see #19480 (comment)), so we don't need to change this threshold.

yes, without this change, I got the Constant Pool limit exception of the {{NestedClass}}. Actually I tried to address this issue in this PR (#19447), but without the other changes in thee current PR I wasn't able to create a UT for it.

Can you try #19480 (comment), so we may avoid changing this threshold?

I can't change that for the reason I explained there.

viirya · 2017-10-12T09:25:22Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+ *
+ * @param functionName String representing the name of the function
+ * @param subclassName Optional value which is empty if the function is added to
+ *                     the superclass, otherwise it contains the name of the


superclass or outerclass?

You're right, it's outerclass, thanks, my bad.

viirya · 2017-10-12T09:40:18Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+          // that subclass
+          val code = s"""
+              |private $returnType $func($argString) {
+              |  ${makeSplitFunction(foldFunctions(subclassFunctions.map(name =>


I think we don't need makeSplitFunction here.

we do need for methods like compare where we have to return something.

This is going to call the split methods, not the expression codes. In original splitExpressions, makeSplitFunction only works on the expression codes, not the calls of the methods. But here it applies it on calls of the methods.

Oh. I see. Nvm.

Yes, but this are still splits, just bigger.
The case which is critical for this is compare. In the other cases this function is identity. Let's consider that case: without using it here, you'd get a compilation error, because these "grouping methods" don't return anything by default; instead, if you add it here, you have the default return 0; statement at the end.

viirya · 2017-10-12T09:40:31Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+                    s"$name(${arguments.map(_._2).mkString(", ")})")))}
+              |}
+            """.stripMargin
+          addNewFunctionToClass(func, code, subclassName)


The class to add codes into can be over the threshold too.

So shall we call addNewFunction to add the caller method into an inner class and call those split functions in that?

no, this is made intentionally because if we add it to the NestedClass where these methods are added: they are not available in other nested classes...

Can't we call them with instance?

we might, but I think this would not be the best option, since in this way we are "reusing" some ConstantPool entries (like Utf8 for the name of the function) and it keeps everything about those things in the same class.
Moreover, foreach instance referenced from other classes, like it would be in that case, we are adding new entries to the Constant Pool of that class.
Thus this is the option which minimizes the number of entries added to the Constant Pools.

sorry, I didn't get completely you last comments, might you elaborate it a bit more? Thanks.

Oh. I meant the increased inner classes/instances are added into the outerclass's Constant Pool entries. Outerclass should be more likely to be filled by various variables/methods...than inner classes. So I think to add into Constant Pool of inner classes should be less serious issue than outerclass.

It is also convincing that nested classes should be less. So maybe this is not a serious issue, although I'm not sure this can be completely safe.

yes, but we are likely to recreate the same issue the PR is trying to solve in the last NestedClass. The problem is that a method call, despite it is few bytes of code, introduce multiple entries in the constant pool. If we use addNewFunction here, we will end up creating all the wrapper methods in the last NestedClass, which will have the same issues as the outer class, becoming a new bottelneck.
Instead, in this way, we are splitting the method call among the several nested classes and each of them has its owns, thus it is reusing some constant pool entries.
As you pointed out, nested classes are less critical than the outer one, because the attributes are declared in the outer class and currently all the many small methods are called there. If we put addNewFunction here, instead, we are making critical the last nested function because we are adding there all the many small function calls.

Sounds reasonable to me.

viirya · 2017-10-12T13:03:59Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+ *                     the outer class, otherwise it contains the name of the
+ *                     inner class in which the function has been added.
+ * @param subclassInstance Optional value which is empty if the function is added to
+ *                         the superclass, otherwise it contains the name of the


superclass here too.

viirya · 2017-10-12T13:05:50Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+          val key = (f.subclassName.get, f.subclassInstance.get)
+          acc.updated(key, acc.getOrElse(key, Seq.empty[String]) ++ Seq(f.functionName))
+        }
+        .map { case ((subclassName, subclassInstance), subclassFunctions) =>


If subclassFunctions.length is only 1, I think we don't need to add redundant wrapper caller?

you're right, even though this is very unlikely. We might also think of a threshold maybe. Which is the right approach according to you?

Yeah, I think it can only happen at the first or last sub-class functions. Seems the functions might be only included in two (?) sub-classes. Most of functions will be wrapped in one function call.

I'm not sure the proper threshold for this, maybe 5?

Thanks, I will create a field to make this easy to be changed.

viirya · 2017-10-12T13:33:10Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

-      foldFunctions(functions.map(name => s"$name(${arguments.map(_._2).mkString(", ")})"))
+      val outerClassFunctions = functions
+        .filter(_.subclassName.isEmpty)
+        .map(_.functionName)


So the calls to outerclass functions should not be an issue, even they could be many?

exactly, because since they are defined there, they add no entry to the constant pool

viirya · 2017-10-12T13:39:28Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+        .filter(_.subclassName.isDefined)
+        .foldLeft(Map.empty[(String, String), Seq[String]]) { case (acc, f) =>
+          val key = (f.subclassName.get, f.subclassInstance.get)
+          acc.updated(key, acc.getOrElse(key, Seq.empty[String]) ++ Seq(f.functionName))


The order of function calls should be important and be kept. Does the map guarantee that updated map still preserves the insertion order?

only up to 4 keys, thanks for the nice catch, I will fix this.

viirya · 2017-10-12T13:41:37Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+        .filter(_.subclassName.isEmpty)
+        .map(_.functionName)
+
+      val innerClassFunctions = functions


This might not be very intuitive for later readers. We'd better to add comments to explain it.

I will, thanks for the suggestion

viirya · 2017-10-12T13:42:53Z

@HyukjinKwon or @cloud-fan May you help to trigger the jenkins test? Thank you.

viirya · 2017-10-12T13:44:57Z

@mgaido91 Thank you for working on this! Based on your explanation, sounds like a reasonable direction to me.

HyukjinKwon · 2017-10-12T13:49:35Z

ok to test

viirya · 2017-10-12T14:18:03Z

Thank you @HyukjinKwon

viirya · 2017-10-12T14:19:42Z

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

@@ -201,6 +201,23 @@ class CodeGenerationSuite extends SparkFunSuite with ExpressionEvalHelper {
    }
  }

+  test("SPARK-22226: group splitted expressions into one method per nested class") {


Besides the unit test, can you provide an end-to-end case that can trigger this issue too?

I have a use case where I faced this problem. And I tried this patch on it. Unfortunately this contains a very complex business logic and I have not been able to reproduce it in a simple one. But if needed, I can try again.

Instead of copying your customer codes, can you making a fake one?

@mgaido91 I can reproduce the issue by following test case. You can check it:

test("SPARK-22226: too much splitted expressions should not exceed constant pool limit") { withSQLConf( (SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key, "false")) { val colNumber = 1000 val baseDF = spark.range(10).toDF() val newCols = (1 to colNumber).map { colIndex => expr(s"id + $colIndex").as(s"_$colIndex") } val input = baseDF.select(newCols: _*) val aggs = (1 to colNumber).flatMap { colIndex => val colName = s"_$colIndex" Seq(expr(s"stddev($colName)"), expr(s"stddev_samp($colName)"), expr(s"stddev_pop($colName)"), expr(s"variance($colName)"), expr(s"var_samp($colName)"), expr(s"var_pop($colName)"), expr(s"skewness($colName)"), expr(s"kurtosis($colName)")) } input.agg(aggs.head, aggs.tail: _*).collect() } }

[info] Cause: org.codehaus.janino.JaninoRuntimeException: failed to compile: org.codehaus.janino.JaninoRuntimeExc eption: Constant pool for class org.apache.spark.sql.catalyst.expressions.GeneratedClass$SpecificMutableProjection has grown past JVM limit of 0xFFFF [info] at org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator$.org$apache$spark$sql$catalyst$expressi ons$codegen$CodeGenerator$$doCompile(CodeGenerator.scala:1079)

thank you very much for your help @viirya ! In my use cases it seemed to be connected to the dropDuplicates method and I focused on it, but thanks to your suggestion now I realize that dropDuplicates by itself is not enough, it needs also some functions applied to columns to generate the issue! Thank you so much. Where should I add this test case? I am adding it to DataFrameAggregateSuite since this is related to aggregating some functions, is it ok? Thanks.

I was adding it to DataFrameAggregateSuite.

@viirya I have a good and a bad news... Thanks to your suggestion I have been able to understand and reproduce the issue. Moreover, I found also another issue which is fixed by this problem and I am adding a UT for that too: in some cases, we might have a

Code of method apply(...) grows beyond 64 KB

And with this PR the problem is fixed.

The bad thing is that the UT you provided still fails, but with a different error: actually it is always a Constant Pool limit exceeded exception, but it is in a NestedClass. From my analysis, this is caused by another problem, ie. that we might reference too many fields of the superclass in the NestedClasses. This might be addressed maybe trying to tune the magic number which I brought to 1000k in this PR, but I am pretty sure that it will be also addressed by the ongoing PR for SPARK-18016, since he is trying to reduce the number of variables. Thus I consider this out of scope for this PR.

@mgaido91 Do you meant "2: compact primitive declarations into arrays" in SPARK-18016?

@mgaido91 Thanks for trying it. Yeah, those expressions like skewness are very complicated, so they're likely to cause the issue you encountered.

@viirya exactly, I meant that. Thank you for your suggestion. You have been very helpful to me.

kiszk · 2017-10-12T15:25:10Z

nit: Do we need nestedClassInstance1.apply(i); in the description?

mgaido91 · 2017-10-12T15:30:37Z

@kiszk I added just to show that there is one method for instance, but then I am missing the NestedClass1 declaration. I am adding it. Thanks.

kiszk · 2017-10-12T16:29:47Z

@mgaido91 thanks, this example makes sense.

SparkQA · 2017-10-12T16:33:37Z

Test build #82682 has finished for PR 19480 at commit bdc2fdb.

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

SparkQA · 2017-10-12T16:44:03Z

Test build #82687 has finished for PR 19480 at commit 831fc40.

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

SparkQA · 2017-10-12T20:11:04Z

Test build #82697 has finished for PR 19480 at commit 76b5489.

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

viirya · 2017-10-13T00:15:10Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

@@ -1010,6 +1079,8 @@ object CodeGenerator extends Logging {
  // This is the value of HugeMethodLimit in the OpenJDK JVM settings
  val DEFAULT_JVM_HUGE_METHOD_LIMIT = 8000

+  val MERGE_SPLIT_METHODS_THRESHOLD = 3


Let's add a short comment for this too.

Another magic number?

@gatorsmile it comes from this discussion.

viirya · 2017-10-13T00:26:51Z

The current change LGTM.

If you can have an end-to-end test for this. This might be better. This seems not an issue very easy to encounter by end users. Can you try to have one? @mgaido91

SparkQA · 2017-10-25T21:07:32Z

Test build #83052 has finished for PR 19480 at commit 41c0b2b.

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

gatorsmile · 2017-10-26T07:09:34Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+ *                         the outer class, otherwise it contains the name of the
+ *                         instance of the inner class in the outer class.
+ */
+private[codegen] case class NewFunction(


Rename it to NewFunctionSpec?

gatorsmile · 2017-10-26T07:14:34Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+ * @param subclassName Optional value which is empty if the function is added to
+ *                     the outer class, otherwise it contains the name of the
+ *                     inner class in which the function has been added.
+ * @param subclassInstance Optional value which is empty if the function is added to


I saw three ways to represent the same concept. subclass, inner class, nested classes.

How about renaming all of them to inner classes? Could you go over all the code changes in this PR to make them consistent?

gatorsmile · 2017-10-26T07:17:21Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+          } else {
+            subclassFunctions.map(f => s"$subclassInstance.$f")
+          }
+        }


Build a separate private function for generating innerClassFunctions? Now, the function splitExpressions is pretty large after this PR.

gatorsmile · 2017-10-26T07:18:10Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+        .filter(_.subclassName.isEmpty)
+        .map(_.functionName)
+
+      val argsString = arguments.map(_._2).mkString(", ")


move this just one line before line 876?

gatorsmile · 2017-10-26T07:26:22Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+        .filter(_.subclassName.isDefined)
+        .foldLeft(ListMap.empty[(String, String), Seq[String]]) { case (acc, f) =>
+          val key = (f.subclassName.get, f.subclassInstance.get)
+          acc.updated(key, acc.getOrElse(key, Seq.empty[String]) ++ Seq(f.functionName))


To improve the readability, let us avoid using foldLeft here. You can use a mutable map.

SparkQA · 2017-10-26T11:37:13Z

Test build #83079 has finished for PR 19480 at commit be84c4d.

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

SparkQA · 2017-10-26T16:41:06Z

Test build #83084 has finished for PR 19480 at commit a39098d.

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

gatorsmile · 2017-10-26T18:00:05Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+      val argsString = arguments.map(_._2).mkString(", ")
+      foldFunctions((outerClassFunctions ++ innerClassFunctions).map(
+        name => s"$name($argsString)"))
+    }


val (outerClassFunctions, innerClassFunctions) = functions.partition(_.innerClassName.isEmpty) val argsString = arguments.map(_._2).mkString(", ") val outerClassFunctionCalls = outerClassFunctions.map(f => s"${f.functionName}($argsString)") val innerClassFunctionCalls = generateInnerClassesFunctionCalls( innerClassFunctions, func, arguments, returnType, makeSplitFunction, foldFunctions) foldFunctions(outerClassFunctionCalls ++ innerClassFunctionCalls)

gatorsmile · 2017-10-26T18:48:24Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+      returnType: String,
+      makeSplitFunction: String => String,
+      foldFunctions: Seq[String] => String): Iterable[String] = {
+    val innerClassToFunctions = mutable.ListMap.empty[(String, String), Seq[String]]


Why not using LinkedHashMap

gatorsmile · 2017-10-26T18:50:08Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+        innerClassToFunctions.getOrElse(key, Seq.empty[String]))
+    })
+    // for performance reasons, the functions are prepended, instead of appended,
+    // thus they are in reversed order


Have you seen the perf issue when the number of functions is just thousands?

with 2.000 my tests on my machine (2.5 GHz Intel Core i7 OSX) are:

with this solution 3ms

with the append at the end 121 ms

And of course if the number increases, the difference grows.

gatorsmile · 2017-10-26T18:51:11Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+          addNewFunctionToClass(funcName, code, innerClassName)
+          Seq(s"$innerClassInstance.$funcName")
+        } else {
+          innerClassFunctions.map(f => s"$innerClassInstance.$f")


Here, we are returning the function's invocations. Thus, passing arguments is more straightforward

innerClassFunctions.reverse.map(f => s"$innerClassInstance.$f($argInvocationString)")

gatorsmile · 2017-10-26T18:51:17Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+              |}
+            """.stripMargin
+          addNewFunctionToClass(funcName, code, innerClassName)
+          Seq(s"$innerClassInstance.$funcName")


The same here.

SparkQA · 2017-10-27T12:19:40Z

Test build #83119 has finished for PR 19480 at commit 6e8cd00.

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

gatorsmile · 2017-10-27T15:36:24Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+      val key = (f.innerClassName.get, f.innerClassInstance.get)
+      innerClassToFunctions.update(key, f.functionName +:
+        innerClassToFunctions.getOrElse(key, Seq.empty[String]))
+    })


functions.foreach { f => val key = (f.innerClassName.get, f.innerClassInstance.get) val value = f.functionName +: innerClassToFunctions.getOrElse(key, Seq.empty[String]) innerClassToFunctions.put(key, value) }

gatorsmile · 2017-10-27T15:47:19Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+              |private $returnType $funcName($argDefinitionString) {
+              |  ${makeSplitFunction(body)}
+              |}
+            """.stripMargin


// Adding a new function to each inner class which contains the invocation of all the // ones which have been added to that inner class. For example, // private class NestedClass { // private void apply_862(InternalRow i) { ... } // private void apply_863(InternalRow i) { ... } // ... // private void apply(InternalRow i) { // apply_862(i); // apply_863(i); // ... // } // } val body = foldFunctions(orderedFunctions.map(name => s"$name($argInvocationString)")) val code = s""" |private $returnType $funcName($argDefinitionString) { | ${makeSplitFunction(body)} |} """.stripMargin

gatorsmile · 2017-10-27T15:48:08Z

LGTM except two comments.

Thanks for your work!!! @mgaido91

mgaido91 · 2017-10-27T16:09:58Z

Thank you all for your reviews. I addressed the last comments.
@gatorsmile meanwhile may I ask you to backport this to branch-2.2 when it will be merged?
Thanks.

gatorsmile · 2017-10-27T16:11:34Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+          //     }
+          //   }
+          val body = foldFunctions(orderedFunctions.map(name =>
+            s"$name($argInvocationString)"))


one line or you need to use {}

gatorsmile · 2017-10-27T16:11:59Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+    functions.foreach(f => {
+      val key = (f.innerClassName.get, f.innerClassInstance.get)
+      val value = f.functionName +: innerClassToFunctions.getOrElse(key, Seq.empty[String])
+      innerClassToFunctions.update(key, value)


Nit: update -> put

gatorsmile · 2017-10-27T16:17:56Z

@mgaido91 #18377 was reverted. I believe we are unable to merge this to 2.2. Sorry for that.

gatorsmile · 2017-10-27T16:19:28Z

LGTM pending Jenkins

mgaido91 · 2017-10-27T16:19:57Z

@gatorsmile sorry, I wasn't aware. Thank you anyway.

SparkQA · 2017-10-27T19:05:07Z

Test build #83127 has finished for PR 19480 at commit 36d8e2e.

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

SparkQA · 2017-10-27T19:13:17Z

Test build #83129 has finished for PR 19480 at commit 4952880.

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

gatorsmile · 2017-10-27T20:43:25Z

Thanks! Merged to master.

mgaido91 added 2 commits October 12, 2017 09:59

add a method for each inner class and use it in the superclass

c2cc295

Adding UT and modifying class size limit

d3a5b87

viirya reviewed Oct 12, 2017

View reviewed changes

fix naming error in the comment

e63264b

viirya reviewed Oct 12, 2017

View reviewed changes

fix naming error in comment

bdc2fdb

viirya reviewed Oct 12, 2017

View reviewed changes

mgaido91 added 2 commits October 12, 2017 16:52

set threshold to merge the methods, preserve order and add comments

b9ffc52

Merge branch 'SPARK-22226' of github.com:mgaido91/spark into SPARK-22226

831fc40

mgaido91 changed the title ~~[SPARK-22226] splitExpression can create too many method calls in the outer class~~ [SPARK-22226][SQL] splitExpression can create too many method calls in the outer class Oct 12, 2017

fix bug when the merge split methods threshold is not satisfied

76b5489

viirya reviewed Oct 13, 2017

View reviewed changes

gatorsmile reviewed Oct 26, 2017

View reviewed changes

mgaido91 added 2 commits October 26, 2017 10:09

rename to inner class consistently

6a1eeca

refactor splitExpressions

be84c4d

fix ordering error

a39098d

gatorsmile reviewed Oct 26, 2017

View reviewed changes

further refactor

6e8cd00

gatorsmile reviewed Oct 27, 2017

View reviewed changes

addressing review comments

36d8e2e

gatorsmile reviewed Oct 27, 2017

View reviewed changes

code style fix

4952880

asfgit closed this in b3d8fc3 Oct 27, 2017

mgaido91 deleted the SPARK-22226 branch November 4, 2017 08:50

mgaido91 mentioned this pull request Nov 27, 2017

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

Closed

[SPARK-22226][SQL] splitExpression can create too many method calls in the outer class #19480

[SPARK-22226][SQL] splitExpression can create too many method calls in the outer class #19480

Uh oh!

Conversation

mgaido91 commented Oct 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!

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!

viirya Oct 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgaido91 Oct 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Oct 12, 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!

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!

viirya Oct 12, 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!

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

mgaido91 commented Oct 12, 2017 •

edited

Loading

viirya Oct 12, 2017 •

edited

Loading

mgaido91 Oct 12, 2017 •

edited

Loading

viirya Oct 12, 2017 •

edited

Loading

viirya Oct 12, 2017 •

edited

Loading

viirya Oct 13, 2017 •

edited

Loading

mgaido91 Oct 13, 2017 •

edited

Loading