dongjoon-hyun commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4252556116
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -481,6 +481,42 @@ 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)")
+ }
+ checkEvaluation(cast("0e2147483647", DecimalType(10, 2)),
Decimal("0.00"))
+
+ // The value is shown in plain notation unless its scale is hugely
negative.
+ Seq("1e2147483647" -> "1E+2147483647", "1e40" -> ("1" + "0" *
40)).foreach {
+ case (str, value) =>
+ def expr: Expression = cast(str, DecimalType(10, 2))
+ if (!isTryCast) {
+ // Interpreted, and both codegen paths, which report the error
from `CastUtils`.
Review Comment:
**[Nit] The interpreted path also reports the error from `CastUtils`.**
The interpreted `Cast` also calls `CastUtils.changePrecisionExact`
(`Cast.scala:1358-1360`). So all three evaluations go through the helper that
this PR switches, not only the codegen ones. My earlier comment implied
otherwise, sorry about that.
Also, `evaluateWithMutableProjection` and `evaluateWithUnsafeProjection` run
here in the default `FALLBACK` factory mode. If the generated code failed to
compile, they would silently fall back to interpreted evaluation.
`checkErrorInExpression` runs each projection under both `CODEGEN_ONLY` and
`NO_CODEGEN`. Could we run these two under
`withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key ->
CodegenObjectFactoryMode.CODEGEN_ONLY.toString)` and drop "which report the
error from `CastUtils`" from the comment?
##########
sql/api/src/main/scala/org/apache/spark/sql/errors/DataTypeErrors.scala:
##########
@@ -207,10 +212,14 @@ private[sql] object DataTypeErrors extends
DataTypeErrorsBase {
decimalPrecision: Int,
decimalScale: Int,
context: QueryContext): ArithmeticException = {
+ // A huge negative scale (legacy mode only) makes the plain string too
long to build,
+ // e.g. for 1E+2147483647.
+ val valueString =
Review Comment:
**[Low] Pre-existing: the other caller of this helper passes a stale value.**
`Decimal.set(unscaled, precision, scale)` (`Decimal.scala:83-88`) calls
`unscaledValueTooLargeForPrecisionError(this, precision, scale)` after
`setOrNull` has returned null. `setOrNull` returns before it writes any field,
so `this` still holds its previous value. For a new `Decimal` that is 0. So
`value` here is usually `0`, not the rejected unscaled value. For example,
`Decimal(123456L, 3, 0)` throws "0 cannot be represented as Decimal(3, 0)".
Callers that reach this include `MakeDecimal` under ANSI (`new
Decimal().set(longInput, precision, scale)`) and
`AvroDeserializer.createDecimal` for a value with more digits than its declared
precision.
This is not from this PR and does not need to block it. But the PR changes
how this value is rendered, so could we track it in a JIRA? It could, for
example, build the value from `unscaled` and `scale`.
##########
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.01 ulp of the target scale rounds to 0 or +/-1 ulp
depending only on its
+ // sign and the rounding mode, so it must round like a small value of the
same sign.
+ Seq("1e-2147483647", "-1e-2147483647", "9.99e-100000000", "-1e-100000000",
+ "0e-2147483647").foreach { str =>
+ val bd = new java.math.BigDecimal(str)
+ allSupportedRoundModes.foreach { mode =>
+ val expected = BigDecimal(bd.signum, 50).setScale(2, mode)
+ val d = Decimal(bd)
+ assert(d.changePrecision(10, 2, mode), s"$str, $mode")
+ assert(d.toString === expected.toString, s"$str, $mode")
+ }
+ }
+
+ withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key ->
"true") {
+ Seq("1e2147483647", "-1e2147483646", "12e2147483647",
"1e100000000").foreach { str =>
+ allSupportedRoundModes.foreach { mode =>
+ val d = Decimal(new java.math.BigDecimal(str))
+ assert(d.toPrecision(10, 2, mode) === null, s"$str, $mode")
+ }
+ }
+ val zero = Decimal(new java.math.BigDecimal("0e2147483647"))
+ assert(zero.changePrecision(10, 2))
+ assert(zero.toString === "0.00")
+ }
+
+ // Values around the bounds of the shortcuts must round as
BigDecimal.setScale does.
+ Seq(false, true).foreach { allowNegativeScale =>
+ withSQLConf(
+ SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key ->
allowNegativeScale.toString) {
+ val targets =
+ Seq((10, 2), (3, 3), (5, 0)) ++ (if (allowNegativeScale) Seq((5,
-3)) else Nil)
+ for {
+ // The long mantissas make the scale change large enough to take the
shortcuts.
+ mantissa <- Seq("1", "4.9", "5", "5.1", "9.99", "9.995",
+ "1." + "0" * 79 + "1", "4." + "9" * 80, "5." + "0" * 80, "5." +
"0" * 79 + "1",
+ "9." + "9" * 80)
+ exponent <- -8 to 12
+ sign <- Seq("", "-")
+ mode <- allSupportedRoundModes
+ (precision, scale) <- targets
+ } {
+ val bd = new java.math.BigDecimal(s"$sign${mantissa}e$exponent")
+ val rounded = BigDecimal(bd).setScale(scale, mode)
+ val expected = if (rounded.precision > precision) null else
rounded.toString
+ val d = Decimal(bd).toPrecision(precision, scale, mode)
Review Comment:
**[Low] No test uses a long-form input.**
Every new test builds its input from a string or a `java.math.BigDecimal`,
so `decimalVal` is always set. A long-form value, which `Decimal(unscaled,
precision, scale)`, `Decimal.createUnsafe` and `UnsafeRow.getDecimal` (for a
precision up to 18) return, goes through the Long branch first. No new test
covers it, either the switch to BigDecimal at L451 that reaches the new
shortcut, or the scale changes beyond the Int range in my comment on L465.
A few long-form cases in the negative-scale block would cover both. For
example, `Decimal.createUnsafe(1, 1, -40)` and `Decimal.createUnsafe(0, 1,
-2147483647)` changed to `(10, 2)`, and `Decimal.createUnsafe(100, 3, 2)`
changed to `(1, -2147483647)`.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +458,20 @@ 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's cost grows with the scale change, and it throws once
10^change exceeds
+ // BigInteger's range (e.g. for 1e-2147483647) or the change exceeds
the Int range.
+ // Skip it when the result is known.
+ val numIntegralDigits = dv.precision.toLong - dv.scale
Review Comment:
**[Medium] The Long branch above still computes the scale change in Int.**
This new check computes in Long, but the Long branch of the same function
does not. `val diff = _scale - scale` (L402) and `val diff = scale - _scale`
(L445) are Int. With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`,
`DecimalType` accepts any negative scale, so the difference can exceed the Int
range and wrap to a negative value. Then `POW_10(diff)` throws
`ArrayIndexOutOfBoundsException`:
- L402, when the scale decreases: for any long-form value. For example,
`spark.range(1, 2).select(col("id").cast(DecimalType(10,
2)).cast(DecimalType(1, -2147483647)))`. The inner cast leaves `1.00` as a long
with scale 2, and `diff = 2 - (-2147483647)` wraps to -2147483647.
- L445, when the scale increases: for a long-form zero. For example,
`spark.range(1).select(col("id").cast(DecimalType(1,
-2147483647)).cast(DecimalType(10, 2)))`. `diff` wraps to -2147483647, so `p`
becomes 1, the zero passes `lv > -p && lv < p`, and it reaches `lv *=
POW_10(diff)`. A nonzero value fails that check, switches to BigDecimal and
takes the new shortcut, so only zero fails here.
I reproduced both with a jshell port of this branch:
```
1.00 -> DECIMAL(1, -2147483647): diff = -2147483647 -> THROWS
java.lang.ArrayIndexOutOfBoundsException: Index -2147483647 out of bounds for
length 19
outer diff = -2147483647
THROWS java.lang.ArrayIndexOutOfBoundsException: Index -2147483647 out of
bounds for length 19
```
The same values in BigDecimal form go through the new shortcut and return
`0E+2147483647` and `0.00`, so the result depends on how the value is stored.
`UnsafeRow.getDecimal` returns the long form for a precision up to 18, so any
row round trip can produce it.
This is pre-existing and legacy-only, but it is the same scale-change
problem in the same function. Could we compute both differences in Long, as
this check does, and convert them to Int only where they index `POW_10` after
the `MAX_LONG_DIGITS` checks? Then the Long branch would take its existing
paths: the rounding at L403 and the switch to BigDecimal at L451.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +458,20 @@ 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's cost grows with the scale change, and it throws once
10^change exceeds
+ // BigInteger's range (e.g. for 1e-2147483647) or the change exceeds
the Int range.
+ // 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) {
Review Comment:
**[Nit] The `dv.signum == 0` clause is not needed.**
`BigDecimal.setScale` returns `zeroValueOf(newScale)` immediately when the
value is zero, whatever the scale change. On JDK 17.0.20:
```
0e-2147483647 -> 0.00 in 311us
0e2147483647 -> 0.00 in 6us
0e-100000000 -> 0.00 in 3us
```
JDK 21.0.12 and 25.0.4 give the same results. The `dv.signum != 0` guard on
the overflow check above is still needed, so that a zero is never reported as
an overflow. But a zero could go straight to `setScale` here, and the condition
and its comment could cover only values below 0.01 ulp.
For the same reason, the zero inputs in the new tests (`0e-2147483647` in
`CastSuiteBase` and `DecimalSuite`, and `0e2147483647` in the cast suites and
`DecimalSuite`) pass without this change.
--
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]
