[SPARK-28610][SQL] Allow having a decimal buffer for long sum by mgaido91 · Pull Request #25347 · apache/spark

mgaido91 · 2019-08-03T09:36:33Z

What changes were proposed in this pull request?

If spark.sql.arithmeticOperations.failOnOverFlow is true temporary overflows in sum of long values cause an exception. This is against what other RDBMS do. The root cause is that we use a long attribute to store the intermediate result of a sum of longs: so if there is an intermediate value which is our of the range representable by long, an exception is raised.

The PR introduces a flag which allows to control the datatype of the intermediate attribute of sum of long. When the flag is set to true, intermediate buffer is a decimal, so temporary overflows don't cause any issue. The flag has been introduced on @cloud-fan 's suggestion because as he pointed out, using always a decimal as intermediate buffer would cause a performance regression.

How was this patch tested?

Added UT

SparkQA · 2019-08-03T23:30:09Z

Test build #108605 has finished for PR 25347 at commit af78b62.

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

xuanyuanking

+1 for me, cc @cloud-fan for a double-check.
How about link this comment from you and Wenchen to the PR description?

mgaido91 · 2019-08-04T08:04:45Z

thanks for the suggestion @xuanyuanking , I added the link!

SparkQA · 2019-08-04T11:30:32Z

Test build #108620 has finished for PR 25347 at commit e81e8fa.

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

kiszk · 2019-08-04T14:34:31Z

Do we need similar handing for average?

mgaido91 · 2019-08-04T14:56:59Z

@kiszk no, since the buffer value for average is a double for longs, so there is not the same issue.

kiszk · 2019-08-04T15:20:37Z

@mgaido91 I see. It looks truncation during double sum for average is expected, as described here. #21599 throws overflow only for integral arithmetic operations.

HyukjinKwon

Looks good by me too.

maropu · 2019-08-05T22:53:29Z

Is this common handling for long sum in DBMSs? In postgresql and mysql, it seems that the output type is decimal and the answer is correct;

// PostgreSQL
postgres=# \d t
Table "public.t"
Column | Type | Collation | Nullable | Default --------+--------+-----------+----------+---------
l | bigint | | | postgres=# select * from t;
l ---------------------
9223372036854775807
1
(2 rows)
postgres=# select sum(l) from t;
sum ---------------------
9223372036854775808
(1 row)
postgres=# create temporary table v as select sum(l) from t;
postgres=# \d v
Table "pg_temp_3.v"
Column | Type | Collation | Nullable | Default --------+---------+-----------+----------+---------
sum | numeric | | | // MySQL
mysql> explain t;
+-------+------------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+-------+------------+------+-----+---------+-------+
| l | bigint(20) | YES | | NULL | |
+-------+------------+------+-----+---------+-------+
1 row in set (0.00 sec)
mysql> select * from t;
+---------------------+
| l |
+---------------------+
| 9223372036854775807 |
| 1 |
+---------------------+
2 rows in set (0.00 sec)
mysql> select sum(l) from t;
+---------------------+
| sum(l) |
+---------------------+
| 9223372036854775808 |
+---------------------+
1 row in set (0.00 sec)
mysql> create temporary table v as select sum(l) from t;
mysql> explain v;
+--------+---------------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+--------+---------------+------+-----+---------+-------+
| sum(l) | decimal(41,0) | YES | | NULL | NULL |
+--------+---------------+------+-----+---------+-------+
1 row in set (0.01 sec)

mgaido91 · 2019-08-06T07:44:48Z

Yes @maropu, you're right. The reason why I didn't change the output attribute was not to cause a breaking change. But since we are introducing a flag for it, it may be ok to do so. What do you think? cc @cloud-fan what do you think about this?

maropu · 2019-08-07T00:13:03Z

Yea, I think so. If we have a new flag, IMO it'd be better that spark has the same behivour with the others.

SparkQA · 2019-08-08T11:41:39Z

Test build #108807 has finished for PR 25347 at commit 5738511.

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

cloud-fan · 2019-08-19T09:07:59Z

@maropu good point. I think it's more than overflow now. Maybe we should create a config to specify the dialect, and provide 2 options: legacy and ansi. For ansi dialect, we try out best to make the behavior of Spark be consistent with ansi sql standard, including parser, overflow, sum data type, etc.

