dongjoon-hyun commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4247345658
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -648,8 +649,10 @@ object 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 &&
+ // For example: Decimal("6.0790316E+25569151"). Zero fits any decimal
type whatever its
+ // exponent, e.g. "0e39", so it is not checked.
+ if (bigDecimal.signum != 0 &&
Review Comment:
With the zero exemption, a zero with a large exponent goes to
`Decimal(bigDecimal)`. `Decimal.set` then computes `this._precision =
decimal.precision - decimal.scale` (L152), which is the same `Int` expression
this PR moves to `Long` in `numDigitsInIntegralPart`:
```
0e39: bd.precision=1 bd.scale=-39 -> Decimal._precision=40
-0E+100: bd.precision=1 bd.scale=-100 -> Decimal._precision=101
0e2147483647: bd.precision=1 bd.scale=-2147483647 ->
Decimal._precision=-2147483648
```
So `Decimal.fromString("0e39")` used to return `null` and now returns a
`Decimal` whose precision is 40, above `MAX_PRECISION`. The `'0e2147483647'`
case was already like this on master. `Cast` is not affected because
`changePrecision` rescales the value, but `fromString` / `fromStringANSI` are
public, and a caller that reads `.precision` or builds
`DecimalType(d.precision, d.scale)` gets an invalid value.
SPARK-60132 tracks `Decimal.set` in general. Since this PR is what newly
sends these zeros there, how about normalizing zero here? For example,
`Decimal(bigDecimal.setScale(math.max(bigDecimal.scale, 0)))` for `signum ==
0`, which keeps a non-negative scale such as `'0.00'` and gives precision 1 for
`'0e39'`.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -475,6 +475,14 @@ class CastWithAnsiOnSuite extends CastSuiteBase with
QueryErrorsBase {
checkExceptionInExpression[ArithmeticException](
cast("6E+37", DecimalType(38, 1)),
"cannot be represented as Decimal(38, 1)")
+ Seq("1e2147483647", "1.5e2147483647", "12e2147483647").foreach { s =>
+ checkExceptionInExpression[ArithmeticException](
+ cast(s, DecimalType(10, 2)),
+ "NUMERIC_OUT_OF_SUPPORTED_RANGE")
+ }
+ Seq("0e39", "0e2147483647").foreach { s =>
Review Comment:
These zero checks are identical to the ones added in `CastWithAnsiOffSuite`
(L565-567), because the expected `0.00` doesn't depend on ANSI mode.
Could they move to `CastSuiteBase`? Then they run once for ANSI on, ANSI off
and `try_cast`, and there is only one copy to maintain.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,24 @@ class DecimalSuite extends SparkFunSuite with
PrivateMethodTester with SQLHelper
}
}
+ test("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 =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === Decimal(0))
Review Comment:
`Decimal.equals` compares only the value (through `compare`), so `===
Decimal(0)` passes for any precision and scale. It passes today although
`fromString("0e39")` has precision 40 and `fromString("0e2147483647")` has
precision `-2147483648` (see the comment in `Decimal.scala`).
Could you also assert the precision and scale, e.g. `precision` within `[1,
DecimalType.MAX_PRECISION]` and `scale == 0`?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -668,8 +671,10 @@ object 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 &&
+ // For example: Decimal("6.0790316E+25569151"). Zero fits any decimal
type whatever its
+ // exponent, e.g. "0e39", so it is not checked.
+ if (bigDecimal.signum != 0 &&
Review Comment:
This three-line condition and its three-line comment are a copy of the ones
in `fromString` (L651-656). This PR already had to change both copies the same
way for the zero exemption.
How about a shared private predicate, e.g.
`exceedsMaxPrecision(bigDecimal)`? Another option is to fold the zero rule into
`numDigitsInIntegralPart`, since zero has one integral digit. Either way the
rule would be defined in one place.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,24 @@ class DecimalSuite extends SparkFunSuite with
PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
Review Comment:
nit: The neighboring regression tests use the JIRA prefix (`SPARK-37451:
...`, `SPARK-41554: ...`). How about `test("SPARK-60118: UTF8String to Decimal
with exponent near Int.MaxValue")`?
--
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]
