[SPARK-28224][SQL] Check overflow in decimal Sum aggregate #25033

mickjermsurawong-stripe · 2019-07-02T16:16:31Z

What changes were proposed in this pull request?

Currently sum in aggregates for decimal type can overflow and return null.

Sum expression codegens arithmetic on sql.Decimal and the output which preserves scale and precision goes into UnsafeRowWriter. Here overflowing will be converted to null when writing out.

It also does not go through this branch in DecimalAggregates because it's expecting precision of the sum (not the elements to be summed) to be less than 5.

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

Lines 1400 to 1403 in 4ebff5b

    
           case ae @ AggregateExpression(af, _, _, _) => af match { 
        
             case Sum(e @ DecimalType.Expression(prec, scale)) if prec + 10 <= MAX_LONG_DIGITS => 
        
               MakeDecimal(ae.copy(aggregateFunction = Sum(UnscaledValue(e))), prec + 10, scale)

This PR adds the check at the final result of the sum operator itself.

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/interfaces.scala

Lines 372 to 376 in 4ebff5b

    
             /** 
        
              * An expression which returns the final value for this aggregate function. Its data type should 
        
              * match this expression's [[dataType]]. 
        
              */ 
        
             val evaluateExpression: Expression

https://issues.apache.org/jira/browse/SPARK-28224

How was this patch tested?

Added an integration test on dataframe suite

cc @mgaido91 @JoshRosen

mgaido91

The change makes sense to me. May you please also check that SQLServer behaves in the same way? Ie. that it throws an exception in this specific case?

mgaido91 · 2019-07-02T18:50:26Z

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

Seq(true, false).foreach {

mgaido91 · 2019-07-02T18:55:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Sum.scala

this should use resultType I think

dataType actually calls resultType but the latter reads better for our purpose here. Will update so.

yes, sorry, I meant something like this:

case _: DecimalType => CheckOverflow(sum, resultType, SQLConf.get.decimalOperationsNullOnOverflow)

In this way we do not create a new DecimalType and I think it is more readable... Thanks.

Updated to using matched case because resultType is still a generic DataType and not Decimal type

override lazy val evaluateExpression: Expression = resultType match { case d: DecimalType => CheckOverflow(sum, d, SQLConf.get.decimalOperationsNullOnOverflow) case _ => sum }

mgaido91 · 2019-07-02T18:58:10Z

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

why do we need this?

This is the convention to how other dataset in this file is created.

well, if others think it is ok to leave it I am fine with it, I'd remove it since it is useless honestly.

I'm fine either way and would also defer to others. Although i do see value in consistency here.

Yea, I personally think we don't need these changes in this pr, too.

mgaido91 · 2019-07-02T19:15:24Z

PS Please add [SQL] to the PR title.

Honestly, I don't consider this a followup to #25010. This was actually a bug leading to weird behaviors, such as show prints an output, while collect returns null. And this is independent from the PR you linked.

dongjoon-hyun · 2019-07-05T01:32:05Z

ok to test

mickjermsurawong-stripe · 2019-07-05T04:25:08Z

@mgaido91, thank you for the review! I made the updates.
To your original question on SQL server behavior, there seem to be different outcome among sql versions. From fiddle here:
Sql5.6 would arbitrarily loose precision even on integral parts with rounding. [0]
Sql MS Sql Server107 would show error Data truncation [1]

Aiming for a least-surprise behavior, do you think sum here should be treated the same as other arithmetic operations on fixed precisions?

A larger question here is perhaps if there are other operations on fixed precision that could result in overflow. And supposed we are okay with this PR fix, does it mean we should patch for all such operators? A non-issue here is pow operator with Double type.

[0] http://sqlfiddle.com/#!9/676c31/1
[1] http://sqlfiddle.com/#!18/0baa5/1

SparkQA · 2019-07-05T04:44:38Z

Test build #107252 has finished for PR 25033 at commit 01174be.

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

mgaido91 · 2019-07-05T05:40:25Z

Thanks for your work. Regarding SQLServer, I also checked its behaviour. The main difference with our current behaviour is that it always returns the max precision, instead of our formula 10 + precision. So I submitted a PR for that.

LGTM

SparkQA · 2019-07-05T07:05:01Z

Test build #107258 has finished for PR 25033 at commit b668d6f.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

mickjermsurawong-stripe · 2019-07-09T22:26:38Z

jenkins retest this please

JoshRosen · 2019-07-09T22:32:27Z

jenkins retest this please

SparkQA · 2019-07-10T01:44:00Z

Test build #107419 has finished for PR 25033 at commit 4094335.

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

mickjermsurawong-stripe · 2019-08-12T17:29:54Z

hi @dongjoon-hyun, @cloud-fan. Just would like to follow-up on this PR please.

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

maropu · 2019-08-15T07:51:53Z

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

+    }
+  }
+
+  test("Aggregate sum integers") {


We need the two tests below for this change?

@maropu the change introduces branching for BigDecimal case, so I want to verify that sum still works for integer/double case.

But, the case for int/double is an existing path in master, so we already have tests for that somewhere, right? I personally think the test above is enough for this pr.

SparkQA · 2019-08-17T20:05:13Z

Test build #109280 has finished for PR 25033 at commit 67a956e.

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

maropu

LGTM except for one minor comment.

SparkQA · 2019-08-19T07:05:02Z

Test build #109312 has finished for PR 25033 at commit 4497576.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-08-19T08:11:24Z

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

    df
  }

