Skip to content

[SPARK-28470][SQL] Cast to decimal throws ArithmeticException on overflow #25253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mgaido91 wants to merge 3 commits into from
Closed

[SPARK-28470][SQL] Cast to decimal throws ArithmeticException on overflow #25253

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d85089
[SPARK-28470][SQL] Cast to decimal throws ArithemticException on over…
mgaido91 Jul 25, 2019
2b3aa89
address comments
mgaido91 Aug 3, 2019
23b8ee1
address comment
mgaido91 Aug 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import org.apache.spark.sql.catalyst.expressions.codegen._
import org.apache.spark.sql.catalyst.expressions.codegen.Block._
import org.apache.spark.sql.catalyst.util._
import org.apache.spark.sql.catalyst.util.DateTimeUtils._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._
import org.apache.spark.unsafe.UTF8StringBuilder
import org.apache.spark.unsafe.types.{CalendarInterval, UTF8String}
Expand Down Expand Up @@ -498,22 +499,34 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
b => x.numeric.asInstanceOf[Numeric[Any]].toInt(b).toByte
}

private val nullOnOverflow = SQLConf.get.decimalOperationsNullOnOverflow

/**
* Change the precision / scale in a given decimal to those set in `decimalType` (if any),
* returning null if it overflows or modifying `value` in-place and returning it if successful.
*
* NOTE: this modifies `value` in-place, so don't call it on external data.
*/
private[this] def changePrecision(value: Decimal, decimalType: DecimalType): Decimal = {
Comment thread
mgaido91 marked this conversation as resolved.
if (value.changePrecision(decimalType.precision, decimalType.scale)) value else null
if (value.changePrecision(decimalType.precision, decimalType.scale)) {
value
} else {

@HyukjinKwon HyukjinKwon Jul 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}).")
}

@gengliangwang gengliangwang Jul 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mgaido91 mgaido91 Jul 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HyukjinKwon HyukjinKwon Aug 8, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that's fine. no big deal.

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

@gengliangwang gengliangwang Jul 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mgaido91 mgaido91 Jul 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@gengliangwang gengliangwang Jul 30, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}

/**
* 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.

@viirya viirya Aug 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this comment too?

*/
private[this] def toPrecision(value: Decimal, decimalType: DecimalType): Decimal =
value.toPrecision(decimalType.precision, decimalType.scale)
value.toPrecision(
decimalType.precision, decimalType.scale, Decimal.ROUND_HALF_UP, nullOnOverflow)


private[this] def castToDecimal(from: DataType, target: DecimalType): Any => Any = from match {
Expand Down Expand Up @@ -959,15 +972,25 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
evPrim: ExprValue, evNull: ExprValue, canNullSafeCast: Boolean): Block = {
if (canNullSafeCast) {
code"""
|$d.changePrecision(${decimalType.precision}, ${decimalType.scale});
|$d.changePrecision(
| ${decimalType.precision}, ${decimalType.scale});

@maropu maropu Aug 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

|$evPrim = $d;
""".stripMargin
} else {
val overflowCode = if (nullOnOverflow) {
s"$evNull = true;"
} else {
s"""
|throw new ArithmeticException($d.toDebugString() + " cannot be represented as " +
| "Decimal(${decimalType.precision}, ${decimalType.scale})");

@maropu maropu Aug 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@maropu maropu Aug 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: can you write it in a single line?

| ${decimalType.precision}, ${decimalType.scale})) {
| $evPrim = $d;
|} else {
| $evNull = true;
| $overflowCode
|}
""".stripMargin
}
Expand Down
22 changes: 22 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import org.apache.spark.sql.catalyst.expressions.codegen.CodegenContext
import org.apache.spark.sql.catalyst.util.DateTimeTestUtils._
import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.catalyst.util.DateTimeUtils._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._
import org.apache.spark.unsafe.types.UTF8String

Expand Down Expand Up @@ -1018,4 +1019,25 @@ class CastSuite extends SparkFunSuite with ExpressionEvalHelper {
checkEvaluation(ret, InternalRow(null))
}
}

test("SPARK-28470: Cast should honor nullOnOverflow property") {
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)

@maropu maropu Aug 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java.sql.Timestamp->Timestamp?

checkEvaluation(Cast(Literal(BigDecimal(134.12)), DecimalType(3, 2)), null)
checkEvaluation(Cast(Literal(134.12), DecimalType(3, 2)), null)
}
withSQLConf(SQLConf.DECIMAL_OPERATIONS_NULL_ON_OVERFLOW.key -> "false") {
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 maropu Aug 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

"cannot be represented")
checkExceptionInExpression[ArithmeticException](
Cast(Literal(BigDecimal(134.12)), DecimalType(3, 2)), "cannot be represented")
checkExceptionInExpression[ArithmeticException](
Cast(Literal(134.12), DecimalType(3, 2)), "cannot be represented")
}
}
}