viirya commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4262662817
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Int =
- bigDecimal.precision - bigDecimal.scale
+ // Computed in Long since `precision - scale` can overflow Int, e.g. for
"1e2147483647".
+ private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Long =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than
`s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to
Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns
null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales
are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is
very slow. For
+ * example: Decimal("6.0790316E+25569151")
+ *
+ * This runs for every row of a cast, so the conf is read only for a zero
with a negative scale
Review Comment:
Thanks. Scoped the sentence to this helper's own checks, as you suggested.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Int =
- bigDecimal.precision - bigDecimal.scale
+ // Computed in Long since `precision - scale` can overflow Int, e.g. for
"1e2147483647".
+ private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Long =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than
`s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to
Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns
null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales
are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is
very slow. For
+ * example: Decimal("6.0790316E+25569151")
+ *
+ * This runs for every row of a cast, so the conf is read only for a zero
with a negative scale
+ * and for a value with too many integral digits.
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal =
{
+ if (bigDecimal.signum == 0) {
+ // Zero fits any decimal type whatever its exponent, e.g. "0e39". Drop a
negative scale so
+ // that the precision does not grow with a positive exponent.
+ if (bigDecimal.scale < 0 &&
!SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal.setScale(0))
} else {
Decimal(bigDecimal)
}
+ } else if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION
&&
+ !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
Review Comment:
Fixed, thanks. No worries about the snippet; I should have run scalafmt on
sql/api myself.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Int =
- bigDecimal.precision - bigDecimal.scale
+ // Computed in Long since `precision - scale` can overflow Int, e.g. for
"1e2147483647".
+ private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Long =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than
`s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to
Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns
null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales
are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is
very slow. For
+ * example: Decimal("6.0790316E+25569151")
Review Comment:
Fixed with scalafmt's wrapping.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,42 @@ class DecimalSuite extends SparkFunSuite with
PrivateMethodTester with SQLHelper
}
}
+ test("SPARK-60118: UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values
overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647",
+ "1.5e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === null)
+ checkError(
+ exception = intercept[SparkArithmeticException](
+ Decimal.fromStringANSI(UTF8String.fromString(string))),
+ condition = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+ parameters = Map("value" -> string))
+ }
+ // Zero fits any decimal type whatever its exponent.
+ Seq("0e39", "-0E+100", "0e2147483647").foreach { string =>
Review Comment:
Good point. Added `'0e37'` and `'0e38'` here (both now assert precision 1
and scale 0) and `'0e38'` to the `CastSuiteBase` list.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