mgaido91 · 2019-08-19T12:35:43Z

thanks for your comments @cloud-fan and @maropu.

I agree long term to have coarser flags controlling all these things, like: ansi strict, legacy and so on. So far we are creating a new flag for each case as it is done now in this PR.

I am not sure if your comments say that we should hold this until we create such flags or we can go ahead on this as it is and create those flags later. May you please clarify?

Thanks.

cloud-fan · 2019-08-19T13:15:37Z

We can create the coarser flag later.

For this PR, in general LGTM, but let's make the story about sum completed. Is it true that, in the mainstream databases, sum of long returns decimal and sum of other integral types return long? Or do they always return decimal for sum?

mgaido91 · 2019-08-20T16:26:50Z

Well, actually mainstream DBs behave differently among each other:

SQLServer returns bigint for a sum of bigint, int for a sum of int, decimal(28,s) for a sum of decimal(p,s);
MySQL returns decimal(38,0) for a sum of int, decimal(41, 0) for a sum of long;
Postgres behaves like after this PR with the flag on, ie. a sum of integers returns a long, while a sum of longs returns a decimal.

maropu · 2019-08-20T23:59:45Z

SQLServer throws an exception when results overflowed?
@wangyum Do you know a Oracle behaviour for that?

wangyum · 2019-08-21T05:19:02Z

PostgreSQL:

postgres=# create table t1(c1 bigint);
CREATE TABLE
postgres=# insert into t1 values(9223372036854775807), (9223372036854775807);
INSERT 02
postgres=# select sum(c1) from t1;
sum
----------------------18446744073709551614
(1 row)

db2:

[db2inst1@2f3c821d36b7 ~]$ db2 "create table t1(c1 bigint)"
DB20000I The SQL command completed successfully.
[db2inst1@2f3c821d36b7 ~]$ db2 "insert into t1 values(9223372036854775807), (9223372036854775807)"
DB20000I The SQL command completed successfully.
[db2inst1@2f3c821d36b7 ~]$ db2 "select sum(c1) from t1"1--------------------
SQL0802N Arithmetic overflow or other arithmetic exception occurred.
SQLSTATE=22003

SQL Server:2019-CTP3.0

1> create table t1(c1 bigint)
2> go
1>insert into t1 values(9223372036854775807), (9223372036854775807)
2> go
(2 rows affected)
1>1>selectsum(c1) from t1
2> go
Msg 8115, Level 16, State 2, Server 8c82b3c03354, Line1
Arithmetic overflow error converting expression to data type bigint.

Vertica:

dbadmin=> create table t1(c1 bigint);
CREATE TABLE
dbadmin=>insert into t1 values(9223372036854775807);
OUTPUT
--------1
(1 row)
dbadmin=>insert into t1 values(9223372036854775807);
OUTPUT
--------1
(1 row)
dbadmin=>selectsum(c1) from t1;
ERROR 4845: Sum() overflowed
HINT: Try sum_float() instead
dbadmin=>select sum_float(c1) from t1;
sum_float
----------------------1.84467440737096e+19
(1 row)

Oracle:

SQL>-- BIGINT -> NUMBER(19) : https://docs.oracle.com/cd/B19306_01/gateways.102/b14270/apa.htm
SQL> create table t1(c1 NUMBER(19));
Table created.
SQL>insert into t1 values(9223372036854775807);
1 row created.
SQL>insert into t1 values(9223372036854775807);
1 row created.
SQL>selectsum(c1) from t1;
SUM(C1)
----------1.8447E+19
SQL> create table t2 asselectsum(c1) as s from t1;
Table created.
SQL>desc t2;
Name Null? Type
----------------------------------------- -------- ----------------------------
S NUMBER

maropu · 2019-08-21T05:36:27Z

Great thanks for the exhaustive survey!! I'm a bit suprised that they have totally different default behaivours.... Oracle truncates the result?

SQL> select sum(c1) from t1;
SUM(C1)
----------
1.8447E+19

mgaido91 · 2019-08-21T09:45:16Z

thanks @wangyum !

Actually I tried your very same command and Oracle returned the exact result to me. Anyway, I don't think it is relevant here. Oracle uses always DECIMAL, it doesn't really have a LONG datatype...

cloud-fan · 2019-08-21T12:02:31Z

