dongjoon-hyun commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4236859782
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -481,6 +481,17 @@ class CastWithAnsiOnSuite extends CastSuiteBase with
QueryErrorsBase {
castErrMsg("abcd", DecimalType(38, 1)))
}
+ test("SPARK-60119: cast string with a huge exponent to decimal with negative
scale allowed") {
+ withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key ->
"true") {
+ Seq("1e2147483647", "-1e2147483646", "12e2147483647",
"1e100000000").foreach { str =>
+ checkExceptionInExpression[ArithmeticException](
+ cast(str, DecimalType(10, 2)),
+ "cannot be represented as Decimal(10, 2)")
Review Comment:
This checks only the message substring, so the `DataTypeErrors` change
(rendering a negative-scale value like `1E+2147483647` with `toString`) is not
asserted directly. If that branch were changed later, e.g. to print a truncated
plain string or the wrong value, this test would still pass. Could we add a
`checkError` on `NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` with the expected
`value` parameter for at least one of these inputs?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +458,19 @@ final class Decimal extends Ordered[Decimal] with
Serializable {
if (dv.ne(null)) {
// We get here if either we started with a BigDecimal, or we switched to
one because we would
// have overflowed our Long; in either case we must rescale dv to the
new scale.
+ if (math.abs(dv.scale.toLong - scale) > DecimalType.MAX_PRECISION) {
+ // setScale takes time proportional to the scale change and fails once
the change
+ // exceeds the Int range, e.g. for 1e-2147483647. Skip it when the
result is known.
+ val numIntegralDigits = dv.precision.toLong - dv.scale
+ if (dv.signum != 0 && numIntegralDigits > precision.toLong - scale) {
+ return false
+ }
+ if (dv.signum == 0 || numIntegralDigits < -scale.toLong - 1) {
+ // |dv| is below 0.01 ulp of the new scale, so the result depends
only on the sign
+ // and the rounding mode. Round a value of the same sign and 0.01
ulp instead.
+ dv = BigDecimal(dv.signum, scale + 2)
+ }
+ }
dv = dv.setScale(scale, roundMode)
Review Comment:
This shortcut protects only `changePrecision`. `Decimal.set(decimal:
BigDecimal, precision: Int, scale: Int)` still calls `decimal.setScale(scale,
ROUND_HALF_UP)` without a bound, and it is used by the JSON, CSV and XML
parsers (`JacksonParser.scala:441/444`, `UnivocityParser.scala:226`,
`StaxXmlParser.scala:788`). For example, `from_json('{"a": 1e-2147483647}', 'a
DECIMAL(10,2)')`, or a CSV/XML column `1e-2147483647` read with a
`DECIMAL(10,2)` schema, still fails with `BigInteger would overflow supported
range`, and `1e-100000000` still takes ~40s per value. `set(decimal)`'s
`decimal.setScale(0)` has the same issue.
Would it make sense to move this magnitude check into a small shared rescale
helper used by `changePrecision` and both `set` overloads, so every entry point
is covered? If you prefer to keep this PR focused on casts, a separate JIRA
would be fine too.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +458,19 @@ final class Decimal extends Ordered[Decimal] with
Serializable {
if (dv.ne(null)) {
// We get here if either we started with a BigDecimal, or we switched to
one because we would
// have overflowed our Long; in either case we must rescale dv to the
new scale.
+ if (math.abs(dv.scale.toLong - scale) > DecimalType.MAX_PRECISION) {
Review Comment:
This uses `Long` arithmetic for the scale difference and the integral
digits, but the existing fast-fail in `Decimal.fromString`/`fromStringANSI`
still uses `numDigitsInIntegralPart`, which computes `bigDecimal.precision -
bigDecimal.scale` in `Int`. For `1e2147483647` (precision 1, scale
-2147483647), the result overflows to `-2147483648`, so the `>
DecimalType.MAX_PRECISION` fast-fail is skipped. With the default
`spark.sql.legacy.allowNegativeScaleOfDecimal=false`, `Decimal(bigDecimal)`
then goes to `set(decimal)`, which calls `decimal.setScale(0)`:
```
jshell> var b = new java.math.BigDecimal("1e2147483647");
jshell> b.precision() - b.scale()
$2 ==> -2147483648
jshell> b.setScale(0)
| Exception java.lang.ArithmeticException: BigInteger would overflow
supported range
```
So `SELECT try_cast('1e2147483647' AS DECIMAL(10,2))` (and the non-ANSI
cast) throws instead of returning NULL, and the ANSI cast throws a raw
`ArithmeticException` instead of `NUMERIC_VALUE_OUT_OF_RANGE`.
`'12e2147483646'` hits the same overflow. The new tests cover these huge
exponents only with the legacy config on. Could you compute
`numDigitsInIntegralPart` in `Long` as well and add the non-legacy cases to
`CastSuiteBase`?
##########
sql/api/src/main/scala/org/apache/spark/sql/errors/DataTypeErrors.scala:
##########
@@ -210,7 +210,9 @@ private[sql] object DataTypeErrors extends
DataTypeErrorsBase {
new SparkArithmeticException(
errorClass = "NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION",
messageParameters = Map(
- "value" -> value.toPlainString,
+ // A negative scale (legacy mode only) can make the plain string
arbitrarily long,
+ // e.g. 1E+2147483647.
+ "value" -> (if (value.scale < 0) value.toString else
value.toPlainString),
Review Comment:
This changes the message for every negative-scale value, not only for the
huge ones. With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, an
overflow of a value stored as unscaled `1` with scale `-2` used to show `100`
and now shows `1E+2`, and the same applies to ordinary values like `1E+40`.
Since only very large `-scale` values make the plain string too long, would it
be better to switch to `toString` only above some threshold (e.g. when
`-value.scale` exceeds a reasonable number of digits), so the existing message
stays the same for normal values?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -492,4 +492,57 @@ class DecimalSuite extends SparkFunSuite with
PrivateMethodTester with SQLHelper
s"unscaled: $unscaled, scaleFrom: $scaleFrom, scaleTo: $scaleTo, mode:
$roundMode")
}
}
+
+ test("SPARK-60119: changePrecision with a source scale far from the target
scale") {
+ // A value below 0.1 ulp of the target scale rounds to 0 or +/-1 ulp
depending only on its
Review Comment:
nit. This comment says `below 0.1 ulp`, while the shortcut in
`changePrecision` applies below `0.01 ulp` (`numIntegralDigits < -scale.toLong
- 1`), and its comment says `0.01 ulp`. The statement is mathematically true,
but it can make readers think values in `[0.01, 0.1)` ulp take the shortcut.
Could you align it with the main code?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -108,15 +108,8 @@ private[sql] object QueryExecutionErrors extends
QueryErrorsBase with ExecutionE
decimalPrecision: Int,
decimalScale: Int,
context: QueryContext = null): ArithmeticException = {
- new SparkArithmeticException(
- errorClass = "NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION",
- messageParameters = Map(
- "value" -> value.toPlainString,
- "precision" -> decimalPrecision.toString,
- "scale" -> decimalScale.toString,
- "config" -> toSQLConf(SQLConf.ANSI_ENABLED.key)),
- context = getQueryContext(context),
- summary = getSummary(context))
+ DataTypeErrors.cannotChangeDecimalPrecisionError(
Review Comment:
nit. Now this method only forwards to
`DataTypeErrors.cannotChangeDecimalPrecisionError` with the same signature.
Since its only main-code caller is `CastUtils.java:110`, we can call
`DataTypeErrors` there directly and remove this wrapper, so that the two copies
cannot drift apart again.
--
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]
