You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by cloud-fan <gi...@git.apache.org> on 2017/10/25 20:35:10 UTC
[GitHub] spark pull request #19576: [SPARK-19727][SQL][followup] Fix for round functi...
GitHub user cloud-fan opened a pull request:
https://github.com/apache/spark/pull/19576
[SPARK-19727][SQL][followup] Fix for round function that modifies original column
## What changes were proposed in this pull request?
This is a followup of https://github.com/apache/spark/pull/17075 , to fix the bug in codegen path.
## How was this patch tested?
new regression test
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark bug
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19576.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 #19576
----
commit cba65d1c72427c9e36eb097dcdf9c2420f66d5db
Author: Wenchen Fan <we...@databricks.com>
Date: 2017-10-25T20:33:40Z
Fix for round function that modifies original column
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19576: [SPARK-19727][SQL][followup] Fix for round functi...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19576#discussion_r147311315
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala ---
@@ -390,7 +390,7 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
*
--- End diff --
Nit: remove this useless line.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19576
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83059/
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 #19576: [SPARK-19727][SQL][followup] Fix for round functi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19576
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19576: [SPARK-19727][SQL][followup] Fix for round functi...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19576#discussion_r147311429
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala ---
@@ -258,6 +258,18 @@ class MathFunctionsSuite extends QueryTest with SharedSQLContext {
)
}
+ test("round/bround with table columns") {
--- End diff --
This is an end-to-end test for code-gen path. Could we add a unit test case in `MathExpressionsSuite`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19576: [SPARK-19727][SQL][followup] Fix for round functi...
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/19576#discussion_r146979542
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala ---
@@ -234,31 +234,28 @@ final class Decimal extends Ordered[Decimal] with Serializable {
changePrecision(precision, scale, ROUND_HALF_UP)
}
- def changePrecision(precision: Int, scale: Int, mode: Int): Boolean = mode match {
- case java.math.BigDecimal.ROUND_HALF_UP => changePrecision(precision, scale, ROUND_HALF_UP)
- case java.math.BigDecimal.ROUND_HALF_EVEN => changePrecision(precision, scale, ROUND_HALF_EVEN)
- }
-
/**
* Create new `Decimal` with given precision and scale.
*
- * @return `Some(decimal)` if successful or `None` if overflow would occur
+ * @return a non-null `Decimal` value if successful or `null` if overflow would occur.
*/
private[sql] def toPrecision(
precision: Int,
scale: Int,
- roundMode: BigDecimal.RoundingMode.Value = ROUND_HALF_UP): Option[Decimal] = {
+ roundMode: BigDecimal.RoundingMode.Value = ROUND_HALF_UP): Decimal = {
--- End diff --
`Option` is hard to use in java code(the codegen path), so I change the return type to nullable `Decimal`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19576
**[Test build #83059 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83059/testReport)** for PR 19576 at commit [`cba65d1`](https://github.com/apache/spark/commit/cba65d1c72427c9e36eb097dcdf9c2420f66d5db).
* 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 #19576: [SPARK-19727][SQL][followup] Fix for round functi...
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/19576#discussion_r147565256
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala ---
@@ -258,6 +258,18 @@ class MathFunctionsSuite extends QueryTest with SharedSQLContext {
)
}
+ test("round/bround with table columns") {
--- End diff --
All the tests in `MathExpressionsSuite` are testing about literals, I think it's better to improve `MathExpressionsSuite` for attributes in a new PR, instead of doing it in a folllow-up PR. BTW the original PR didn't add test in `MathExpressionsSuite` either.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19576
**[Test build #83059 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83059/testReport)** for PR 19576 at commit [`cba65d1`](https://github.com/apache/spark/commit/cba65d1c72427c9e36eb097dcdf9c2420f66d5db).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19576
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19576
**[Test build #83176 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83176/testReport)** for PR 19576 at commit [`2fe8eda`](https://github.com/apache/spark/commit/2fe8eda698ec5c4bec549bf722e1ed4e96cf7313).
* 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 #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19576
cc @wojtek-szymanski @hvanhovell
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19576
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83176/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19576
**[Test build #83176 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83176/testReport)** for PR 19576 at commit [`2fe8eda`](https://github.com/apache/spark/commit/2fe8eda698ec5c4bec549bf722e1ed4e96cf7313).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19576
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 #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19576
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 #19576: [SPARK-19727][SQL][followup] Fix for round function that...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19576
Thanks! Merged to master/2.2
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org