[SPARK-20786][SQL]Improve ceil and floor handle the value which is not expected #18016

heary-cao · 2017-05-17T10:32:30Z

What changes were proposed in this pull request?

spark-sql>SELECT ceil(1234567890123456);
1234567890123456

spark-sql>SELECT ceil(12345678901234567);
12345678901234568

spark-sql>SELECT ceil(123456789012345678);
123456789012345680

when the length of the getText is greater than 16. long to double will be precision loss.

but mysql handle the value is ok.

mysql> SELECT ceil(1234567890123456);
+------------------------+
| ceil(1234567890123456) |
+------------------------+
| 1234567890123456 |
+------------------------+
1 row in set (0.00 sec)

mysql> SELECT ceil(12345678901234567);
+-------------------------+
| ceil(12345678901234567) |
+-------------------------+
| 12345678901234567 |
+-------------------------+
1 row in set (0.00 sec)

mysql> SELECT ceil(123456789012345678);
+--------------------------+
| ceil(123456789012345678) |
+--------------------------+
| 123456789012345678 |
+--------------------------+
1 row in set (0.00 sec)

How was this patch tested?

Supplement the unit test.

srowen · 2017-05-17T11:07:01Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

I think the question is, why is it a valid long but being used as a double (?) elsewhere?
Also you can use methods on BigDecimal to query its precision and scale. toString isn't the right approach

hvanhovell · 2017-05-17T15:09:37Z

+1 on Sean's comment. This is not a parser issue. Can you just fix this by adding LongType to Ceil.inputTypes?

heary-cao · 2017-05-18T03:20:24Z

@hvanhovell @srowen
I have modify it again. and floor is same problem.
review please.
thanks.

gatorsmile · 2017-05-18T18:29:15Z

ok to test

gatorsmile · 2017-05-18T18:30:56Z

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

Could yo move all these new tests to the end of operators.sql?

You can run the following command to generate the result files.

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

ok, add new tests to the end of operators.sql.
please review it again.
thanks.

Could you revert the changes you made in this file MathFunctionsSuite.scala?

OK,
please review it again.
thanks.

SparkQA · 2017-05-18T18:36:08Z

Test build #77061 has finished for PR 18016 at commit 9888e16.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-19T03:23:55Z

Test build #77073 has finished for PR 18016 at commit 68ecf5e.

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

SparkQA · 2017-05-19T03:28:14Z

Test build #77074 has finished for PR 18016 at commit 1f771bd.

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

SparkQA · 2017-05-19T04:58:50Z

Test build #77077 has finished for PR 18016 at commit 8b346e6.

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

SparkQA · 2017-05-19T05:08:25Z

Test build #77078 has finished for PR 18016 at commit 6d51c07.

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

SparkQA · 2017-05-19T06:32:36Z

Test build #77083 has started for PR 18016 at commit 437343e.

SparkQA · 2017-05-19T09:45:08Z

Test build #77087 has finished for PR 18016 at commit 1698e80.

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

gatorsmile · 2017-05-19T15:46:35Z

LGTM except one comment.

SparkQA · 2017-05-22T03:45:41Z

Test build #77162 has finished for PR 18016 at commit 6d8f507.

This patch fails PySpark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2017-05-22T03:48:59Z

Test build #77163 has finished for PR 18016 at commit 6c50fec.

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

SparkQA · 2017-05-22T04:00:41Z

Test build #77164 has finished for PR 18016 at commit f7afebf.

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

gatorsmile · 2017-05-22T04:08:09Z

LGTM

gatorsmile · 2017-05-22T05:40:08Z

Thanks! Merging to master.

Could you submit another PR to backport it to 2.2?

