[SPARK-20665][SQL]"Bround" and "Round" function return NULL #17906

10110346 · 2017-05-09T03:21:20Z

What changes were proposed in this pull request?

spark-sql>select bround(12.3, 2);
spark-sql>NULL
For this case, the expected result is 12.3, but it is null.
So ,when the second parameter is bigger than "decimal.scala", the result is not we expected.
"round" function has the same problem. This PR can solve the problem for both of them.

How was this patch tested?

unit test cases in MathExpressionsSuite and MathFunctionsSuite

dongjoon-hyun · 2017-05-09T22:21:57Z

Hi, @10110346 .
What about round?

10110346 · 2017-05-10T00:41:52Z

"round" has the same problem. @dongjoon-hyun . Actually, this PR can solve the problem for both of them

dongjoon-hyun · 2017-05-10T00:43:59Z

Then, let's change the title and description of this PR and the JIRA.
Also, could you add more test cases for round, too?

10110346 · 2017-05-10T00:45:28Z

ok, i will do it, thanks @dongjoon-hyun

10110346 · 2017-05-10T11:59:19Z

Please reveiw it，thanks @dongjoon-hyun @cloud-fan

cloud-fan · 2017-05-11T08:20:26Z

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

@@ -546,15 +546,14 @@ class MathExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
    val bdResults: Seq[BigDecimal] = Seq(BigDecimal(3.0), BigDecimal(3.1), BigDecimal(3.14),
      BigDecimal(3.142), BigDecimal(3.1416), BigDecimal(3.14159),
      BigDecimal(3.141593), BigDecimal(3.1415927))
-    // round_scale > current_scale would result in precision increase
-    // and not allowed by o.a.s.s.types.Decimal.changePrecision, therefore null


seems we are changing the behavior, can you check with other database and see if it's expected to return the original value for this case?

10110346 · 2017-05-11T08:35:21Z

@cloud-fan
I have tested in mysql:
mysql> select round(12.3, 2);
+----------------+
| round(12.3, 2) |
+----------------+
| 12.30 |
+----------------+

Also，I tested in spark1.4, as follows:

select round(12.3,2)
+-------+
| _c0 |
+-------+
| 12.3 |
+-------+

cloud-fan · 2017-05-11T10:13:01Z

LGTM, cc @gatorsmile to double check

gatorsmile · 2017-05-11T17:00:49Z

ok to test

gatorsmile · 2017-05-11T17:52:45Z

Yeah. This is a right fix. I checked Hive and it behaves the same.

hive> SELECT round(3.1415925, 7), round(3.1415925, 8), round(3.1415925, 9), round(3.1415925, 10), round(3.1415925, 100), round(3.1415925, 6), round(null, 8);
OK
3.1415925	3.1415925	3.1415925	3.1415925	3.1415925	3.141593	NULL

Before the fix, we return a wrong result.

spark-sql> SELECT round(3.1415925, 7), round(3.1415925, 8), round(3.1415925, 9), round(3.1415925, 10), round(3.1415925, 100), round(3.1415925, 6), round(null, 8);
3.1415925	NULL	NULL	NULL	NULL	3.141593	NULL

gatorsmile · 2017-05-11T17:56:16Z

sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala

