[SPARK-22750][SQL] Reuse mutable states when possible #19940

mgaido91 · 2017-12-10T13:27:49Z

What changes were proposed in this pull request?

The PR introduces a new method addImmutableStateIfNotExists to CodeGenerator to allow reusing and sharing the same global variable between different Expressions. This helps reducing the number of global variables needed, which is important to limit the impact on the constant pool.

How was this patch tested?

added UTs

SparkQA · 2017-12-10T16:21:42Z

Test build #84695 has finished for PR 19940 at commit 978bfd6.

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

mgaido91 · 2017-12-11T10:36:10Z

@cloud-fan @kiszk @viirya might you please help reviewing this? Thanks.

viirya · 2017-12-11T10:44:25Z

A high level question is, do we need to share mutable status, if we can compact global variables into array later?

Will sharing mutable status increase the difficulty of debugging codegen in the future?

mgaido91 · 2017-12-11T10:56:41Z

@viirya we have seen that using arrays affects performance. Thus if we can reduce their usage it is better.

I don't think that debugging is harder. These variables I made shared are never assigned, but in the initialization. Do you have an other opinion? Or are you thinking for something specific?

cloud-fan · 2017-12-11T11:47:23Z

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

+  def addSingleMutableState(
+      javaType: String,
+      variableName: String,
+      initCode: String = ""): Unit = {


How can we support different initCode for the same name?

It is not supported.

cloud-fan · 2017-12-11T12:03:36Z

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

+      variableName: String,
+      initCode: String = ""): Unit = {
+    if (!singleMutableStates.contains(variableName)) {
+      addMutableState(javaType, variableName, initCode)


shall we add an assert here to make sure initCode is same with the previous one?

if you want, I can add it.

Please also check if the java type is the same. If one expression uses the same name with different type, we should alert it early.

viirya · 2017-12-11T13:23:24Z

@mgaido91 Do you mean the shared global variables are required to be only assigned once (initialization) and never changed again?

mgaido91 · 2017-12-11T13:27:36Z

@viirya this is the requirement I followed in this change which ensures that it is safe to share the variable across all the operators, since all the access are read only and there cannot be influences. Maybe this might be relaxed in the future, but if we follow this requirement, we are sure that this is safe.

viirya · 2017-12-11T13:30:34Z

Shall we make them as final variables to guarantee this? I think this is an important requirement to prevent something wrong when wrongly using the shared global variables.

viirya · 2017-12-11T13:31:38Z

Oh, the initialization is not right away in declaration.

SparkQA · 2017-12-11T18:16:31Z

Test build #84714 has finished for PR 19940 at commit ce76015.

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

kiszk · 2017-12-11T18:33:35Z

I have one question. We are implementing #19811 to compact mutable states. When it will be merged, does this PR can reduce large number of constant pool entries?

mgaido91 · 2017-12-11T20:07:14Z

@kiszk for instance it can remove one entry for every timestamp function (to_timestamp or from_utc_timestamp). Of course #19811 is the most important PR, because it solves the problem. But I think we all agree that if we can avoid to waste global variables it is better and there have been many PRs to avoid the usage of global variables. This is one of the many.

mgaido91 · 2017-12-12T15:45:10Z

Jenkins, retest this please

kiszk · 2017-12-12T16:04:25Z

Got it. It seems to be . I have two questions.

Is this mutable? It looks it is global, but immutable. IIUC, is there any other better API name?
As @viirya pointed out here, why don't we declare final?

mgaido91 · 2017-12-12T16:14:38Z

Yes, to ensure that there are no problems, variables' value should not be changed. I agree that we can find a better API name, to enforce this: what about addImmutableStateIfNotExists? Or do you have any suggestion?
Because as @viirya pointed out here, they are not initialized in the declaration, but in the init method of the generated class, thus it cannot be made final unfortunately.

kiszk · 2017-12-12T18:03:19Z

For 2., I noticed there are two types of initialization. One is in init method by using addPartitionInitializationStatement. The other is in constructor that includes an initialization statement in addSingleMutableState. Can we declare final for latter cases?

SparkQA · 2017-12-12T18:41:51Z

Test build #84775 has finished for PR 19940 at commit ce76015.

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

mgaido91 · 2017-12-13T10:39:42Z

I don't think so, it would be very risky, since splitExpressions might put the initialization outside the constructor.
What about 1? Any thought?

SparkQA · 2017-12-13T13:40:19Z

Test build #84846 has finished for PR 19940 at commit 90f517c.

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

mgaido91 · 2017-12-15T08:49:10Z

@cloud-fan @kiszk @viirya any more comments on this?

kiszk · 2017-12-15T09:40:19Z

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

@@ -170,6 +170,14 @@ class CodegenContext {
  val mutableStates: mutable.ArrayBuffer[(String, String, String)] =
    mutable.ArrayBuffer.empty[(String, String, String)]

+  /**
+   * A map containing the mutable states which have been defined so far using
+   * `addSingleMutableState`. Each entry contains the name of the mutable state as key and its


Is addSingleMutableState old one?

kiszk · 2017-12-15T09:40:44Z

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

@@ -401,4 +401,16 @@ class CodeGenerationSuite extends SparkFunSuite with ExpressionEvalHelper {
    ctx.addReferenceObj("foo", foo)
    assert(ctx.mutableStates.isEmpty)
  }
+
+  test("SPARK-22750: addSingleMutableState") {


Is addSingleMutableState old one?

yes, thanks, good catch!

SparkQA · 2017-12-15T13:31:25Z

Test build #84956 has finished for PR 19940 at commit e2725d2.

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

mgaido91 · 2017-12-15T13:38:37Z

the test failure is not related to the PR. It looks the same R failure we had some days ago, I thought it was solved:

checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) : 
  dims [product 22] do not match the length of object [0]

mgaido91 · 2017-12-15T18:30:06Z

Jenkins, retest this please

SparkQA · 2017-12-15T21:21:01Z

Test build #84972 has finished for PR 19940 at commit e2725d2.

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

SparkQA · 2017-12-20T21:26:02Z

Test build #85202 has finished for PR 19940 at commit c650196.

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

SparkQA · 2017-12-21T01:01:16Z

Test build #85212 has finished for PR 19940 at commit 6143b22.

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

mgaido91 · 2017-12-21T09:48:44Z

Jenkins, retest this please

mgaido91 · 2017-12-21T10:52:28Z

the test errors are unrelated to this change. Any other comments @cloud-fan @kiszk @viirya ?

SparkQA · 2017-12-21T12:16:26Z

Test build #85252 has finished for PR 19940 at commit 6143b22.

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

mgaido91 · 2017-12-21T12:18:29Z

Jenkins, retest this please

SparkQA · 2017-12-21T15:10:55Z

Test build #85261 has finished for PR 19940 at commit 6143b22.

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

cloud-fan · 2017-12-21T16:42:24Z

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

+   * `addImmutableStateIfNotExists`. Each entry contains the name of the mutable state as key and
+   * its Java type and init code as value.
+   */
+  val singleMutableStates: mutable.Map[String, (String, String)] =


immutableStates?

cloud-fan · 2017-12-21T16:46:25Z

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

@@ -1193,7 +1196,8 @@ case class ToUTCTimestamp(left: Expression, right: Expression)
        val dtu = DateTimeUtils.getClass.getName.stripSuffix("$")
        val tzTerm = ctx.addMutableState(tzClass, "tz",
          v => s"""$v = $dtu.getTimeZone("$tz");""")
-        val utcTerm = ctx.addMutableState(tzClass, "utc",
+        val utcTerm = "tzUTC"
+        ctx.addImmutableStateIfNotExists(tzClass, utcTerm,
          v => s"""$v = $dtu.getTimeZone("UTC");""")


unrelated question: in the codebase sometimes we use UTC sometimes we use GMT, is it corrected?

yes, there is no difference between them in practice. But I think that being consistent would be better for readability

cloud-fan · 2017-12-21T16:47:36Z

LGTM

SparkQA · 2017-12-21T20:36:10Z

Test build #85273 has finished for PR 19940 at commit 7397cf3.

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

cloud-fan · 2017-12-22T02:13:39Z

thanks, merging to master!

[SPARK-22750][SQL] Reuse mutable states when possible

978bfd6

cloud-fan reviewed Dec 11, 2017

View reviewed changes

Add assertions

ce76015

mgaido91 added 2 commits December 13, 2017 11:48

Merge branch 'master' of github.com:apache/spark into SPARK-22750

c3e30a0

rename to addImmutableStateIfNotExists

90f517c

kiszk reviewed Dec 15, 2017

View reviewed changes

remove references to the old name addSingleMutableState

e2725d2

mgaido91 added 2 commits December 20, 2017 20:11

Merge branch 'master' of github.com:apache/spark into SPARK-22750

bc73954

remove useless tests

c650196

fix merge error

6143b22

cloud-fan reviewed Dec 21, 2017

View reviewed changes

renaming

7397cf3

asfgit closed this in c6f01ca Dec 22, 2017

[SPARK-22750][SQL] Reuse mutable states when possible #19940

[SPARK-22750][SQL] Reuse mutable states when possible #19940

Conversation

mgaido91 commented Dec 10, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Dec 10, 2017

mgaido91 commented Dec 11, 2017

viirya commented Dec 11, 2017

mgaido91 commented Dec 11, 2017

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

Choose a reason for hiding this comment

viirya commented Dec 11, 2017

mgaido91 commented Dec 11, 2017 • edited Loading

viirya commented Dec 11, 2017

viirya commented Dec 11, 2017

SparkQA commented Dec 11, 2017

kiszk commented Dec 11, 2017 • edited Loading

mgaido91 commented Dec 11, 2017

mgaido91 commented Dec 12, 2017

kiszk commented Dec 12, 2017

mgaido91 commented Dec 12, 2017 • edited Loading

kiszk commented Dec 12, 2017

SparkQA commented Dec 12, 2017

mgaido91 commented Dec 13, 2017

SparkQA commented Dec 13, 2017

mgaido91 commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 15, 2017

mgaido91 commented Dec 15, 2017

mgaido91 commented Dec 15, 2017

SparkQA commented Dec 15, 2017

SparkQA commented Dec 20, 2017

SparkQA commented Dec 21, 2017

mgaido91 commented Dec 21, 2017

mgaido91 commented Dec 21, 2017

SparkQA commented Dec 21, 2017

mgaido91 commented Dec 21, 2017

SparkQA commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Dec 21, 2017

SparkQA commented Dec 21, 2017

cloud-fan commented Dec 22, 2017

mgaido91 commented Dec 10, 2017 •

edited

Loading

mgaido91 commented Dec 11, 2017 •

edited

Loading

kiszk commented Dec 11, 2017 •

edited

Loading

mgaido91 commented Dec 12, 2017 •

edited

Loading