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 all commits
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
36 changes: 30 additions & 6 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,37 @@ 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.
* modifying `value` in-place and returning it if successful. If an overflow occurs, it
* either returns null or throws an exception according to the value set for
* `spark.sql.decimalOperations.nullOnOverflow`.
*
* NOTE: this modifies `value` in-place, so don't call it on external data.
*/
private[this] def changePrecision(value: Decimal, decimalType: DecimalType): Decimal = {
if (value.changePrecision(decimalType.precision, decimalType.scale)) value else null
if (value.changePrecision(decimalType.precision, decimalType.scale)) {
value
} else {
Copy link
Member

@HyukjinKwon HyukjinKwon Jul 25, 2019

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

Copy link
Member

@gengliangwang gengliangwang Jul 25, 2019

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.

Copy link
Contributor Author

@mgaido91 mgaido91 Jul 26, 2019

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.

Copy link
Member

@HyukjinKwon HyukjinKwon Aug 8, 2019

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

@gengliangwang gengliangwang Jul 25, 2019

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?

Copy link
Contributor Author

@mgaido91 mgaido91 Jul 26, 2019

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?

Copy link
Member

@gengliangwang gengliangwang Jul 30, 2019

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.
* Create new `Decimal` with precision and scale given in `decimalType` (if any).
* If overflow occurs, if `spark.sql.decimalOperations.nullOnOverflow` is true, null is returned;
* otherwise, an `ArithmeticException` is thrown.
*/
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 @@ -963,11 +979,19 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
|$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}).");
""".stripMargin
}
code"""
|if ($d.changePrecision(${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(Timestamp.valueOf("2019-07-25 22:04:36")), DecimalType(3, 2)), null)
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(Timestamp.valueOf("2019-07-25 22:04:36")), DecimalType(3, 2)),
"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")
}
}
}