+    checkAnswer(
+      sql(s"SELECT bround($bdPi, 7), bround($bdPi, 8), bround($bdPi, 9), bround($bdPi, 10), " +
+        s"bround($bdPi, 100), bround($bdPi, 6), bround(null, 8)"),
+      Seq(Row(bdPi, bdPi, bdPi, bdPi, bdPi, BigDecimal("3.141592"), null))


How about moving the test case to SQLQueryTestSuite and create a new file mathFunctions.sql. You can refer to s.c.s.t.resources/sql-tests/inputs/cast.sql. To generate the result file, you can run the following command:

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/test-only *SQLQueryTestSuite"

we can do it in a follow-up

SparkQA · 2017-05-11T21:12:37Z

Test build #76820 has finished for PR 17906 at commit 09712e1.

This patch fails from timeout after a configured wait of `250m`.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2017-05-11T21:14:50Z

Retest this please.

SparkQA · 2017-05-11T23:34:08Z

Test build #76832 has finished for PR 17906 at commit 09712e1.

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

cloud-fan · 2017-05-12T00:36:52Z

hi @10110346 , can you set the affected versions more accurate in the JIRA ticket? Does Spark 2.0 has the same issue? We rely on that information to decide which branches we want to backport this patch. Thanks!

10110346 · 2017-05-12T00:44:14Z

@cloud-fan ok, I will do it

10110346 · 2017-05-12T02:18:55Z

@cloud-fan Spark 2.0 and Spark 2.1 have the same issue. I have updated the affected versions in the JIRA. Thanks!

## What changes were proposed in this pull request? spark-sql>select bround(12.3, 2); spark-sql>NULL For this case, the expected result is 12.3, but it is null. So ,when the second parameter is bigger than "decimal.scala", the result is not we expected. "round" function has the same problem. This PR can solve the problem for both of them. ## How was this patch tested? unit test cases in MathExpressionsSuite and MathFunctionsSuite Author: liuxian <liu.xian3@zte.com.cn> Closes #17906 from 10110346/wip_lx_0509. (cherry picked from commit 2b36eb6) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

spark-sql>select bround(12.3, 2); spark-sql>NULL For this case, the expected result is 12.3, but it is null. So ,when the second parameter is bigger than "decimal.scala", the result is not we expected. "round" function has the same problem. This PR can solve the problem for both of them. unit test cases in MathExpressionsSuite and MathFunctionsSuite Author: liuxian <liu.xian3@zte.com.cn> Closes #17906 from 10110346/wip_lx_0509. (cherry picked from commit 2b36eb6) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2017-05-12T03:43:59Z

thanks, merging to master/2.2/2.1/2.0!

spark-sql>select bround(12.3, 2); spark-sql>NULL For this case, the expected result is 12.3, but it is null. So ,when the second parameter is bigger than "decimal.scala", the result is not we expected. "round" function has the same problem. This PR can solve the problem for both of them. unit test cases in MathExpressionsSuite and MathFunctionsSuite Author: liuxian <liu.xian3@zte.com.cn> Closes #17906 from 10110346/wip_lx_0509. (cherry picked from commit 2b36eb6) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

## What changes were proposed in this pull request? spark-sql>select bround(12.3, 2); spark-sql>NULL For this case, the expected result is 12.3, but it is null. So ,when the second parameter is bigger than "decimal.scala", the result is not we expected. "round" function has the same problem. This PR can solve the problem for both of them. ## How was this patch tested? unit test cases in MathExpressionsSuite and MathFunctionsSuite Author: liuxian <liu.xian3@zte.com.cn> Closes apache#17906 from 10110346/wip_lx_0509.

## What changes were proposed in this pull request? add test case to MathExpressionsSuite as apache#17906 ## How was this patch tested? unit test cases Author: liuxian <liu.xian3@zte.com.cn> Closes apache#18082 from 10110346/wip-lx-0524.

spark-sql>select bround(12.3, 2); spark-sql>NULL For this case, the expected result is 12.3, but it is null. So ,when the second parameter is bigger than "decimal.scala", the result is not we expected. "round" function has the same problem. This PR can solve the problem for both of them. unit test cases in MathExpressionsSuite and MathFunctionsSuite Author: liuxian <liu.xian3@zte.com.cn> Closes apache#17906 from 10110346/wip_lx_0509. (cherry picked from commit 2b36eb6) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

"Bround" function return NULL

9b6dab8

10110346 changed the title ~~[SPARK-20665][SQL]"Bround" function return NULL~~ [SPARK-20665][SQL]"Bround" and "Round" function return NULL May 10, 2017

10110346 force-pushed the wip_lx_0509 branch 2 times, most recently from f5ef3e7 to b76f802 Compare May 10, 2017 02:04

Fix:Add unit test case

09712e1

10110346 force-pushed the wip_lx_0509 branch from b76f802 to 09712e1 Compare May 11, 2017 01:54

cloud-fan reviewed May 11, 2017

View reviewed changes

gatorsmile reviewed May 11, 2017

View reviewed changes

asfgit closed this in 2b36eb6 May 12, 2017

10110346 mentioned this pull request May 24, 2017

[SPARK-20665][SQL][FOLLOW-UP]Move test case to MathExpressionsSuite #18082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-20665][SQL]"Bround" and "Round" function return NULL #17906

[SPARK-20665][SQL]"Bround" and "Round" function return NULL #17906

10110346 commented May 9, 2017 •

edited

Loading

dongjoon-hyun commented May 9, 2017

10110346 commented May 10, 2017 •

edited

Loading

dongjoon-hyun commented May 10, 2017

10110346 commented May 10, 2017

10110346 commented May 10, 2017

cloud-fan May 11, 2017

10110346 commented May 11, 2017 •

edited

Loading

cloud-fan commented May 11, 2017

gatorsmile commented May 11, 2017

gatorsmile commented May 11, 2017

gatorsmile May 11, 2017

cloud-fan May 12, 2017

SparkQA commented May 11, 2017

dongjoon-hyun commented May 11, 2017

SparkQA commented May 11, 2017

cloud-fan commented May 12, 2017

10110346 commented May 12, 2017

10110346 commented May 12, 2017

cloud-fan commented May 12, 2017

[SPARK-20665][SQL]"Bround" and "Round" function return NULL #17906

[SPARK-20665][SQL]"Bround" and "Round" function return NULL #17906

Conversation

10110346 commented May 9, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

dongjoon-hyun commented May 9, 2017

10110346 commented May 10, 2017 • edited Loading

dongjoon-hyun commented May 10, 2017

10110346 commented May 10, 2017

10110346 commented May 10, 2017

cloud-fan May 11, 2017

Choose a reason for hiding this comment

10110346 commented May 11, 2017 • edited Loading

cloud-fan commented May 11, 2017

gatorsmile commented May 11, 2017

gatorsmile commented May 11, 2017

gatorsmile May 11, 2017

Choose a reason for hiding this comment

cloud-fan May 12, 2017

Choose a reason for hiding this comment

SparkQA commented May 11, 2017

dongjoon-hyun commented May 11, 2017

SparkQA commented May 11, 2017

cloud-fan commented May 12, 2017

10110346 commented May 12, 2017

10110346 commented May 12, 2017

cloud-fan commented May 12, 2017

10110346 commented May 9, 2017 •

edited

Loading

10110346 commented May 10, 2017 •

edited

Loading

10110346 commented May 11, 2017 •

edited

Loading