viirya commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4252646145
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ 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)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns
null if no decimal
+ * type can hold it. We fast fail because constructing a very large
JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal =
{
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
Review Comment:
Good catch, that was a regression from the previous commit. Done with your
suggested structure; an in-range string no longer reads the conf.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ 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)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns
null if no decimal
+ * type can hold it. We fast fail because constructing a very large
JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal =
{
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
+ } else if (bigDecimal.signum == 0) {
Review Comment:
Added the zero case and this `Literal` example to SPARK-60132.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ 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)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns
null if no decimal
+ * type can hold it. We fast fail because constructing a very large
JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal =
{
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
+ } else 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 the exponent.
Review Comment:
Narrowed the comment to positive exponents. I'd rather not cap the scale
only for zero, since a nonzero value like `'1e-50'` gets precision 50 the same
way and `changePrecision` handles both.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,29 @@ 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:
Done, added the legacy block and the `fromStringANSI` check for `'0.00'`.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ 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)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns
null if no decimal
+ * type can hold it. We fast fail because constructing a very large
JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal =
{
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
Review Comment:
Done as part of the restructuring above.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ 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)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns
null if no decimal
Review Comment:
Done.
--
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]
