Repository navigation
[SPARK-60119][SQL] Skip rescaling in Decimal.changePrecision when the result is known from the magnitude #59335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shortcut protects only Would it make sense to move this magnitude check into a small shared rescale helper used by
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that |
||
| if (dv.precision > precision) { | ||
| return false | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -481,6 +481,36 @@ 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)") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This checks only the message substring, so the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a |
||
| } | ||
| 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) => | ||
| if (!isTryCast) { | ||
| checkError( | ||
| exception = intercept[SparkArithmeticException]( | ||
| cast(str, DecimalType(10, 2)).eval()), | ||
| condition = "NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION", | ||
| parameters = Map( | ||
| "value" -> value, | ||
| "precision" -> "10", | ||
| "scale" -> "2", | ||
| "config" -> """"spark.sql.ansi.enabled""""), | ||
| queryContext = Array(ExpectedContext(fragment = "", start = -1, stop = -1))) | ||
| } else { | ||
| checkEvaluation(cast(str, DecimalType(10, 2)), null) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| protected def checkCastToBooleanError(l: Literal, to: DataType, tryCastResult: Any): Unit = { | ||
| checkExceptionInExpression[SparkRuntimeException]( | ||
| cast(l, to), """cannot be cast to "BOOLEAN"""") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses
Longarithmetic for the scale difference and the integral digits, but the existing fast-fail inDecimal.fromString/fromStringANSIstill usesnumDigitsInIntegralPart, which computesbigDecimal.precision - bigDecimal.scaleinInt. For1e2147483647(precision 1, scale -2147483647), the result overflows to-2147483648, so the> DecimalType.MAX_PRECISIONfast-fail is skipped. With the defaultspark.sql.legacy.allowNegativeScaleOfDecimal=false,Decimal(bigDecimal)then goes toset(decimal), which callsdecimal.setScale(0):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 rawArithmeticExceptioninstead ofNUMERIC_VALUE_OUT_OF_RANGE.'12e2147483646'hits the same overflow. The new tests cover these huge exponents only with the legacy config on. Could you computenumDigitsInIntegralPartinLongas well and add the non-legacy cases toCastSuiteBase?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This is the Int overflow in
numDigitsInIntegralPart, which is fixed separately in #59334 (SPARK-60118) together with non-legacyCastWithAnsiOffSuite/CastWithAnsiOnSuitecases for1e2147483647,12e2147483647, etc. This PR covers the remaining cases that reachchangePrecision.