[SPARK-22704][SQL] Least and Greatest use less global variables #19899

kiszk · 2017-12-05T15:55:30Z

What changes were proposed in this pull request?

This PR accomplishes the following two items.

Reduce # of global variables from two to one
Make lifetime of global variable local within an operation

Item 1. reduces # of constant pool entries in a Java class. Item 2. ensures that an variable is not passed to arguments in a method split by CodegenContext.splitExpressions(), which is addressed by #19865.

How was this patch tested?

Added new test into ArithmeticExpressionSuite

SparkQA · 2017-12-05T18:38:31Z

Test build #84490 has finished for PR 19899 at commit 544d23d.

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

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

Test build #84502 has finished for PR 19899 at commit d322931.

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

kiszk · 2017-12-06T01:00:00Z

@cloud-fan @viirya @mgaido91 could you please review this?

cloud-fan · 2017-12-06T02:37:46Z

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

-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
-    def updateEval(eval: ExprCode): String = {
+    val isNull = ctx.freshName("isNull")


nit: leastTmpIsNull

greatestTmpIsNull?

val greatestTmpIsNull = ctx.freshName("greatestTmpIsNull")

cloud-fan · 2017-12-06T02:40:07Z

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

+      $isNull = true;
+      ${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
+      $codes
+      boolean ${ev.isNull} = $isNull;""")


viirya · 2017-12-06T04:40:57Z

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

-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
-    def updateEval(eval: ExprCode): String = {
+    val isNull = ctx.freshName("leastTmpIsNull")


val leastTmpIsNull = ctx.freshName("leastTmpIsNull")

isNull, ev.isNull and eval.isNull looks too close.

yes, but I think we can stay with ev.isNull and there is no need for this new variable.

viirya · 2017-12-06T04:43:49Z

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

+    val resultType = ctx.javaType(dataType)
+    val codes = ctx.splitExpressionsWithCurrentInputs(
+      expressions = evals,
+      funcName = "least",


SparkQA · 2017-12-06T05:53:32Z

Test build #84527 has finished for PR 19899 at commit facaf1c.

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

SparkQA · 2017-12-06T08:04:29Z

Test build #84529 has finished for PR 19899 at commit d467192.

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

mgaido91 · 2017-12-06T09:43:21Z

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

+      $isNull = true;
+      ${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
+      $codes
+      final boolean ${ev.isNull} = $isNull;""")


nit: can we switch to the new string style (with stripMargin...)?

yea, good catch

mgaido91 · 2017-12-06T09:45:33Z

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

-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
-    def updateEval(eval: ExprCode): String = {
+    val isNull = ctx.freshName("greatestTmpIsNull")


I think there is no need for it and we can use ev.isNull

As I wrote in the description, we would like to make the lifetime of a global variable local.
Sorry for missing addition of this explanation.

I am not sure about this. Because anyway, if ev.isNull is a global variable, here the problem would still exist since we are declaring it in the method (line 635). So I don't see how this changes/solves the problem.

IIUC, at line 635 ev.isNull is declared as local variable.

yes, thus if it is declared as global variable, we still have a global variable with the same name of a local one. Am I missing something?

cloud-fan · 2017-12-06T11:55:44Z

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

-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
-    def updateEval(eval: ExprCode): String = {
+    val leastTmpIsNull = ctx.freshName("leastTmpIsNull")


super nit: a convention from your another PR: val tmpIsNull = ctx.freshName("leastTmpIsNull")

cloud-fan · 2017-12-06T11:57:04Z

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

-      ${ev.value} = ${ctx.defaultValue(dataType)};
-      $codes""")
+         |${eval.code}
+         |if (!${eval.isNull} && (${leastTmpIsNull} ||


nit $leastTmpIsNull

cloud-fan · 2017-12-06T11:58:31Z

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

-    ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
-    ctx.addMutableState(ctx.javaType(dataType), ev.value)
-    def updateEval(eval: ExprCode): String = {
+    val greatestTmpIsNull = ctx.freshName("greatestTmpIsNull")


cloud-fan · 2017-12-06T11:58:52Z

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

-      ${ev.value} = ${ctx.defaultValue(dataType)};
-      $codes""")
+         |${eval.code}
+         |if (!${eval.isNull} && (${greatestTmpIsNull} ||


cloud-fan · 2017-12-06T12:00:43Z

LGTM except some minor style comments

SparkQA · 2017-12-06T12:52:03Z

Test build #84547 has finished for PR 19899 at commit 47fcdc2.

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

SparkQA · 2017-12-06T13:42:54Z

Test build #84556 has finished for PR 19899 at commit e5aa90e.

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

cloud-fan · 2017-12-06T14:18:15Z

LGTM

mgaido91 · 2017-12-06T15:35:04Z

LGTM, thanks.

SparkQA · 2017-12-06T16:41:59Z

Test build #84560 has finished for PR 19899 at commit e1ed6c1.

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

cloud-fan · 2017-12-06T16:46:04Z

thanks, merging to master!

kiszk added 2 commits December 6, 2017 04:09

initial commit

bd3b4c7

remove print

d322931

kiszk force-pushed the SPARK-22704 branch from 544d23d to d322931 Compare December 5, 2017 19:11

cloud-fan reviewed Dec 6, 2017

View reviewed changes

address review comments

facaf1c

viirya reviewed Dec 6, 2017

View reviewed changes

kiszk added 2 commits December 6, 2017 05:11

address review comments

d467192

address review comments

e9a2202

mgaido91 reviewed Dec 6, 2017

View reviewed changes

address review comments

47fcdc2

cloud-fan reviewed Dec 6, 2017

View reviewed changes

address review comment

e5aa90e

fix build failure

e1ed6c1

asfgit closed this in 813c0f9 Dec 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22704][SQL] Least and Greatest use less global variables #19899

[SPARK-22704][SQL] Least and Greatest use less global variables #19899

kiszk commented Dec 5, 2017 •

edited

Loading

SparkQA commented Dec 5, 2017

SparkQA commented Dec 5, 2017

kiszk commented Dec 6, 2017

cloud-fan Dec 6, 2017

viirya Dec 6, 2017

viirya Dec 6, 2017

cloud-fan Dec 6, 2017

viirya Dec 6, 2017

viirya Dec 6, 2017

mgaido91 Dec 6, 2017

viirya Dec 6, 2017

SparkQA commented Dec 6, 2017

SparkQA commented Dec 6, 2017

mgaido91 Dec 6, 2017

kiszk Dec 6, 2017

mgaido91 Dec 6, 2017

kiszk Dec 6, 2017 •

edited

Loading

mgaido91 Dec 6, 2017

kiszk Dec 6, 2017

mgaido91 Dec 6, 2017

cloud-fan Dec 6, 2017

cloud-fan Dec 6, 2017

cloud-fan Dec 6, 2017

cloud-fan Dec 6, 2017

cloud-fan commented Dec 6, 2017

SparkQA commented Dec 6, 2017

SparkQA commented Dec 6, 2017

cloud-fan commented Dec 6, 2017

mgaido91 commented Dec 6, 2017

SparkQA commented Dec 6, 2017

cloud-fan commented Dec 6, 2017

[SPARK-22704][SQL] Least and Greatest use less global variables #19899

[SPARK-22704][SQL] Least and Greatest use less global variables #19899

Conversation

kiszk commented Dec 5, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Dec 5, 2017

SparkQA commented Dec 5, 2017

kiszk commented Dec 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 6, 2017

SparkQA commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiszk Dec 6, 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

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

cloud-fan commented Dec 6, 2017

SparkQA commented Dec 6, 2017

SparkQA commented Dec 6, 2017

cloud-fan commented Dec 6, 2017

mgaido91 commented Dec 6, 2017

SparkQA commented Dec 6, 2017

cloud-fan commented Dec 6, 2017

kiszk commented Dec 5, 2017 •

edited

Loading

kiszk Dec 6, 2017 •

edited

Loading