+  protected lazy val largeDecimals: DataFrame = {


SQLTestData should only contain data that are widely used by many tests. Let's move the largeDecimals to its test case.

SparkQA · 2019-08-19T16:13:27Z

Test build #109353 has finished for PR 25033 at commit 49c97d8.

This patch fails to build.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2019-08-19T20:48:17Z

Test build #109356 has finished for PR 25033 at commit db2d9a5.

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

maropu · 2019-08-20T00:47:33Z

Thanks! Merged to master.

mgaido91 reviewed Jul 2, 2019

View reviewed changes

mickjermsurawong-stripe changed the title ~~[SPARK-28224] Aggregate sum large decimals~~ [SPARK-28224][SQL] Aggregate sum large decimals Jul 2, 2019

dongjoon-hyun added the SQL label Jul 3, 2019

mickjermsurawong-stripe force-pushed the SPARK-28224 branch from b668d6f to 4094335 Compare July 9, 2019 22:32

JoshRosen changed the title ~~[SPARK-28224][SQL] Aggregate sum large decimals~~ [SPARK-28224][SQL] Check overflow in decimal Sum aggregate Jul 9, 2019

maropu reviewed Aug 15, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala Outdated Show resolved Hide resolved

maropu reviewed Aug 15, 2019

View reviewed changes

maropu approved these changes Aug 17, 2019

View reviewed changes

cloud-fan reviewed Aug 19, 2019

View reviewed changes

cloud-fan approved these changes Aug 19, 2019

View reviewed changes

mickjermsurawong-stripe added 7 commits August 19, 2019 09:39

aggregation not handled

4845bb7

register test data

bb9a477

test other datatypes int/double are not affected

023639f

simplify test

549257f

use previate more explicit variable

893881a

use matched result type

04a3164

more specific exception

d2f7843

mickjermsurawong-stripe added 3 commits August 19, 2019 09:39

remove unnecessary create temp view

8110ad1

remove unnecessary test

873c702

move fixture to test itself

db2d9a5

mickjermsurawong-stripe force-pushed the SPARK-28224 branch from 49c97d8 to db2d9a5 Compare August 19, 2019 16:54

maropu closed this in b79cf0d Aug 20, 2019

	case ae @ AggregateExpression(af, _, _, _) => af match {
	case Sum(e @ DecimalType.Expression(prec, scale)) if prec + 10 <= MAX_LONG_DIGITS =>
	MakeDecimal(ae.copy(aggregateFunction = Sum(UnscaledValue(e))), prec + 10, scale)

	/**
	* An expression which returns the final value for this aggregate function. Its data type should
	* match this expression's [[dataType]].
	*/
	val evaluateExpression: Expression

[SPARK-28224][SQL] Check overflow in decimal Sum aggregate #25033

[SPARK-28224][SQL] Check overflow in decimal Sum aggregate #25033

Uh oh!

Conversation

mickjermsurawong-stripe commented Jul 2, 2019 • edited by JoshRosen 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!

mgaido91 left a comment

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!

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!

mgaido91 commented Jul 2, 2019

Uh oh!

dongjoon-hyun commented Jul 5, 2019

Uh oh!

mickjermsurawong-stripe commented Jul 5, 2019

Uh oh!

SparkQA commented Jul 5, 2019

Uh oh!

mgaido91 commented Jul 5, 2019

Uh oh!

SparkQA commented Jul 5, 2019

Uh oh!

mickjermsurawong-stripe commented Jul 9, 2019

Uh oh!

JoshRosen commented Jul 9, 2019

Uh oh!

SparkQA commented Jul 10, 2019

Uh oh!

mickjermsurawong-stripe commented Aug 12, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu Aug 17, 2019 • 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!

SparkQA commented Aug 17, 2019

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 19, 2019

Uh oh!

SparkQA commented Aug 19, 2019

Uh oh!

maropu commented Aug 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

mickjermsurawong-stripe commented Jul 2, 2019 •

edited by JoshRosen

Loading

maropu Aug 17, 2019 •

edited

Loading