If different databases have different behaviors, then I don't see a strong reason to change the return type of sum in Spark. How about we only use decimal as sum buffer?

mgaido91 · 2019-08-21T12:42:40Z

I think the reason is to have feature parity with Postgres. Using it as return type too supports also cases which would overflow otherwise. I think, since we are guarding it with a flag, it is fine to return a decimal and have wider support.

cloud-fan · 2019-08-21T13:49:20Z

We need to be careful when adding configs to control query semantic. We have a lot of such configs now, but most of them are legacy configs. Legacy configs are OK, since they are not mainly for controlling query semantic, but to provide a way to restore the legacy behavior.

That said, if there is a strong reason to change the return type of sum from long to decimal, let's go for it and add a legacy config. But seems it's not the case here. Spark needs to follow SQL standard but not Postgres, so making Spark behave the same with Postgres is not a strong reason to me.

If the goal is to leverage the ported Postgres tests, I think it's reasonable. But the config should be internal and clearly states that it's only used in the ported Postgres tests.

mgaido91 · 2019-08-21T15:26:41Z

Spark needs to follow SQL standard but not Postgres, so making Spark behave the same with Postgres is not a strong reason to me.

I 100% agree on that.

@maropu what do you think?

maropu · 2019-08-22T00:11:54Z

Spark needs to follow SQL standard but not Postgres, so making Spark behave the same with Postgres is not a strong reason to me.
I 100% agree on that.

Yea, the opinion looks reasonable to me, too.

How about we only use decimal as sum buffer?

But, I feel a bit uncomfortable to add a new flag only for this sum behaviour change and I personally think most users might not notice this option. The change to support decimal sum buffer itself looks nice to me, but I think we'd like a coarser flag for that.

mgaido91 · 2019-08-22T08:16:20Z

The change to support decimal sum buffer itself looks nice to me, but I think we'd like a coarser flag for that.

@maropu @cloud-fan any suggestion on what this flag should be/look like?

Meanwhile I'll update the PR to limit the scope only to the buffer, thanks!

This reverts commit 5738511.

SparkQA · 2019-08-22T10:47:05Z

Test build #109565 has finished for PR 25347 at commit aae4642.

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

SparkQA · 2019-08-22T14:45:33Z

Test build #109573 has finished for PR 25347 at commit 7606b5c.

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

SparkQA · 2019-08-22T19:11:35Z

Test build #109585 has finished for PR 25347 at commit bd27f58.

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

cloud-fan · 2019-08-26T14:11:50Z

@mgaido91 can you do a simple microbenchmark? If the performance overhead is not significant, we can use decimal as sum buffer and provide a legacy config to use long as buffer.

mgaido91 · 2019-08-26T16:54:54Z

I'll do that @cloud-fan , thanks

mgaido91 · 2019-08-27T13:59:24Z

@cloud-fan unfortunately the performance overhead is very significant. I tried and run the benchmarks in both modes.

Here you can see the code:

runBenchmark("aggregate without grouping") {
val N = 500L << 22
withSQLConf(SQLConf.SUM_DECIMAL_BUFFER_FOR_LONG.key -> "false") {
codegenBenchmark("agg w/o group long buffer", N) {
spark.range(N).selectExpr("sum(id)").collect()
}
}
withSQLConf(SQLConf.SUM_DECIMAL_BUFFER_FOR_LONG.key -> "true") {
codegenBenchmark("agg w/o group decimal buffer", N) {
spark.range(N).selectExpr("sum(id)").collect()
}
}
}

and here it is the output (as you can see, the overhead is more than 10x on a simple sum of longs):

[info] Running benchmark: agg w/o group long buffer
[info] Running case: agg w/o group long buffer wholestage off
[info] Stopped after 2 iterations, 105407 ms
[info] Running case: agg w/o group long buffer wholestage on
[info] Stopped after 5 iterations, 6282 ms
[info] [info] Java HotSpot(TM) 64-Bit Server VM 1.8.0_152-b16 on Mac OS X 10.13.6
[info] Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
[info] agg w/o group long buffer: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] agg w/o group long buffer wholestage off 48538 52704 NaN 43,2 23,1 1,0X
[info] agg w/o group long buffer wholestage on 1231 1257 28 1703,6 0,6 39,4X
[info] [info] Running benchmark: agg w/o group decimal buffer
[info] Running case: agg w/o group decimal buffer wholestage off
[info] Stopped after 2 iterations, 1276890 ms
[info] Running case: agg w/o group decimal buffer wholestage on
[info] Stopped after 5 iterations, 496100 ms
[info] [info] Java HotSpot(TM) 64-Bit Server VM 1.8.0_152-b16 on Mac OS X 10.13.6
[info] Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
[info] agg w/o group decimal buffer: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] agg w/o group decimal buffer wholestage off 633585 638445 NaN 3,3 302,1 1,0X
[info] agg w/o group decimal buffer wholestage on 92037 99220 NaN 22,8 43,9 6,9X

