[SPARK-23546][SQL] Refactor stateless methods/values in CodegenContext #20700

kiszk · 2018-03-01T11:02:13Z

What changes were proposed in this pull request?

A current CodegenContext class has immutable value or method without mutable state, too.
This refactoring moves them to CodeGenerator object class which can be accessed from anywhere without an instantiated CodegenContext in the program.

How was this patch tested?

Existing tests

kiszk · 2018-03-01T11:03:31Z

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

@@ -773,7 +773,7 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
         |  ${if (i != 0) s"""$buffer.append(" ");""" else ""}
         |
         |  // Append $i field into the string buffer
-         |  ${ctx.javaType(ft)} $field = ${ctx.getValue(row, ft, s"$i")};
+         |  ${ctx.javaType(ft)} $field = ${CodeGenerator.getValue(row, ft, s"$i")};


Some methods that are very frequently used (e.g. ctx.javaType()) are not refactored yet. This is because this refactoring updates more files.

SparkQA · 2018-03-01T11:04:26Z

Test build #87828 has finished for PR 20700 at commit 0b2b381.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-01T11:44:43Z

Test build #87832 has finished for PR 20700 at commit fc4c6ad.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-01T12:32:45Z

Test build #87835 has finished for PR 20700 at commit 276df59.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-01T16:14:19Z

Test build #87837 has finished for PR 20700 at commit f8b478f.

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

mgaido91 · 2018-03-01T16:17:58Z

I like this approach. Let's see others' opinion, but it seems a good thing to do up to me.

kiszk · 2018-03-01T16:27:18Z

@mgaido91 thanks. Let me ping @cloud-fan and @viirya

hvanhovell · 2018-03-01T19:27:58Z

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

@@ -1524,4 +1348,215 @@ object CodeGenerator extends Logging {
          result
        }
      })
+
+  final val JAVA_BOOLEAN = "boolean"


Is this normally spaced?

viirya

Seems fine to me.

viirya · 2018-03-02T06:39:29Z

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

