[SPARK-28470][SQL] Cast to decimal throws ArithmeticException on overflow by mgaido91 · Pull Request #25253 · apache/spark

mgaido91 · 2019-07-25T07:02:28Z

What changes were proposed in this pull request?

The flag spark.sql.decimalOperations.nullOnOverflow is not honored by the Cast operator. This means that a casting which causes an overflow currently returns null.

The PR makes Cast respecting that flag, ie. when it is turned to false and a decimal overflow occurs, an exception id thrown.

How was this patch tested?

Added UT

…flow

SparkQA · 2019-07-25T11:26:37Z

Test build #108155 has finished for PR 25253 at commit 2d85089.

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

HyukjinKwon · 2019-07-25T12:11:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

-    if (value.changePrecision(decimalType.precision, decimalType.scale)) value else null
+    if (value.changePrecision(decimalType.precision, decimalType.scale)) {
+      value
+    } else {


not a biggie but:

if (value.changePrecision(decimalType.precision, decimalType.scale)) { value } else if (nullOnOverflow) { null } else { throw new ArithmeticException(s"${value.toDebugString} cannot be represented as " + s"Decimal(${decimalType.precision}, ${decimalType.scale}).") }

I like the way in this PR. It is more clear about what to do on overflow.

I agree with @gengliangwang but I am fine changing it. Please @HyukjinKwon let me know if you think we should change it, I'll do it. Thanks.

ah, that's fine. no big deal.

gengliangwang · 2019-07-25T18:39:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

+        null
+      } else {
+        throw new ArithmeticException(s"${value.toDebugString} cannot be represented as " +
+          s"Decimal(${decimalType.precision}, ${decimalType.scale}).")


Nit: should we just use ${decimalType.catalogString} here?

this is consistent with other similar error messages. We should change it in all cases, then. WDYT?

This is trivial. Maybe we can have another PR to fix it.

mgaido91 · 2019-08-03T07:51:36Z

cc @cloud-fan @JoshRosen @maropu

maropu

LGTM except for minor comments.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

maropu · 2019-08-03T12:26:55Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

      code"""
-         |$d.changePrecision(${decimalType.precision}, ${decimalType.scale});
+         |$d.changePrecision(
+         |  ${decimalType.precision}, ${decimalType.scale});


nit: write this in a single line? |$d.changePrecision(${decimalType.precision}, ${decimalType.scale});

maropu · 2019-08-03T12:31:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

+      } else {
+        s"""
+           |throw new ArithmeticException($d.toDebugString() + " cannot be represented as " +
+           | "Decimal(${decimalType.precision}, ${decimalType.scale})");


super nit: you forgot to add a period in the end? better to have the same message between the codegen/interpreter modes: https://github.com/apache/spark/pull/25253/files#diff-258b71121d8d168e4d53cb5b6dc53ffeR518

maropu · 2019-08-03T12:33:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

+      }
      code"""
-         |if ($d.changePrecision(${decimalType.precision}, ${decimalType.scale})) {
+         |if ($d.changePrecision(


ditto: can you write it in a single line?

maropu · 2019-08-03T12:37:13Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+    withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key -> "true") {
+      checkEvaluation(Cast(Literal("134.12"), DecimalType(3, 2)), null)
+      checkEvaluation(
+        Cast(Literal(java.sql.Timestamp.valueOf("2019-07-25 22:04:36")), DecimalType(3, 2)), null)


java.sql.Timestamp->Timestamp?

maropu · 2019-08-03T12:37:21Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

+      checkExceptionInExpression[ArithmeticException](
+        Cast(Literal("134.12"), DecimalType(3, 2)), "cannot be represented")
+      checkExceptionInExpression[ArithmeticException](
+        Cast(Literal(java.sql.Timestamp.valueOf("2019-07-25 22:04:36")), DecimalType(3, 2)),


maropu · 2019-08-03T23:14:45Z

cc: @dongjoon-hyun

SparkQA · 2019-08-04T01:13:12Z

Test build #108611 has finished for PR 25253 at commit 2b3aa89.

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

viirya · 2019-08-04T05:48:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala


  /**
   * Create new `Decimal` with precision and scale given in `decimalType` (if any),
   * returning null if it overflows or creating a new `value` and returning it if successful.


Change this comment too?

SparkQA · 2019-08-04T11:31:56Z

Test build #108621 has finished for PR 25253 at commit 23b8ee1.

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

maropu · 2019-08-06T07:00:39Z

Anyone can check this for sign-off before merging?

cloud-fan

LGTM

gengliangwang

LGTM

maropu · 2019-08-07T23:11:18Z

Thanks! Merged to master.

HyukjinKwon · 2019-08-08T02:43:15Z

late LGTM too.

[SPARK-28470][SQL] Cast to decimal throws ArithemticException on over…

2d85089

…flow

dongjoon-hyun added the SQL label Jul 25, 2019

HyukjinKwon reviewed Jul 25, 2019

View reviewed changes

gengliangwang reviewed Jul 25, 2019

View reviewed changes

maropu requested changes Aug 3, 2019

View reviewed changes

address comments

2b3aa89

maropu approved these changes Aug 3, 2019

View reviewed changes

viirya reviewed Aug 4, 2019

View reviewed changes

address comment

23b8ee1

mgaido91 changed the title ~~[SPARK-28470][SQL] Cast to decimal throws ArithemticException on overflow~~ [SPARK-28470][SQL] Cast to decimal throws ArithmeticException on overflow Aug 4, 2019

cloud-fan approved these changes Aug 7, 2019

View reviewed changes

gengliangwang approved these changes Aug 7, 2019

View reviewed changes

viirya approved these changes Aug 7, 2019

View reviewed changes

maropu closed this Aug 7, 2019

Comments

Conversation

mgaido91 commented Jul 25, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 25, 2019

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 Aug 3, 2019

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

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!

maropu commented Aug 3, 2019

Uh oh!

SparkQA commented Aug 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 4, 2019

Uh oh!

maropu commented Aug 6, 2019

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

maropu commented Aug 7, 2019

Uh oh!

HyukjinKwon commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants