[SPARK-37324][SQL] Adds support for decimal rounding mode up, down, half_down by sathiyapk · Pull Request #34593 · apache/spark

sathiyapk · 2021-11-14T23:07:33Z

What changes were proposed in this pull request?

Adds support for Decimal RoundingMode.UP, DOWN and HALF_DOWN by letting users to pass the rounding mode as argument to round function.
bround function calls round function with the argument "half_even"

Why are the changes needed?

Currently there's no easier and straight forward way to round decimals with the mode UP, DOWN and HALF_DOWN. People need to use UDF or complex operations to do the same.

Opening support for the other rounding modes might interest a lot of use cases.

SAP Hana Sql ROUND function does it :

ROUND(<number> [, <position> [, <rounding_mode>]])

REF : https://help.sap.com/viewer/7c78579ce9b14a669c1f3295b0d8ca16/Cloud/en-US/20e6a27575191014bd54a07fd86c585d.html

Sql Server does something similar to this :

ROUND ( numeric_expression , length [ ,function ] )

REF : https://docs.microsoft.com/en-us/sql/t-sql/functions/round-transact-sql?view=sql-server-ver15

Does this PR introduce any user-facing change?

Now, users can specify the rounding mode while calling round function for round modes up, down, half_down. Calling round function without rounding mode will default to half_up.

> SELECT round(3.145, 2)
3.15

>SELECT round(3.145, 2, 'down')
3.14