-  final val JAVA_FLOAT = "float"
-  final val JAVA_DOUBLE = "double"
+  /**
+   * Returns true if the Java type has a special accessor and setter in [[InternalRow]].


This comment is copied. But seems not totally correct. InternalRow also has special accessor for Decimal. But it is not a primitive type here.

viirya · 2018-03-02T06:49:42Z

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

+    val getValueMapKeyArray0 = CodeGenerator.getValue(s"$map.keyArray()", kt, "0")
+    val getValueMapValArray0 = CodeGenerator.getValue(s"$map.valueArray()", vt, "0")
+    val getValueMapKeyArray = CodeGenerator.getValue(s"$map.keyArray()", kt, loopIndex)
+    val getValueMapValArray = CodeGenerator.getValue(s"$map.valueArray()", vt, loopIndex)


getMapKeyArray and getMapValueArray

viirya · 2018-03-02T06:50:14Z

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

@@ -734,23 +734,26 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
    val keyToStringFunc = dataToStringFunc("keyToString", kt)
    val valueToStringFunc = dataToStringFunc("valueToString", vt)
    val loopIndex = ctx.freshName("loopIndex")
+    val getValueMapKeyArray0 = CodeGenerator.getValue(s"$map.keyArray()", kt, "0")
+    val getValueMapValArray0 = CodeGenerator.getValue(s"$map.valueArray()", vt, "0")


getMapFirstKey and getMapFirstValue

viirya · 2018-03-02T06:53:08Z

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

-                ${javaType(elementType)} $elementA = ${getValue("a", elementType, "i")};
-                ${javaType(elementType)} $elementB = ${getValue("b", elementType, "i")};
+                $jt $elementA = ${CodeGenerator.getValue("a", elementType, "i")};
+                $jt $elementB = ${CodeGenerator.getValue("b", elementType, "i")};


Inside CodegenContext, I think we can just do import CodeGenerator._.

SparkQA · 2018-03-02T08:05:01Z

Test build #87881 has finished for PR 20700 at commit 9fed753.

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

rednaxelafx · 2018-03-02T08:20:51Z

Aha! Thanks @kiszk san for working on this! I really wanted the stateless methods to be extracted so that I can use more utils without having to pass around a CodegenContext for no good.

I was actually working on the exact same thing last week but got carried away on other tasks so my patch is still WIP.
For those curious, here's my WIP patch: rednaxelafx@249fb93

I was hoping we can cover the ones I extracted here: rednaxelafx@249fb93#diff-8bcc5aea39c73d4bf38aef6f6951d42cR1201
Your PR and my version extracted mostly the same fields/methods, which is great for me that I'll just help get this PR in instead of having to send mine out.

The approach is different, though. I'd like to have a discussion on the tradeoffs you had in mind when you picked your approach.
I initially did something similar, which is to extract stateless methods into companion object methods and change the original one to delegate to the companion one. But the old instance methods were a bad smell to me (passing in a instance when it shouldn't need the instance), so since we're at it, I thought why not just completely remove that bad smell once and for all.
So I deleted the original instance methods, and yes that made the diff huge because all use sites of those fields/methods have to be touched to switch to using the new version.

@kiszk WDYT? Were you intending to minimize the diff but still want to be able to access the stateless util methods as standalone functions?

kiszk · 2018-03-02T13:45:56Z

@rednaxelafx Oh, very interesting since we are doing the similar thing in West coast and Japan!

I just say not refactored YET. Yeah, I absolutely love to delete old instance method if we totally agree. Is it OK to delete all of old method in this PR?
Some methods that very frequently used (e.g. javaType()) are not refactored yet.

SparkQA · 2018-03-02T14:51:10Z

Test build #87886 has finished for PR 20700 at commit f589a2a.

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

SparkQA · 2018-03-02T14:56:00Z

Test build #87887 has finished for PR 20700 at commit ebce1f2.

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

SparkQA · 2018-03-02T16:29:29Z

Test build #87889 has finished for PR 20700 at commit 7da3e6b.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-02T17:33:14Z

Test build #87894 has finished for PR 20700 at commit 4538ebc.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-02T18:15:58Z

Test build #87897 has finished for PR 20700 at commit 3837eff.

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

SparkQA · 2018-03-02T21:45:46Z

Test build #87899 has finished for PR 20700 at commit 4069e1f.

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

kiszk · 2018-03-03T17:08:16Z

@rednaxelafx thanks, I integrated some of your changes into PR.

hvanhovell · 2018-03-05T00:07:30Z

@kiszk can you update the PR description?

hvanhovell

LGTM - as stated before update the description and then we should be gtg.

kiszk · 2018-03-05T04:28:17Z

Good catch. I removed the old statement from the description.

hvanhovell · 2018-03-05T09:16:04Z

Merging to master. Thanks!

hvanhovell · 2018-03-05T09:45:43Z

I am having some problems with the merge script, give me a little bit of time.

cloud-fan · 2018-03-05T19:41:11Z

a late LGTM :)

initial commit

0b2b381

kiszk commented Mar 1, 2018

View reviewed changes

fix compilation errors

fc4c6ad

fix compilation error

276df59

fix compilation error

f8b478f

kiszk changed the title ~~[SPARK-23546][SQL] Refactor non-stateful methods/values in CodegenContext~~ [SPARK-23546][SQL] Refactor stateless methods/values in CodegenContext Mar 1, 2018

mgaido91 mentioned this pull request Mar 1, 2018

[SPARK-22856][SQL] Add wrappers for codegen output and nullability #20043

Closed

hvanhovell reviewed Mar 1, 2018

View reviewed changes

address review comment

9fed753

viirya reviewed Mar 2, 2018

View reviewed changes

kiszk added 2 commits March 2, 2018 14:27

address review comments

f589a2a

address review comment

ebce1f2

address review comment

7da3e6b

fix style error

4538ebc

fix build failure

3837eff

fix test failures

4069e1f

hvanhovell approved these changes Mar 5, 2018

View reviewed changes

asfgit closed this in 2ce37b5 Mar 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-23546][SQL] Refactor stateless methods/values in CodegenContext #20700

[SPARK-23546][SQL] Refactor stateless methods/values in CodegenContext #20700

kiszk commented Mar 1, 2018 •

edited

Loading

kiszk Mar 1, 2018

SparkQA commented Mar 1, 2018

SparkQA commented Mar 1, 2018

SparkQA commented Mar 1, 2018

SparkQA commented Mar 1, 2018

mgaido91 commented Mar 1, 2018

kiszk commented Mar 1, 2018

hvanhovell Mar 1, 2018

viirya left a comment

viirya Mar 2, 2018

viirya Mar 2, 2018

viirya Mar 2, 2018

viirya Mar 2, 2018

SparkQA commented Mar 2, 2018

rednaxelafx commented Mar 2, 2018

kiszk commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

kiszk commented Mar 3, 2018

hvanhovell commented Mar 5, 2018

hvanhovell left a comment

kiszk commented Mar 5, 2018

hvanhovell commented Mar 5, 2018

hvanhovell commented Mar 5, 2018

cloud-fan commented Mar 5, 2018

[SPARK-23546][SQL] Refactor stateless methods/values in CodegenContext #20700

[SPARK-23546][SQL] Refactor stateless methods/values in CodegenContext #20700

Conversation

kiszk commented Mar 1, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

kiszk Mar 1, 2018

Choose a reason for hiding this comment

SparkQA commented Mar 1, 2018

SparkQA commented Mar 1, 2018

SparkQA commented Mar 1, 2018

SparkQA commented Mar 1, 2018

mgaido91 commented Mar 1, 2018

kiszk commented Mar 1, 2018

hvanhovell Mar 1, 2018

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

viirya Mar 2, 2018

Choose a reason for hiding this comment

viirya Mar 2, 2018

Choose a reason for hiding this comment

viirya Mar 2, 2018

Choose a reason for hiding this comment

viirya Mar 2, 2018

Choose a reason for hiding this comment

SparkQA commented Mar 2, 2018

rednaxelafx commented Mar 2, 2018

kiszk commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

SparkQA commented Mar 2, 2018

kiszk commented Mar 3, 2018

hvanhovell commented Mar 5, 2018

hvanhovell left a comment

Choose a reason for hiding this comment

kiszk commented Mar 5, 2018

hvanhovell commented Mar 5, 2018

hvanhovell commented Mar 5, 2018

cloud-fan commented Mar 5, 2018

kiszk commented Mar 1, 2018 •

edited

Loading