dongjoon-hyun commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4256056610
##########
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:
L649 is 99 characters, over the `maxColumn = 98` in `dev/.scalafmt.conf`
that applies to `sql/api`. It is the other spot behind the scalafmt failure.
Scalastyle allows 100 characters, so it doesn't catch this. This is how
scalafmt wraps the paragraph:
```suggestion
* 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")
```
##########
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:
This line fails the scalafmt check in CI. At e7dd3a4, the `Run / Linters,
licenses, and dependencies` job fails in the `Scala linter` step with
`Scalafmt: Unformatted files found` on `spark-sql-api`. The previous commit
passed this job. For `sql/api`, scalafmt wants a 2-space continuation indent
here, as in the original code on master, not 4. Sorry, the 4-space indent came
from the snippet I suggested in my previous review.
I ran the formatter on this commit, and it changed only this line and the
Javadoc at L649-652 (see the other comment):
```
./build/mvn scalafmt:format -Dscalafmt.skip=false
-Dscalafmt.validateOnly=false -Dscalafmt.changedOnly=false -pl sql/api
```
```suggestion
!SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
```
Because this step failed, the later lint steps (the structured logging
check, the Java linter, the Connect JVM client MiMa check, and the Python, JS
and R linters) were skipped, so they haven't run on this commit yet.
##########
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:
The precision and scale asserts below only cover exponents above 38, which
returned `null` before. This PR also changes two nearby cases that no test
covers. `'0e38'` is the boundary, the first positive exponent that master
rejected (1 - (-38) = 39 > 38). An in-range zero with a negative scale, such as
`'0e37'` or `'0e5'`, now gets precision 1 instead of a precision derived from
the exponent. I checked this with `java.math.BigDecimal` on JDK 21, mirroring
master and this PR followed by `Decimal.set`:
```
0e5: master precision 6, scale 0 -> this PR precision 1, scale 0
0e37: master precision 38, scale 0 -> this PR precision 1, scale 0
0e38: master null -> this PR precision 1, scale 0
```
Suppose a later change normalizes zero only when the fast-fail would have
fired, or moves the rule into `Decimal.set` for SPARK-60132 and misses this
case. Then `'0e37'` could go back to precision 38, and every current test would
still pass. How about adding `"0e38"` and `"0e37"` to this list, and `"0e38"`
to the `CastSuiteBase` list (L334)?
--
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]