cloud-fan · 2019-08-28T14:11:13Z

Now I'm wondering if it's worthwhile to sacrifice 10x perf to handle intermedia overflow. It's not about correctness but usability: without this change, queries are more likely to fail with overflow when summing up long values.

So it's a tradeoff between usability and performance. Given it's 10x slowdown, I don't think any users would turn it on by default. If they hit overflow exception later, I think they will just cast the data to decimal before sum. That said, I think it's not very useful to have this feature.

Anyway, many thanks to @mgaido91 for investigating and providing the perf numbers!

mgaido91 · 2019-08-28T14:23:16Z

yes, I agree the perf issue is very significant. Let me close this one, thank you all for the reviews!

[SPARK-28610][SQL] Allow having a decimal buffer for long sum
af78b62

xuanyuanking approved these changes Aug 4, 2019
View reviewed changes

fix scalastyle
e81e8fa

dongjoon-hyun added the SQL label Aug 4, 2019

HyukjinKwon approved these changes Aug 5, 2019
View reviewed changes

return decimal
5738511

Revert "return decimal"
aae4642
This reverts commit 5738511.

Merge branch 'master' of github.com:apache/spark into SPARK-28610
7606b5c

fix
bd27f58

mgaido91 closed this Aug 28, 2019

mgaido91 mentioned this pull request Feb 19, 2020
[SPARK-28067][SQL]Fix incorrect results during aggregate sum for decimal overflow by throwing exception #27629
Closed

Uh oh!

Conversation

mgaido91 commented Aug 3, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 3, 2019

Uh oh!

xuanyuanking left a comment • 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!

mgaido91 commented Aug 4, 2019

Uh oh!

SparkQA commented Aug 4, 2019

Uh oh!

kiszk commented Aug 4, 2019

Uh oh!

mgaido91 commented Aug 4, 2019

Uh oh!

kiszk commented Aug 4, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

maropu commented Aug 5, 2019

Uh oh!

mgaido91 commented Aug 6, 2019

Uh oh!

maropu commented Aug 7, 2019

Uh oh!

SparkQA commented Aug 8, 2019

Uh oh!

cloud-fan commented Aug 19, 2019

Uh oh!

mgaido91 commented Aug 19, 2019

Uh oh!

cloud-fan commented Aug 19, 2019

Uh oh!

mgaido91 commented Aug 20, 2019

Uh oh!

maropu commented Aug 20, 2019

Uh oh!

wangyum commented Aug 21, 2019

Uh oh!

maropu commented Aug 21, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgaido91 commented Aug 21, 2019

Uh oh!

cloud-fan commented Aug 21, 2019

Uh oh!

mgaido91 commented Aug 21, 2019

Uh oh!

cloud-fan commented Aug 21, 2019

Uh oh!

mgaido91 commented Aug 21, 2019

Uh oh!

maropu commented Aug 22, 2019

Uh oh!

mgaido91 commented Aug 22, 2019

Uh oh!

SparkQA commented Aug 22, 2019

Uh oh!

SparkQA commented Aug 22, 2019

Uh oh!

SparkQA commented Aug 22, 2019

Uh oh!

cloud-fan commented Aug 26, 2019

Uh oh!

mgaido91 commented Aug 26, 2019

Uh oh!

mgaido91 commented Aug 27, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Aug 28, 2019

Uh oh!

mgaido91 commented Aug 28, 2019

mgaido91 commented Aug 3, 2019•
edited
Loading

xuanyuanking left a comment •
edited
Loading

kiszk commented Aug 4, 2019•
edited
Loading

maropu commented Aug 21, 2019•
edited
Loading

mgaido91 commented Aug 27, 2019•
edited
Loading