mgaido91 commented on a change in pull request #25035: [SPARK-28235][SQL] Sum
of decimals should return a decimal with MAX_PRECISION
URL: https://github.com/apache/spark/pull/25035#discussion_r300271855
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/DecimalPrecisionSuite.scala
##########
@@ -86,7 +86,7 @@ class DecimalPrecisionSuite extends AnalysisTest with
BeforeAndAfter {
checkType(Divide(d2, d1), DecimalType(10, 6))
checkType(Remainder(d1, d2), DecimalType(3, 2))
checkType(Remainder(d2, d1), DecimalType(3, 2))
- checkType(Sum(d1), DecimalType(12, 1))
+ checkType(Sum(d1), DecimalType(DecimalType.MAX_PRECISION, 1))
Review comment:
> First, this PR doesn't provide the evidence of the claim, overflows. We
need to add that specific test case.
It is not easy to provide that case @dongjoon-hyun as a UT. you need a
`DataFrame` with more than bilion of rows. If you have a column which is a
`DECIMAL(2,1)`, for instance, the result now is a `DECIMAL(12,1)`. To overflow
it with a sum, you need ~ 11.000.000.000 `9,9`. So it is not feasible to add it
as a UT.
> Second, you may want to change DecimalAggregates optimizer together to
pass the UTs.
About this point, I was asking your opinion, because I can fix it in order
to have it passing UTs, but I am not sure about the semantic of the fix,
because after the change the rule would not be a valid optimizer rule anymore,
as in certain cases like the one mentioned above, this rule can cause the
overflow to happen again (while without that rule the correct value is
returned). Anyway, I'll go ahead and fix it, but I'd love to get your opinion
on this topic. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
