dongjoon-hyun commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4236865023
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ 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 =
Review Comment:
The fast-fail is skipped when `spark.sql.legacy.allowNegativeScaleOfDecimal`
is `true`, so this fix does not apply under the legacy conf. `Decimal.set`
keeps the negative scale, and `changePrecision` fails in `setScale`:
```
spark.sql.legacy.allowNegativeScaleOfDecimal=true
try_cast('1e2147483646' AS DECIMAL(10,2)) => ArithmeticException: Underflow
```
This was already the case before this PR. Should it be covered here, or
should the PR description say that the legacy conf is out of scope?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ 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
Review Comment:
This covers large positive exponents, but the mirror case with a large
negative exponent still fails with a raw `ArithmeticException`. For
`'1e-2147483647'`, `precision - scale` is `1 - 2147483647`, so the fast-fail is
not taken. `Decimal.set` then sets `_precision = _scale = 2147483647`, and
`changePrecision` calls `dv.setScale(2, ...)`, which throws.
On the current master build (the path is the same with this PR):
```
try_cast('1e-2147483647' AS DECIMAL(10,2)) =>
ArithmeticException: BigInteger would overflow supported range
CAST('1e-2147483647' AS DECIMAL(10,2)), ansi.enabled=false => same
CAST('1e-2147483647' AS DECIMAL(10,2)), ansi.enabled=true => same
```
This value rounds to `0.00`, the same as `'1e-100'`. Since it is the same
failure as the one in the PR description, could you handle this case in this PR
too?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,19 @@ class DecimalSuite extends SparkFunSuite with
PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
Review Comment:
Depending on how you handle the comments in `Decimal.scala`, could you add
these cases?
- The negative-exponent case, `'1e-2147483647'`.
- A cast case with `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, like
the `withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key ->
"true")` block in the SPARK-37451 test above. Under the legacy conf,
`fromString` itself succeeds and the exception comes from `changePrecision`, so
it needs a cast-level check.
Both still fail with a raw `ArithmeticException`, and the current tests
don't catch them.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ 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".
Review Comment:
The fast-fail exists because converting a huge value is slow (see the
`Decimal("6.0790316E+25569151")` comment below), but it only bounds large
positive exponents. A large negative exponent goes through the same slow path
in `setScale`:
```
try_cast('1e-10000000' AS DECIMAL(10,2)), 1 row => 0.00 (1867 ms)
try_cast('1e-10000000' AS DECIMAL(10,2)), 5 rows => 0.00 (8779 ms)
```
This was already the case before this PR, so it can be a separate JIRA. A
bound on the negative side could fix this and the `'1e-2147483647'` exception
together. For example, a value below `10^-(MAX_SCALE + 1)` rounds to zero for
any target scale.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ 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 =
Review Comment:
`Decimal.set(BigDecimal)` has the same Int expression, `this._precision =
decimal.precision - decimal.scale` (L152). Other entry points that build a
`Decimal` from a `BigDecimal` don't go through this bound either. For example,
`Decimal(String)`, and the CSV/JSON readers with a decimal schema
(`UnivocityParser` L226, `JacksonParser` L441) call `Decimal(bd, precision,
scale)`, which goes straight to `setScale`.
Would it make sense to move the bound (in `Long`) into `Decimal.set` or a
shared helper, so these paths don't depend on the string-cast fast path? This
can be a separate JIRA.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -475,6 +475,11 @@ class CastWithAnsiOnSuite extends CastSuiteBase with
QueryErrorsBase {
checkExceptionInExpression[ArithmeticException](
cast("6E+37", DecimalType(38, 1)),
"cannot be represented as Decimal(38, 1)")
+ Seq("1e2147483647", "0e2147483647", "1.5e2147483647",
"1e2147483648").foreach { s =>
Review Comment:
`'0e2147483647'` is exactly zero, and it fits `DECIMAL(10,2)`. On master it
returns `0.00`. With this PR it becomes `NULL` (non-ANSI) or
`NUMERIC_OUT_OF_SUPPORTED_RANGE` (ANSI), and these tests lock that in.
It is consistent with `'0e39'` and `'0e100'`, as the PR description says,
but those look like the same false positive: the integral-digit check counts
`precision - scale` even for zero.
```
spark.sql.ansi.enabled=false, master
CAST(s AS DECIMAL(10,2)) for '0e2147483647', '0e39', '0e37' => 0.00, null,
0.00
```
How about skipping the fast-fail for zero, e.g. `bigDecimal.signum != 0 &&
numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION`? Then
`'0e2147483647'` keeps returning `0.00`, and `'0e39'` is fixed too.
--
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]
