[SPARK-28201][SQL] Revisit MakeDecimal behavior on overflow #25010
Conversation
|
Test build #107036 has finished for PR 25010 at commit
|
There was a problem hiding this comment.
Thanks for submitting a patch so quickly! The changes here look pretty straightforward.
I left two review questions, mostly asking about whether certain optimizations are necessary (or whether they can be skipped to improve code readability).
I'd also appreciate a second review from someone else who's also familiar with Decimals and codegen. (/cc @cloud-fan, who reviewed #20350)
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
Show resolved
Hide resolved
|
Test build #107042 has finished for PR 25010 at commit
|
|
Test build #107044 has finished for PR 25010 at commit
|
|
thanks, merging to master! |
|
hi @mgaido91, I made one follow-up here. Seems like aggregation path still can have null output even with throwing exception flag on. https://issues.apache.org/jira/browse/SPARK-28224 |
| s"Decimal precision ${decimalVal.precision} exceeds max precision $precision") | ||
| if (decimalVal.precision > precision) { | ||
| throw new ArithmeticException( | ||
| s"Decimal precision ${decimalVal.precision} exceeds max precision $precision") |
There was a problem hiding this comment.
Hi, All.
This seems to break JDBC Integration Test suite. I'll make a followup PR.
There was a problem hiding this comment.
Thanks @dongjoon-hyun , sorry for the trouble. I hadn't run integration tests indeed.
…ng to the new exception message ## What changes were proposed in this pull request? apache#25010 breaks the integration test suite due to the changing the user-facing exception like the following. This PR fixes the integration test suite. ```scala - require( - decimalVal.precision <= precision, - s"Decimal precision ${decimalVal.precision} exceeds max precision $precision") + if (decimalVal.precision > precision) { + throw new ArithmeticException( + s"Decimal precision ${decimalVal.precision} exceeds max precision $precision") + } ``` ## How was this patch tested? Manual test. ``` $ build/mvn install -DskipTests $ build/mvn -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.12 test ``` Closes apache#25165 from dongjoon-hyun/SPARK-28201. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
What changes were proposed in this pull request?
In SPARK-23179, it has been introduced a flag to control the behavior in case of overflow on decimals. The behavior is: returning
nullwhenspark.sql.decimalOperations.nullOnOverflow(default and traditional Spark behavior); throwing anArithmeticExceptionif that conf is false (according to SQL standards, other DBs behavior).MakeDecimalso far had an ambiguous behavior. In case of codegen mode, it returnednullas the other operators, but in interpreted mode, it was throwing anIllegalArgumentException.The PR aligns
MakeDecimal's behavior with the one of other operators as defined in SPARK-23179. So now both modes returnnullor throwArithmeticExceptionaccording tospark.sql.decimalOperations.nullOnOverflow's value.Credits for this PR to @mickjermsurawong-stripe who pointed out the wrong behavior in #20350.
How was this patch tested?
improved UTs