viirya commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4252709595
##########
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:
Good catch. Both differences are now computed in Long and converted to Int
only where they index `POW_10`, after the `MAX_LONG_DIGITS` checks. A compact
value now takes the existing rounding path or switches to BigDecimal, and
matches the BigDecimal form.
##########
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:
Added compact inputs to the negative-scale block, including your three
examples, compared against the same values in BigDecimal form under every
rounding mode. Without the Long-branch change, this fails with
`ArrayIndexOutOfBoundsException`.
##########
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:
Done. The two projections now run under `CODEGEN_ONLY`, and the comment is
fixed.
##########
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:
Thanks, you're right. Removed the `dv.signum == 0` clause. I kept the zero
inputs in the tests, since `0e2147483647` with negative scales allowed still
guards the `dv.signum != 0` check on the overflow side.
##########
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:
Thanks. Filed SPARK-60205 for it.
--
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]