…ot expected ## What changes were proposed in this pull request? spark-sql>SELECT ceil(1234567890123456); 1234567890123456 spark-sql>SELECT ceil(12345678901234567); 12345678901234568 spark-sql>SELECT ceil(123456789012345678); 123456789012345680 when the length of the getText is greater than 16. long to double will be precision loss. but mysql handle the value is ok. mysql> SELECT ceil(1234567890123456); +------------------------+ | ceil(1234567890123456) | +------------------------+ | 1234567890123456 | +------------------------+ 1 row in set (0.00 sec) mysql> SELECT ceil(12345678901234567); +-------------------------+ | ceil(12345678901234567) | +-------------------------+ | 12345678901234567 | +-------------------------+ 1 row in set (0.00 sec) mysql> SELECT ceil(123456789012345678); +--------------------------+ | ceil(123456789012345678) | +--------------------------+ | 123456789012345678 | +--------------------------+ 1 row in set (0.00 sec) ## How was this patch tested? Supplement the unit test. Author: caoxuewen <cao.xuewen@zte.com.cn> Closes apache#18016 from heary-cao/ceil_long.

ueshin · 2017-05-25T07:45:12Z

@heary-cao Hi, I found Ceil and Floor modified by this pr produce different results between interpret and codegen.
We also need to modify doGenCode() to handle LongType correctly.
Can you fix it at a follow-up pr?

rxin · 2017-05-25T10:10:40Z

hm guys please don’t use the end-to-end tests to test expression behavior. use unit tests which automatically tests code gen, interpreted, and different data types.

heary-cao · 2017-05-27T05:48:32Z

@ueshin ,
thank you for your suggestion.
modify doGenCode() pr: #18103

heary-cao changed the title ~~Improve ceil handle the value which is not expected~~ [SPARK-20786][SQL]Improve ceil handle the value which is not expected May 17, 2017

srowen reviewed May 17, 2017

View reviewed changes

heary-cao force-pushed the ceil_long branch from bb72c7c to 2c0c3ac Compare May 18, 2017 02:59

heary-cao changed the title ~~[SPARK-20786][SQL]Improve ceil handle the value which is not expected~~ [SPARK-20786][SQL]Improve ceil and floor handle the value which is not expected May 18, 2017

gatorsmile reviewed May 18, 2017

View reviewed changes

heary-cao force-pushed the ceil_long branch 2 times, most recently from 68ecf5e to 1f771bd Compare May 19, 2017 01:41

heary-cao force-pushed the ceil_long branch from 8b346e6 to 6d51c07 Compare May 19, 2017 03:32

heary-cao force-pushed the ceil_long branch from 6d51c07 to 437343e Compare May 19, 2017 06:29

heary-cao force-pushed the ceil_long branch from 437343e to 1698e80 Compare May 19, 2017 07:23

heary-cao force-pushed the ceil_long branch 2 times, most recently from 6d8f507 to 6c50fec Compare May 22, 2017 01:31

heary-cao added 3 commits May 22, 2017 09:32

fix:add unit test cases

d07f550

Improve floor handle the value which is not expected

842842f

Adds operators.sql test case

f7afebf

heary-cao force-pushed the ceil_long branch from 6c50fec to f7afebf Compare May 22, 2017 01:37

asfgit closed this in 3c9eef3 May 22, 2017

heary-cao mentioned this pull request May 22, 2017

[SPARK-20786][SQL][Backport-2.2]Improve ceil and floor handle the value which is not expected #18057

Closed

[SPARK-20786][SQL]Improve ceil and floor handle the value which is not expected #18016

[SPARK-20786][SQL]Improve ceil and floor handle the value which is not expected #18016

Uh oh!

Conversation

heary-cao commented May 17, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen May 17, 2017

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented May 17, 2017

Uh oh!

heary-cao commented May 18, 2017

Uh oh!

gatorsmile commented May 18, 2017

Uh oh!

gatorsmile May 18, 2017

Choose a reason for hiding this comment

Uh oh!

heary-cao May 19, 2017

Choose a reason for hiding this comment

Uh oh!

gatorsmile May 19, 2017

Choose a reason for hiding this comment

Uh oh!

heary-cao May 22, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 18, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

gatorsmile commented May 19, 2017

Uh oh!

SparkQA commented May 22, 2017

Uh oh!

SparkQA commented May 22, 2017

Uh oh!

SparkQA commented May 22, 2017

Uh oh!

gatorsmile commented May 22, 2017

Uh oh!

gatorsmile commented May 22, 2017

Uh oh!

ueshin commented May 25, 2017

Uh oh!

rxin commented May 25, 2017

Uh oh!

heary-cao commented May 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants