You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by kiszk <gi...@git.apache.org> on 2017/12/05 15:55:31 UTC
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
GitHub user kiszk opened a pull request:
https://github.com/apache/spark/pull/19899
[SPARK-22704][SQL] Least and Greatest use less global variables
## What changes were proposed in this pull request?
This PR accomplishes the following two items.
1. Reduce # of global variables from two to one
2. Make lifetime of global variable local within an operation
1. reduces # of constant pool entries in a Java class. 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`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kiszk/spark SPARK-22704
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19899.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #19899
----
commit 544d23d599fa89b58f7eb394dd034515fd1eba1a
Author: Kazuaki Ishizaki <is...@jp.ibm.com>
Date: 2017-12-05T15:47:16Z
initial commit
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84490 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84490/testReport)** for PR 19899 at commit [`544d23d`](https://github.com/apache/spark/commit/544d23d599fa89b58f7eb394dd034515fd1eba1a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155186924
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +681,35 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("greatestTmpIsNull")
+ ctx.addMutableState(ctx.JAVA_BOOLEAN, isNull)
+ val evals = evalChildren.map(eval =>
s"""
${eval.code}
- if (!${eval.isNull} && (${ev.isNull} ||
+ if (!${eval.isNull} && (${isNull} ||
${ctx.genGreater(dataType, eval.value, ev.value)})) {
- ${ev.isNull} = false;
+ $isNull = false;
${ev.value} = ${eval.value};
}
"""
- }
- val codes = ctx.splitExpressionsWithCurrentInputs(evalChildren.map(updateEval))
+ )
+
+ val resultType = ctx.javaType(dataType)
+ val codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = evals,
+ funcName = "greatest",
+ extraArguments = Seq(resultType -> ev.value),
+ returnType = resultType,
+ makeSplitFunction = body =>
+ s"""
+ |$body
+ |return ${ev.value};
+ """.stripMargin,
+ foldFunctions = _.map(funcCall => s"${ev.value} = $funcCall;").mkString("\n"))
ev.copy(code = s"""
- ${ev.isNull} = true;
- ${ev.value} = ${ctx.defaultValue(dataType)};
- $codes""")
+ $isNull = true;
+ ${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
+ $codes
+ final boolean ${ev.isNull} = $isNull;""")
--- End diff --
nit: can we switch to the new string style (with stripMargin...)?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155133805
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,36 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("isNull")
+ ctx.addMutableState(ctx.JAVA_BOOLEAN, isNull)
+ val evals = evalChildren.map(eval =>
s"""
${eval.code}
- if (!${eval.isNull} && (${ev.isNull} ||
+ if (!${eval.isNull} && (${isNull} ||
${ctx.genGreater(dataType, ev.value, eval.value)})) {
- ${ev.isNull} = false;
+ $isNull = false;
${ev.value} = ${eval.value};
}
"""
- }
- val codes = ctx.splitExpressionsWithCurrentInputs(evalChildren.map(updateEval))
+ )
+
+ val resultType = ctx.javaType(dataType)
+ val codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = evals,
+ funcName = "least",
+ extraArguments = Seq(resultType -> ev.value),
+ returnType = resultType,
+ makeSplitFunction = body =>
+ s"""
+ |$body
+ |return ${ev.value};
--- End diff --
```
$resultType ${ev.value} = default...
$body
$return ${ev.value}
```
We can use a local variable and avoid passing the parameter.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155193147
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +681,35 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("greatestTmpIsNull")
--- End diff --
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19899
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84490 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84490/testReport)** for PR 19899 at commit [`544d23d`](https://github.com/apache/spark/commit/544d23d599fa89b58f7eb394dd034515fd1eba1a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155214885
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +681,35 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("greatestTmpIsNull")
--- End diff --
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?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84560/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84547 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84547/testReport)** for PR 19899 at commit [`47fcdc2`](https://github.com/apache/spark/commit/47fcdc25600b01e664512bdc4f7b570f3d3b8f81).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19899
@cloud-fan @viirya @mgaido91 could you please review this?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84502 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84502/testReport)** for PR 19899 at commit [`d322931`](https://github.com/apache/spark/commit/d322931ae486d0702cb11618ce3609b138ef8351).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84560 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84560/testReport)** for PR 19899 at commit [`e1ed6c1`](https://github.com/apache/spark/commit/e1ed6c155aeef23ac8378ea44934b6d8ab4648da).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155216067
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,38 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val leastTmpIsNull = ctx.freshName("leastTmpIsNull")
--- End diff --
super nit: a convention from your another PR: `val tmpIsNull = ctx.freshName("leastTmpIsNull")`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84547/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84547 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84547/testReport)** for PR 19899 at commit [`47fcdc2`](https://github.com/apache/spark/commit/47fcdc25600b01e664512bdc4f7b570f3d3b8f81).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84556 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84556/testReport)** for PR 19899 at commit [`e5aa90e`](https://github.com/apache/spark/commit/e5aa90e8119bf8dfbf63c2dc3a8ff99f48cd5aaf).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155190300
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +681,35 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("greatestTmpIsNull")
--- End diff --
As I wrote in the description, we would like to make the lifetime of a global variable local.
Sorry for missing addition of this comment.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/19899
LGTM, thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84490/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84502/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84529 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84529/testReport)** for PR 19899 at commit [`d467192`](https://github.com/apache/spark/commit/d46719234bbed9ded30d6f7db6a6a403e9e25f93).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155216636
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +683,37 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val greatestTmpIsNull = ctx.freshName("greatestTmpIsNull")
--- End diff --
ditto
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155145566
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,36 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("leastTmpIsNull")
--- End diff --
`isNull`, `ev.isNull` and `eval.isNull` looks too close.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155187459
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +681,35 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("greatestTmpIsNull")
--- End diff --
I think there is no need for it and we can use `ev.isNull`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155133884
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,36 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("isNull")
+ ctx.addMutableState(ctx.JAVA_BOOLEAN, isNull)
+ val evals = evalChildren.map(eval =>
s"""
${eval.code}
- if (!${eval.isNull} && (${ev.isNull} ||
+ if (!${eval.isNull} && (${isNull} ||
${ctx.genGreater(dataType, ev.value, eval.value)})) {
- ${ev.isNull} = false;
+ $isNull = false;
${ev.value} = ${eval.value};
}
"""
- }
- val codes = ctx.splitExpressionsWithCurrentInputs(evalChildren.map(updateEval))
+ )
+
+ val resultType = ctx.javaType(dataType)
+ val codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = evals,
+ funcName = "least",
+ extraArguments = Seq(resultType -> ev.value),
+ returnType = resultType,
+ makeSplitFunction = body =>
+ s"""
+ |$body
+ |return ${ev.value};
+ """.stripMargin,
+ foldFunctions = _.map(funcCall => s"${ev.value} = $funcCall;").mkString("\n"))
ev.copy(code = s"""
- ${ev.isNull} = true;
- ${ev.value} = ${ctx.defaultValue(dataType)};
- $codes""")
+ $isNull = true;
+ ${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
+ $codes
+ boolean ${ev.isNull} = $isNull;""")
--- End diff --
`final`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84560 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84560/testReport)** for PR 19899 at commit [`e1ed6c1`](https://github.com/apache/spark/commit/e1ed6c155aeef23ac8378ea44934b6d8ab4648da).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84556 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84556/testReport)** for PR 19899 at commit [`e5aa90e`](https://github.com/apache/spark/commit/e5aa90e8119bf8dfbf63c2dc3a8ff99f48cd5aaf).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155145453
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,36 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("isNull")
--- End diff --
`greatestTmpIsNull`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155133648
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,36 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("isNull")
--- End diff --
nit: `leastTmpIsNull`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84502 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84502/testReport)** for PR 19899 at commit [`d322931`](https://github.com/apache/spark/commit/d322931ae486d0702cb11618ce3609b138ef8351).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155145502
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,36 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("leastTmpIsNull")
--- End diff --
`val leastTmpIsNull = ctx.freshName("leastTmpIsNull")`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155190509
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +681,35 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("greatestTmpIsNull")
+ ctx.addMutableState(ctx.JAVA_BOOLEAN, isNull)
+ val evals = evalChildren.map(eval =>
s"""
${eval.code}
- if (!${eval.isNull} && (${ev.isNull} ||
+ if (!${eval.isNull} && (${isNull} ||
${ctx.genGreater(dataType, eval.value, ev.value)})) {
- ${ev.isNull} = false;
+ $isNull = false;
${ev.value} = ${eval.value};
}
"""
- }
- val codes = ctx.splitExpressionsWithCurrentInputs(evalChildren.map(updateEval))
+ )
+
+ val resultType = ctx.javaType(dataType)
+ val codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = evals,
+ funcName = "greatest",
+ extraArguments = Seq(resultType -> ev.value),
+ returnType = resultType,
+ makeSplitFunction = body =>
+ s"""
+ |$body
+ |return ${ev.value};
+ """.stripMargin,
+ foldFunctions = _.map(funcCall => s"${ev.value} = $funcCall;").mkString("\n"))
ev.copy(code = s"""
- ${ev.isNull} = true;
- ${ev.value} = ${ctx.defaultValue(dataType)};
- $codes""")
+ $isNull = true;
+ ${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
+ $codes
+ final boolean ${ev.isNull} = $isNull;""")
--- End diff --
yea, good catch
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84527/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84529/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155145718
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +681,35 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("isNull")
+ ctx.addMutableState(ctx.JAVA_BOOLEAN, isNull)
+ val evals = evalChildren.map(eval =>
s"""
${eval.code}
- if (!${eval.isNull} && (${ev.isNull} ||
+ if (!${eval.isNull} && (${isNull} ||
${ctx.genGreater(dataType, eval.value, ev.value)})) {
- ${ev.isNull} = false;
+ $isNull = false;
${ev.value} = ${eval.value};
}
"""
- }
- val codes = ctx.splitExpressionsWithCurrentInputs(evalChildren.map(updateEval))
+ )
+
+ val resultType = ctx.javaType(dataType)
+ val codes = ctx.splitExpressionsWithCurrentInputs(
+ expressions = evals,
+ funcName = "least",
--- End diff --
greatest
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155213294
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +681,35 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("greatestTmpIsNull")
--- End diff --
IIUC, at line 635 `ev.isNull` is declared as local variable.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155216708
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -668,22 +683,37 @@ case class Greatest(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val greatestTmpIsNull = ctx.freshName("greatestTmpIsNull")
+ ctx.addMutableState(ctx.JAVA_BOOLEAN, greatestTmpIsNull)
+ val evals = evalChildren.map(eval =>
s"""
- ${eval.code}
- if (!${eval.isNull} && (${ev.isNull} ||
- ${ctx.genGreater(dataType, eval.value, ev.value)})) {
- ${ev.isNull} = false;
- ${ev.value} = ${eval.value};
- }
- """
- }
- val codes = ctx.splitExpressionsWithCurrentInputs(evalChildren.map(updateEval))
- ev.copy(code = s"""
- ${ev.isNull} = true;
- ${ev.value} = ${ctx.defaultValue(dataType)};
- $codes""")
+ |${eval.code}
+ |if (!${eval.isNull} && (${greatestTmpIsNull} ||
--- End diff --
ditto
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84527 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84527/testReport)** for PR 19899 at commit [`facaf1c`](https://github.com/apache/spark/commit/facaf1cda0fae41c86a860265b562f38acd63605).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155216344
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,38 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val leastTmpIsNull = ctx.freshName("leastTmpIsNull")
+ ctx.addMutableState(ctx.JAVA_BOOLEAN, leastTmpIsNull)
+ val evals = evalChildren.map(eval =>
s"""
- ${eval.code}
- if (!${eval.isNull} && (${ev.isNull} ||
- ${ctx.genGreater(dataType, ev.value, eval.value)})) {
- ${ev.isNull} = false;
- ${ev.value} = ${eval.value};
- }
- """
- }
- val codes = ctx.splitExpressionsWithCurrentInputs(evalChildren.map(updateEval))
- ev.copy(code = s"""
- ${ev.isNull} = true;
- ${ev.value} = ${ctx.defaultValue(dataType)};
- $codes""")
+ |${eval.code}
+ |if (!${eval.isNull} && (${leastTmpIsNull} ||
--- End diff --
nit `$leastTmpIsNull`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84556/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84529 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84529/testReport)** for PR 19899 at commit [`d467192`](https://github.com/apache/spark/commit/d46719234bbed9ded30d6f7db6a6a403e9e25f93).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19899
thanks, merging to master!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19899
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155187699
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,36 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("leastTmpIsNull")
--- End diff --
yes, but I think we can stay with `ev.isNull` and there is no need for this new variable.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19899
LGTM except some minor style comments
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19899
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19899: [SPARK-22704][SQL] Least and Greatest use less gl...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19899#discussion_r155145590
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala ---
@@ -602,23 +602,36 @@ case class Least(children: Seq[Expression]) extends Expression {
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val evalChildren = children.map(_.genCode(ctx))
- ctx.addMutableState(ctx.JAVA_BOOLEAN, ev.isNull)
- ctx.addMutableState(ctx.javaType(dataType), ev.value)
- def updateEval(eval: ExprCode): String = {
+ val isNull = ctx.freshName("isNull")
--- End diff --
val greatestTmpIsNull = ctx.freshName("greatestTmpIsNull")
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19899: [SPARK-22704][SQL] Least and Greatest use less global va...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19899
**[Test build #84527 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84527/testReport)** for PR 19899 at commit [`facaf1c`](https://github.com/apache/spark/commit/facaf1cda0fae41c86a860265b562f38acd63605).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org