df.withColumn("value_rounded", round('value, 0)

df.withColumn("value_rounded", round('value, 0, "down")

How was this patch tested?

This patch was tested locally using unit test and git workflow.

HyukjinKwon · 2021-11-15T00:00:05Z

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

qq: is there any reference of DBMSes that supports down, up, etc?

sql-server does something similar :
https://docs.microsoft.com/en-us/sql/t-sql/functions/round-transact-sql?view=sql-server-ver15

Can we match the argument and usage with other references? I think we don't have to bother to come up with spark's own argument style.

Could you please give me an example ?

I currently don't - I haven't taken a look around other DBMSes on that.

SAP Hana SQL ROUND function takes rounding mode as argument as we do : https://help.sap.com/viewer/7c78579ce9b14a669c1f3295b0d8ca16/Cloud/en-US/20e6a27575191014bd54a07fd86c585d.html

ROUND(<number> [, <position> [, <rounding_mode>]])

Hmm, there seems to be no standard / de-fact standard implementation of round which has the third argument.
As you mention, SAP Hana and MS SQL Server allow the third argument for round but they are still ununiform.

sathiyapk · 2021-11-15T00:25:09Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala

Not sure about the changePrecision method for ROUND_UP, ROUND_DOWN and ROUND_HALF_DOWN. If anyone could help me on this that would be great.

@HyukjinKwon Also do you have any input on this please ? I need to integrate that in DecimalSuite.scala.

In the current implementation, changePrecision should not take ROUND_UP, ROUND_DOWN and ROUND_HALF_DOWN. If we have such case, it's a bug so how about having an assertion before this match expression or simply falling to the default _ branch?

Hi @sarutak thanks for your review. Out of curiosity, may i know why it will be a bug if changePrecision takes ROUND_UP, ROUND_DOWN or ROUND_HALF_DOWN ?

So we could do something like this :

case ROUND_UP | ROUND_DOWN | ROUND_HALF_DOWN | _ => throw QueryExecutionErrors.unsupportedRoundingMode(roundMode)

Out of curiosity, may i know why it will be a bug if changePrecision takes ROUND_UP, ROUND_DOWN or ROUND_HALF_DOWN ?

It's not expected that ROUND_UP, ROUND_DOWN and ROUND_HALF_DOWN are not expected to be passed to changePrecision and it's private.

sathiyapk · 2021-11-15T20:29:11Z

Test Fails on compatibility check against spark-sql_2.12:3.2.0! but it is normal, no ? any idea how can we fix it ?

error] spark-sql: Failed binary compatibility check against org.apache.spark:spark-sql_2.12:3.2.0! Found 2 potential problems (filtered 203)
[error] * static method round(org.apache.spark.sql.Column,Int)org.apache.spark.sql.Column in class org.apache.spark.sql.functions does not have a correspondent in current version
[error] filter with: ProblemFilters.excludeDirectMissingMethodProblem
[error] * method round(org.apache.spark.sql.Column,Int)org.apache.spark.sql.Column in object org.apache.spark.sql.functions does not have a correspondent in current version

sathiyapk · 2021-11-15T20:37:01Z

@gengliangwang Could you please kindly take a look at this ? thank you very much.

HyukjinKwon · 2021-11-16T01:42:43Z

ok to test

HyukjinKwon · 2021-11-16T01:43:38Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

This breaks binary compatibility in Java because the default argument doesn't work. we should overload it.

Thanks, that fixed it. I'm currently fixing the schema mismatch fails. But i'm not sure why the kubernetes integration test is failing, do you have any input on that, please ?

SparkQA · 2021-11-16T02:01:14Z

Test build #145261 has finished for PR 34593 at commit 3b1357b.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-11-16T02:43:06Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49730/

SparkQA · 2021-11-16T03:29:28Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49730/

SparkQA · 2021-11-16T16:22:22Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49752/

SparkQA · 2021-11-16T17:17:11Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49752/

SparkQA · 2021-11-16T23:57:51Z

Test build #145282 has finished for PR 34593 at commit 0366617.

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

HyukjinKwon · 2021-11-17T00:53:10Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

would have to be 3.3.0

SparkQA · 2021-11-17T15:30:50Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49810/

SparkQA · 2021-11-17T16:14:46Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49810/

SparkQA · 2021-11-17T16:19:46Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49812/

SparkQA · 2021-11-17T17:04:44Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49812/

SparkQA · 2021-11-17T22:08:25Z

Test build #145350 has finished for PR 34593 at commit 5dc12af.

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

SparkQA · 2021-11-17T22:25:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49821/

SparkQA · 2021-11-17T22:56:48Z

Test build #145339 has finished for PR 34593 at commit 36538d1.

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

SparkQA · 2021-11-17T23:13:23Z

Test build #145341 has finished for PR 34593 at commit 97220a2.

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

SparkQA · 2021-11-17T23:24:24Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49821/

SparkQA · 2021-11-23T22:57:17Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50023/

SparkQA · 2021-11-23T23:42:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50024/

SparkQA · 2021-11-23T23:56:03Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50023/

SparkQA · 2021-11-24T00:39:59Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50024/

SparkQA · 2021-11-24T01:05:41Z

Test build #145552 has finished for PR 34593 at commit ed83e8b.

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

SparkQA · 2021-11-24T03:14:09Z

Test build #145551 has finished for PR 34593 at commit b7c2cc6.

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

SparkQA · 2021-11-25T01:00:21Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50069/

SparkQA · 2021-11-25T02:01:03Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50069/

SparkQA · 2021-11-25T04:04:56Z

Test build #145597 has finished for PR 34593 at commit e7f6261.

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

gengliangwang · 2021-11-25T06:02:46Z

sql/core/src/test/resources/sql-functions/sql-expression-schema.md

 | org.apache.spark.sql.catalyst.expressions.Right | right | SELECT right('Spark SQL', 3) | struct<right(Spark SQL, 3):string> |
 | org.apache.spark.sql.catalyst.expressions.Rint | rint | SELECT rint(12.3456) | struct<rint(12.3456):double> |
-| org.apache.spark.sql.catalyst.expressions.Round | round | SELECT round(2.5, 0) | struct<round(2.5, 0):decimal(2,0)> |
+| org.apache.spark.sql.catalyst.expressions.Round | round | SELECT round(2.5, 0) | struct<round(2.5, 0, half_up):decimal(2,0)> |


Not every DBMS systems supports the third parameter. Some of the external connectors are relying on the SQL representation string of expressions.
So, shall we hide the 3rd parameter if it is the default "half_up" ?

Yes, we can do that. I'll wait for the opinions of other committers too.

gengliangwang · 2021-11-25T06:36:30Z

Sql Server does something similar to this :

ROUND ( numeric_expression , length [ ,function ] )

REF : https://docs.microsoft.com/en-us/sql/t-sql/functions/round-transact-sql?view=sql-server-ver15

This doesn't seem not related to the proposal in this PR.
Is there any other DBMS supporting the rounding mode as 3rd parameter?

sarutak · 2021-11-25T07:13:30Z

I investigated whether the major DBMS and query processing systems (PostgreSQL, BigQuery, Snowflake, MySQL, SAP HANA, MS SQL Server, Firebird, DB2, Teradata, Hive) support the third parameter of round but I found only SAP HANA and MS SQL Server do.

@sathiyapk
How many users need to change the rounding modes and how often the additional rounding modes are used?
If users really need up, down and half_down, I think it's not difficult to implement a UDF by using BigDecimal.setScale.

@HyukjinKwon @gengliangwang What do you think?

gengliangwang · 2021-11-25T07:34:44Z

@sarutak yeah I did some investigations too. This feature seems not commonly used.

cloud-fan · 2021-11-25T09:00:16Z

I think people can just use floor and ceil functions if they need a different rounding mode?

gengliangwang · 2021-11-25T10:23:54Z

I think people can just use floor and ceil functions if they need a different rounding mode?

floor and ceil can't control the position of rounding directly

sathiyapk · 2021-11-25T10:49:51Z

@sarutak Currently, we are relying on UDF for rounding, when i googled to do it via spark native methods, i found a lot of people have the same issue and it's not complex to support it via spark native methods, so i opened this PR.

sathiyapk · 2021-11-25T10:52:26Z

@cloud-fan @gengliangwang I confirm floor and ceil don't match our use case.

cloud-fan · 2021-11-25T11:43:52Z

Then can we add a scale parameter to floor and ceil? This leads to better API design IMO: we have 3 functions for different rounding modes, and each function takes a scale parameter.

sathiyapk · 2021-11-25T13:40:53Z

@cloud-fan I like the idea of adding scale parameter to floor and ceil. This would simulate the rounding mode UP and down for positive integers, but for negative integers it will work differently i believe ?

cloud-fan · 2021-11-25T16:43:33Z

@sathiyapk you are right, but I can also argue that the round function with a rounding mode can not do what I want by using the floor/ceil function with a scale parameter. The problem is which behavior is more common/reasonable and can be useful to more users?

We can collect more data points, I found one example that snowflake supports scale parameter in floor/ceil: https://docs.snowflake.com/en/sql-reference/functions/floor.html

sathiyapk · 2021-11-25T19:09:32Z

@cloud-fan In our case, the first function we looked into was floor/ceil but since it didn't take the scale parameter we had to look for other ways. So i believe adding a scale parameter to floor/ceil is a good idea and also I believe the rounding of negative numbers are not so common.

If everyone agree I will open a new PR for the scale parameter to floor/ceil function.

sarutak · 2021-11-25T19:19:46Z

+1 for adding the scale parameter to floor and ceil.

sathiyapk · 2021-11-25T19:27:00Z

@gengliangwang @HyukjinKwon what do you think ?

gengliangwang · 2021-11-26T02:55:03Z

+1 for adding the scale parameter to floor and ceil.

### What changes were proposed in this pull request? Adds `scale` parameter to `floor`/`ceil` functions in order to allow users to control the rounding position. This feature is proposed in the PR: #34593 ### Why are the changes needed? Currently we support Decimal RoundingModes : HALF_UP (round) and HALF_EVEN (bround). But we have use cases that needs RoundingMode.UP and RoundingMode.DOWN. Floor and Ceil functions helps to do this but it doesn't support the position of the rounding. Adding scale parameter to the functions would help us control the rounding positions. Snowflake supports `scale` parameter to `floor`/`ceil` : ` FLOOR( <input_expr> [, <scale_expr> ] )` REF: https://docs.snowflake.com/en/sql-reference/functions/floor.html ### Does this PR introduce _any_ user-facing change? Now users can pass `scale` parameter to the `floor` and `ceil` functions. ``` > SELECT floor(-0.1); -1.0 > SELECT floor(5); 5 > SELECT floor(3.1411, 3); 3.141 > SELECT floor(3.1411, -3); 1000.0 > SELECT ceil(-0.1); 0.0 > SELECT ceil(5); 5 > SELECT ceil(3.1411, 3); 3.142 > SELECT ceil(3.1411, -3); 1000.0 ``` ### How was this patch tested? This patch was tested locally using unit test and git workflow. Closes #34729 from sathiyapk/SPARK-37475-floor-ceil-scale. Authored-by: Sathiya KUMAR <ext.sathiyaprabhu.kumar@sncf.fr> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

github-actions · 2022-03-07T00:15:48Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

github-actions bot added the SQL label Nov 14, 2021

HyukjinKwon changed the title ~~SPARK-37324 Adds support for decimal rounding mode up, down, half_down~~ [SPARK-37324][SQL] Adds support for decimal rounding mode up, down, half_down Nov 14, 2021

HyukjinKwon reviewed Nov 15, 2021

View reviewed changes

sathiyapk commented Nov 15, 2021

View reviewed changes

HyukjinKwon reviewed Nov 16, 2021

View reviewed changes

HyukjinKwon reviewed Nov 17, 2021

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated

Copy link

Member

HyukjinKwon Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would have to be 3.3.0

sathiyapk added 6 commits November 18, 2021 21:41

SPARK-37324 Adds support for decimal rounding mode up, down, half_down

fc6a481

SPARK-37324 Adds support for decimal rounding mode up, down, half_down

647addb

SPARK-37324 Adds support for decimal rounding mode up, down, half_down

0300396

SPARK-37324 Adds support for decimal rounding mode up, down, half_down

1e3ec88

SPARK-37324 Modifies round function schemas in unit test

091586d

SPARK-37324 Modifies round function schemas in unit test

42dbf72

SPARK-37324 Addresses review comments

e7f6261

gengliangwang reviewed Nov 25, 2021

View reviewed changes

sathiyapk mentioned this pull request Nov 28, 2021

[SPARK-37475][SQL] Add scale parameter to floor and ceil functions #34729

Closed

github-actions bot added the Stale label Mar 7, 2022

github-actions bot closed this Mar 8, 2022

Conversation

sathiyapk commented Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sathiyapk Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sathiyapk Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sathiyapk commented Nov 15, 2021

Uh oh!

sathiyapk commented Nov 15, 2021

Uh oh!

HyukjinKwon commented Nov 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

SparkQA commented Nov 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 17, 2021

Uh oh!

SparkQA commented Nov 17, 2021

Uh oh!

SparkQA commented Nov 17, 2021

Uh oh!

SparkQA commented Nov 17, 2021

Uh oh!

SparkQA commented Nov 17, 2021

Uh oh!

SparkQA commented Nov 17, 2021

Uh oh!

SparkQA commented Nov 17, 2021

Uh oh!

sathiyapk commented Nov 14, 2021 •

edited

Loading

sathiyapk Nov 15, 2021 •

edited

Loading

sathiyapk Nov 16, 2021 •

edited

Loading

sarutak commented Nov 25, 2021 •

edited

Loading

sathiyapk commented Nov 25, 2021 •

edited

Loading