dongjoon-hyun commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4256047727
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -492,4 +492,77 @@ 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")
+
+ // A compact (long) input takes the Long branch first, where the scale
change can exceed
+ // the Int range, and may switch to the BigDecimal branch. It must round
like the same
+ // value in BigDecimal form.
+ Seq(
+ (1L, 1, -40, 10, 2),
Review Comment:
**[Nit] Two of the compact cases have no independent expected value.**
For `(1L, 1, -40, 10, 2)` and `(0L, 1, -2147483647, 10, 2)`, the Long branch
switches to BigDecimal (`dv = BigDecimal(lv, _scale)` at `Decimal.scala` L453).
After that, the compact and expanded forms run through the same BigDecimal
branch, so comparing them cannot catch a mistake in that branch. For example,
if the overflow shortcut returned `true` for `1E+40` to `DECIMAL(10, 2)`, both
sides would agree and the test would still pass. Only the `100L` and `-100L`
cases compare two different code paths.
Asserting the expected results for these two cases (`null` and `"0.00"`)
would cover this. Optionally, a nonzero compact value whose scale increases
beyond the Int range, such as `(1L, 1, Int.MinValue, 38, 20)`, would also cover
the new `p` computation at `Decimal.scala` L447. The old code threw
`ArrayIndexOutOfBoundsException` there for a nonzero value too: the wrapped
`diff` made `MAX_LONG_DIGITS - diff` equal 2147483646, and `POW_10(2147483646)`
is out of range.
##########
sql/api/src/main/scala/org/apache/spark/sql/errors/DataTypeErrors.scala:
##########
@@ -31,6 +31,11 @@ import org.apache.spark.unsafe.types.UTF8String
* into [[CompilationErrors]].
*/
private[sql] object DataTypeErrors extends DataTypeErrorsBase {
+ // A decimal whose scale is below -MAX_PLAIN_STRING_TRAILING_ZEROS (that is,
with more than
+ // this many zeros implied by a negative scale) is rendered in scientific
notation in error
+ // messages instead of plain notation.
+ private val MAX_PLAIN_STRING_TRAILING_ZEROS = 1000
Review Comment:
**[Nit] Could this constant move next to `numericValueOutOfRange`?**
It is used only by `numericValueOutOfRange` (L210), about 175 lines below,
and `DataTypeErrors` has no constants section at the top. `AGENTS.md` asks to
"put the new member with the code it belongs with". Placing it right above
`numericValueOutOfRange` would keep the threshold next to the code that applies
it.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +459,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:
**[Nit] This duplicates `numDigitsInIntegralPart`.**
`Decimal.numDigitsInIntegralPart` (L653) computes the same `precision -
scale`. #59334 (SPARK-60118) changes it to return Long for the same overflow
reason (`bigDecimal.precision.toLong - bigDecimal.scale`). Whichever of the two
PRs merges second could call `numDigitsInIntegralPart(dv.underlying)` here.
Then the integral-digit count would live in one place, and a later fix to one
copy could not miss the other. The class already imports `Decimal._`, so the
private helper is accessible.
--
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]
