Repository navigation
Conversation
… result is known from the magnitude In the BigDecimal branch of `Decimal.changePrecision`, when the scale change exceeds `DecimalType.MAX_PRECISION`, check the value's number of integral digits before calling `setScale`: a nonzero value with too many integral digits overflows, and a zero or a value below 0.01 ulp of the target scale is replaced by a same-sign value of 0.01 ulp, which rounds the same under every rounding mode. This avoids `ArithmeticException` and very slow rescaling for inputs like '1e-2147483647' and, with negative scales allowed, '1e2147483647'. `NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` now renders a decimal with a negative scale with `toString`, since its plain form can be too long to fit in a String. Co-authored-by: Isaac <[email protected]>
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for the fix, @viirya. The magnitude shortcut in changePrecision itself looks correct to me: a nonzero value with more integral digits than precision - scale overflows under every rounding mode (including ROUND_DOWN, since 10^(p-s) is a multiple of the target ulp), and the remaining setScale work is bounded by the number of digits of the value. I left 6 inline comments. Summary:
Decimal.fromString/fromStringANSIstill crash on'1e2147483647'with the default (non-legacy) config, becausenumDigitsInIntegralPartoverflowsIntand the existing fast-fail is skipped.try_castthrowsArithmeticExceptioninstead of returning NULL.Decimal.set(decimal, precision, scale), used by the JSON/CSV/XML parsers, still callssetScalewithout a bound, sofrom_jsonwith1e-2147483647still fails and1e-100000000is still slow. A shared rescale helper would cover all entry points.- The error message now uses scientific notation for every negative-scale value (e.g.
1E+2instead of100), not only for huge ones. - The ANSI test checks only a message substring, not the error condition or the new
valuerendering. - A test comment says
0.1 ulp, while the code uses0.01 ulp. QueryExecutionErrors.cannotChangeDecimalPrecisionErroris now a pure passthrough and can be removed.
| 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) { |
There was a problem hiding this 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?
There was a problem hiding this comment.
Thanks. This is the Int overflow in numDigitsInIntegralPart, which is fixed separately in #59334 (SPARK-60118) together with non-legacy CastWithAnsiOffSuite/CastWithAnsiOnSuite cases for 1e2147483647, 12e2147483647, etc. This PR covers the remaining cases that reach changePrecision.
| dv = BigDecimal(dv.signum, scale + 2) | ||
| } | ||
| } | ||
| dv = dv.setScale(scale, roundMode) |
There was a problem hiding this 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.
There was a problem hiding this comment.
Agreed that set(decimal, precision, scale) and set(decimal) have the same issue. I'd like to keep this PR focused on changePrecision. The parser path also needs a decision on how to report overflow there (NUMERIC_VALUE_OUT_OF_RANGE.WITHOUT_SUGGESTION carries a roundedValue). Filed SPARK-60133 for it.
| "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), |
There was a problem hiding this 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?
There was a problem hiding this comment.
Good point. It now uses toString only when the scale is below -1000, so the message for ordinary values like 1E+2 and 1E+40 is unchanged.
| Seq("1e2147483647", "-1e2147483646", "12e2147483647", "1e100000000").foreach { str => | ||
| checkExceptionInExpression[ArithmeticException]( | ||
| cast(str, DecimalType(10, 2)), | ||
| "cannot be represented as Decimal(10, 2)") |
There was a problem hiding this 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?
There was a problem hiding this comment.
Added a checkError with the expected value for 1e2147483647 (1E+2147483647) and for 1e40 (plain notation).
| } | ||
|
|
||
| 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 |
There was a problem hiding this 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?
| "config" -> toSQLConf(SQLConf.ANSI_ENABLED.key)), | ||
| context = getQueryContext(context), | ||
| summary = getSummary(context)) | ||
| DataTypeErrors.cannotChangeDecimalPrecisionError( |
There was a problem hiding this 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.
…rt it Co-authored-by: Isaac <[email protected]>
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
Review summary
The magnitude shortcut in Decimal.changePrecision looks correct. A nonzero value with more integral digits than precision - scale is at least 10^(precision - scale), so it overflows under every rounding mode. A value below 0.01 ulp rounds exactly like the same-sign 0.01-ulp value it is replaced with. Both early exits return before any field is written. The DecimalSuite boundary loop checks both bounds against BigDecimal.setScale with 81-digit mantissas, in both signs, under every supported rounding mode, and with and without negative scales. The two findings are minor wording issues in new comments: the shortcut's justification misstates when setScale fails, and the rendering threshold is described as a trailing-zero count while the check compares the scale.
The earlier review feedback is addressed in this revision. Scientific notation now applies only below scale -1000. The ANSI test asserts the error condition and the value parameter in both notations. The DecimalSuite comment matches the 0.01-ulp bound. The duplicate QueryExecutionErrors helper is gone. The default-mode fromString overflow for 1e2147483647 and the parser Decimal.set paths are unchanged pre-existing behavior that the author has deferred to SPARK-60118 (#59334) and SPARK-60133.
Cross-cutting observations
- Both findings are wording nits in new explanatory text; the code they describe is correct. The PR description repeats both statements ("throws once it exceeds the Int range" and "more than 1000 trailing zeros"), so rewording it alongside the comments would keep the description consistent with the code.
Separation: The comments describe different mechanisms in different files. Their shared theme does not create a common owner, a coordinated repair, or any change to their P3 severity.
Findings
2 total: 0 P0, 0 P1, 0 P2, 2 P3.
Nit (P3)
- Shortcut comment misstates when
setScalefails —sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:462— see inline. - Threshold comment says trailing zeros, but the check compares the scale —
sql/api/src/main/scala/org/apache/spark/sql/errors/DataTypeErrors.scala:34— see inline.
Verification
- Checked on a local JDK 11: BigDecimal("1e-2147483647").setScale(2, HALF_UP) throws "BigInteger would overflow supported range", while setScale(-3) on the same value and setScale(2) on 1e2147483647 throw "Underflow".
- Checked on a local JDK 11: new BigDecimal("1000e1000") has unscaled value 1000, scale -1000, and a 1004-character plain string.
PR description suggestions
- In "Why are the changes needed?", "
setScaletakes time proportional to the scale change and throws once it exceeds the Int range" does not match the first example:1e-2147483647to scale 2 is within the Int range and fails because 10^change overflows BigInteger. Consider "setScaletakes time that grows with the scale change and throws once 10^change exceeds BigInteger's range or the change exceeds the Int range". - "a value with more than 1000 trailing zeros (e.g.
1E+2147483647) is shown in scientific notation" could say "a value whose scale is below -1000", since that is the implemented rule (for example,1000e1000has 1003 trailing zeros but keeps plain notation).
Generated by Omnigent on Databricks.
| // 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 |
There was a problem hiding this comment.
Nit (P3): The example here doesn't match the stated condition. Rescaling 1e-2147483647 (scale 2147483647) to scale 2 is a change of 2147483645, which fits in an Int. On the JDK, new BigDecimal("1e-2147483647").setScale(2, RoundingMode.HALF_UP) throws BigInteger would overflow supported range, because 10^change is too large for BigInteger. The Int-range check fails differently (Underflow), and only for other inputs, e.g. 1e2147483647 to scale 2 with negative scales allowed. Could you reword it to something like:
// 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.Verification:
- Inspection: The revised comment matches BigDecimal.setScale's behavior for each cited example, and the diff touches only comment lines.
There was a problem hiding this comment.
Thanks, reworded as suggested.
| * into [[CompilationErrors]]. | ||
| */ | ||
| private[sql] object DataTypeErrors extends DataTypeErrorsBase { | ||
| // Above this many trailing zeros, a decimal value in an error message is rendered in |
There was a problem hiding this comment.
Nit (P3): This describes the threshold as a count of trailing zeros, but the check in numericValueOutOfRange is value.scale < -MAX_PLAIN_STRING_TRAILING_ZEROS, which counts only the zeros implied by a negative scale. For example, 1000e1000 parses to unscaled 1000 with scale -1000. Its plain form has 1003 trailing zeros, but it is still rendered in plain notation. The behavior itself looks right for keeping the string bounded, so only the wording needs to match, e.g.:
// 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.Verification:
- Inspection: The revised comment describes the same condition as the check in numericValueOutOfRange, and the diff touches only comment lines.
There was a problem hiding this comment.
Thanks, reworded as suggested. Also updated the PR description for both points.
Co-authored-by: Isaac <[email protected]>
Co-authored-by: Isaac <[email protected]>
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for addressing the previous comments, @viirya. All six points from my earlier review are now handled. Points 3-6 are fixed in this revision, and points 1 and 2 are deferred to #59334 (SPARK-60118) and SPARK-60133. I checked that #59334 and this PR merge without conflicts. The shortcut in changePrecision still looks correct to me. I left 3 non-blocking inline comments. Summary:
round/broundwith a huge negative scale (mathExpressions.scala:1782) still callsetScalewithout a bound.round(1.5BD, -2147483646)throwsArithmeticExceptioneven without the legacy config. Could SPARK-60133 cover this path too?- The plain-string guard applies only to the error message. Under ANSI, casting to string, and
Dataset.showthroughToPrettyString, still calltoPlainStringon a decimal with a huge negative scale, which fails withOutOfMemoryError(legacy config only, pre-existing). - The new
checkErroron thevalueparameter covers only interpreted evaluation.checkErrorInExpressionwould also cover the codegen paths.
| 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) { |
There was a problem hiding this comment.
This shortcut fixes changePrecision, but RoundBase still rescales without a bound. For a negative _scale, nullSafeEval calls decimal.toBigDecimal.setScale(_scale, mode) directly (mathExpressions.scala:1782). The byte/short/int/long branches (mathExpressions.scala:1786-1807) and the generated code (mathExpressions.scala:1856-1857) do the same. On the JDK:
jshell> new BigDecimal("1.5").setScale(-2147483646, RoundingMode.HALF_UP)
java.lang.ArithmeticException: BigInteger would overflow supported range
jshell> new BigDecimal("1.5").setScale(-2147483647, RoundingMode.HALF_UP)
java.lang.ArithmeticException: Underflow
So SELECT round(1.5BD, -2147483646) fails with a raw ArithmeticException instead of returning 0. Unlike the cast cases, this does not need spark.sql.legacy.allowNegativeScaleOfDecimal, because round/bround accept any foldable Int scale.
This is pre-existing, so it does not need to block this PR. Since SPARK-60133 already covers Decimal.set(decimal, precision, scale), could we add round/bround to its scope? A shared helper in Decimal built from the logic here could then serve all of these callers.
There was a problem hiding this comment.
Thanks. #58939 (SPARK-59686) is already working on this path: it clamps RoundBase._scale, which every branch reads, including the decimal one and the generated code. I'd rather leave it there than fold it into SPARK-60133. If that PR stalls, I'll pick it up in SPARK-60133.
| // A huge negative scale (legacy mode only) makes the plain string too long to build, | ||
| // e.g. for 1E+2147483647. | ||
| val valueString = | ||
| if (value.scale < -MAX_PLAIN_STRING_TRAILING_ZEROS) value.toString else value.toPlainString |
There was a problem hiding this comment.
This bounds the plain string only in the NUMERIC_VALUE_OUT_OF_RANGE message. Other code still calls Decimal.toPlainString on the same values:
Castto string uses plain notation under ANSI (useDecimalPlainString = ansiEnabled,Cast.scala:829, used atToStringBase.scala:200andToStringBase.scala:353).ToPrettyString, whichDataset.showuses, always does (ToPrettyString.scala:50).
DecimalType has no lower bound on a negative scale. With spark.sql.legacy.allowNegativeScaleOfDecimal=true and the default ANSI mode, both of these build the full plain string:
lit("1e2147483647").cast(DecimalType(1, -2147483647)).cast("string")
spark.range(1).select(lit(new java.math.BigDecimal("1e2147483647"))).show()The inner cast succeeds because the precision and scale already match, so changePrecision takes its fast path. On the JDK, new BigDecimal("1e2147483647").toPlainString() throws OutOfMemoryError: too large to fit in a String.
This is pre-existing and legacy-only, so it does not need to block this PR. But it is the same problem this guard solves. Could we track it in a JIRA, or move the threshold into a Decimal-level helper that these callers can share?
There was a problem hiding this comment.
Good catch. Filed SPARK-60200 to bound the plain string for decimals with a huge negative scale in cast-to-string and ToPrettyString, possibly with a Decimal-level helper shared with this error message.
| if (!isTryCast) { | ||
| checkError( | ||
| exception = intercept[SparkArithmeticException]( | ||
| cast(str, DecimalType(10, 2)).eval()), |
There was a problem hiding this comment.
nit. This checkError runs only the interpreted path (.eval()). The codegen path goes through CastUtils.changePrecisionExact, whose error helper this PR switches from QueryExecutionErrors to DataTypeErrors. The loop above checks that path only by a message substring, so the value parameter is not asserted there. checkErrorInExpression[SparkArithmeticException](cast(str, DecimalType(10, 2)), "NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION", Map(...)) checks the parameters in the interpreted, mutable-projection, and unsafe-projection modes. TryCastSuite already overrides it to expect null, so the isTryCast branch would no longer be needed. It does not check queryContext, but that is the same empty context in every mode here.
There was a problem hiding this comment.
Done, with one change: checkErrorInExpression doesn't pass queryContext to checkError, so it expects no context while this error carries an empty one, and the check fails. The test now runs checkError over the interpreted, mutable-projection and unsafe-projection paths itself.
Co-authored-by: Isaac <[email protected]>
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for addressing the previous comments, @viirya. All three points from my last review are handled:
round/broundis left to #58939 (SPARK-59686).- The plain-string bound for cast-to-string and
ToPrettyStringis tracked in SPARK-60200. - The ANSI test now checks the error parameters in the interpreted, mutable-projection and unsafe-projection paths.
The shortcut in the BigDecimal branch still looks correct to me. I reviewed d6b9f83 and left 5 inline comments. The first one is about the Long branch of the same function, which can still fail with ArrayIndexOutOfBoundsException when the scale change exceeds the Int range. The others are minor.
Here is a summary, ordered by priority.
Correctness
Decimal.scalaL465: the Long branch ofchangePrecisionstill computes the scale change in Int (L402, L445). With negative scales allowed, a long-form value can fail withArrayIndexOutOfBoundsExceptionwhen its scale change exceeds the Int range. The same value in BigDecimal form now goes through the new shortcut and returns 0.
Tests
DecimalSuite.scalaL541: every new test builds its input from a string or ajava.math.BigDecimal, so no test covers a long-form input, whichUnsafeRow.getDecimalreturns for a precision up to 18.CastWithAnsiOnSuite.scalaL498: the interpreted path also reports the error fromCastUtils. The two projections run in the defaultFALLBACKfactory mode, which does not guarantee codegen.
Minor
Decimal.scalaL469: thedv.signum == 0clause is not needed, becauseBigDecimal.setScalereturns a zero immediately for any scale change. So the zero inputs in the new tests pass without this change.
Pre-existing
DataTypeErrors.scalaL217:Decimal.set(unscaled, precision, scale)passesthisbefore it is updated, so the error shows the old value, usually0, instead of the rejected one.
| // 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 |
There was a problem hiding this 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 leaves1.00as a long with scale 2, anddiff = 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))).diffwraps to -2147483647, sopbecomes 1, the zero passeslv > -p && lv < p, and it reacheslv *= 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.
There was a problem hiding this 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.
| 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) |
There was a problem hiding this 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).
There was a problem hiding this 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.
| case (str, value) => | ||
| def expr: Expression = cast(str, DecimalType(10, 2)) | ||
| if (!isTryCast) { | ||
| // Interpreted, and both codegen paths, which report the error from `CastUtils`. |
There was a problem hiding this 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?
There was a problem hiding this comment.
Done. The two projections now run under CODEGEN_ONLY, and the comment is fixed.
| if (dv.signum != 0 && numIntegralDigits > precision.toLong - scale) { | ||
| return false | ||
| } | ||
| if (dv.signum == 0 || numIntegralDigits < -scale.toLong - 1) { |
There was a problem hiding this 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.
There was a problem hiding this 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.
| 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 = |
There was a problem hiding this 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.
Co-authored-by: Isaac <[email protected]>
| // 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), |
There was a problem hiding this 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.
There was a problem hiding this comment.
Good point. Those cases now assert their expected results directly (null, "0.00"), and I added (1L, 1, Int.MinValue, 38, 20) for the nonzero scale-up case. Only the 100L/-100L cases are still compared with the BigDecimal form.
| // 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 |
There was a problem hiding this 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.
There was a problem hiding this comment.
Agreed. Since numDigitsInIntegralPart returns Int until #59334 merges, calling it here now would bring the overflow back. Whichever of the two PRs merges second will switch this to numDigitsInIntegralPart(dv.underlying).
There was a problem hiding this comment.
Following up: I tried switching this to numDigitsInIntegralPart(dv.underlying) after #59334 merged, but calling the companion's private method from the class makes Scala emit it as public, which changes the computed serialVersionUID of Decimal$. UdfSerializationSuite (SPARK-59708) then fails, because that SUID is pinned in UdfSerialization.auditedTransitions. So I reverted to the inline dv.precision.toLong - dv.scale, which keeps Decimal$ unchanged.
| // 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 |
There was a problem hiding this 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.
Co-authored-by: Isaac <[email protected]>
… string to decimal ### What changes were proposed in this pull request? `Decimal.fromString` and `Decimal.fromStringANSI` fast-fail on strings whose number of integral digits exceeds `DecimalType.MAX_PRECISION`, computed as `precision - scale` of the parsed `java.math.BigDecimal`. This PR: - computes that value as a `Long` instead of an `Int`; - skips the check for zero, which fits any decimal type whatever its exponent, and drops a negative scale of zero first so that the result has precision 1 and scale 0 instead of a precision derived from the exponent; - moves the check into one helper shared by `fromString` and `fromStringANSI`. ### Why are the changes needed? For strings with an exponent near `Int.MaxValue`, e.g. `'1e2147483647'` (precision 1, scale -2147483647), `precision - scale` overflows `Int` to a negative number and the fast-fail check is skipped. The subsequent conversion then fails with a raw `java.lang.ArithmeticException` (`BigInteger would overflow supported range`, or `Underflow` on JDK 21+ where `'1e2147483648'` parses), so `try_cast`, legacy `CAST` and ANSI `CAST` all fail the task instead of returning NULL or raising `NUMERIC_OUT_OF_SUPPORTED_RANGE`. The check also counts integral digits for zero, so e.g. `'0e39'` is rejected as out of range although it fits any decimal type, while `'0e37'` returns `0.00`. Strings far below the target scale (e.g. `'1e-2147483647'`) and huge exponents under `spark.sql.legacy.allowNegativeScaleOfDecimal=true` fail later, in `Decimal.changePrecision`; they are fixed separately in SPARK-60119 (#59335). Other `BigDecimal` entry points of `Decimal` are tracked in SPARK-60132. ### Does this PR introduce _any_ user-facing change? Yes. Casting such strings to decimal now returns NULL (`try_cast` / non-ANSI `CAST`) or raises `NUMERIC_OUT_OF_SUPPORTED_RANGE` (ANSI `CAST`), the same as other out-of-range strings such as `'6E+38'`, instead of failing with `java.lang.ArithmeticException`. Zero with a large exponent, e.g. `'0e39'`, now returns `0` (precision 1, scale 0) instead of NULL / `NUMERIC_OUT_OF_SUPPORTED_RANGE`. ### How was this patch tested? New test cases in `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite` and `TryCastSuite` (inherited), covering interpreted and codegen evaluation. They fail without this fix. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) This pull request and its description were written by Isaac. Closes #59334 from viirya/SPARK-60118. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>
… string to decimal ### What changes were proposed in this pull request? `Decimal.fromString` and `Decimal.fromStringANSI` fast-fail on strings whose number of integral digits exceeds `DecimalType.MAX_PRECISION`, computed as `precision - scale` of the parsed `java.math.BigDecimal`. This PR: - computes that value as a `Long` instead of an `Int`; - skips the check for zero, which fits any decimal type whatever its exponent, and drops a negative scale of zero first so that the result has precision 1 and scale 0 instead of a precision derived from the exponent; - moves the check into one helper shared by `fromString` and `fromStringANSI`. ### Why are the changes needed? For strings with an exponent near `Int.MaxValue`, e.g. `'1e2147483647'` (precision 1, scale -2147483647), `precision - scale` overflows `Int` to a negative number and the fast-fail check is skipped. The subsequent conversion then fails with a raw `java.lang.ArithmeticException` (`BigInteger would overflow supported range`, or `Underflow` on JDK 21+ where `'1e2147483648'` parses), so `try_cast`, legacy `CAST` and ANSI `CAST` all fail the task instead of returning NULL or raising `NUMERIC_OUT_OF_SUPPORTED_RANGE`. The check also counts integral digits for zero, so e.g. `'0e39'` is rejected as out of range although it fits any decimal type, while `'0e37'` returns `0.00`. Strings far below the target scale (e.g. `'1e-2147483647'`) and huge exponents under `spark.sql.legacy.allowNegativeScaleOfDecimal=true` fail later, in `Decimal.changePrecision`; they are fixed separately in SPARK-60119 (#59335). Other `BigDecimal` entry points of `Decimal` are tracked in SPARK-60132. ### Does this PR introduce _any_ user-facing change? Yes. Casting such strings to decimal now returns NULL (`try_cast` / non-ANSI `CAST`) or raises `NUMERIC_OUT_OF_SUPPORTED_RANGE` (ANSI `CAST`), the same as other out-of-range strings such as `'6E+38'`, instead of failing with `java.lang.ArithmeticException`. Zero with a large exponent, e.g. `'0e39'`, now returns `0` (precision 1, scale 0) instead of NULL / `NUMERIC_OUT_OF_SUPPORTED_RANGE`. ### How was this patch tested? New test cases in `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite` and `TryCastSuite` (inherited), covering interpreted and codegen evaluation. They fail without this fix. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) This pull request and its description were written by Isaac. Closes #59334 from viirya/SPARK-60118. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]> (cherry picked from commit 79ab768) Signed-off-by: Liang-Chi Hsieh <[email protected]>
… string to decimal ### What changes were proposed in this pull request? `Decimal.fromString` and `Decimal.fromStringANSI` fast-fail on strings whose number of integral digits exceeds `DecimalType.MAX_PRECISION`, computed as `precision - scale` of the parsed `java.math.BigDecimal`. This PR: - computes that value as a `Long` instead of an `Int`; - skips the check for zero, which fits any decimal type whatever its exponent, and drops a negative scale of zero first so that the result has precision 1 and scale 0 instead of a precision derived from the exponent; - moves the check into one helper shared by `fromString` and `fromStringANSI`. ### Why are the changes needed? For strings with an exponent near `Int.MaxValue`, e.g. `'1e2147483647'` (precision 1, scale -2147483647), `precision - scale` overflows `Int` to a negative number and the fast-fail check is skipped. The subsequent conversion then fails with a raw `java.lang.ArithmeticException` (`BigInteger would overflow supported range`, or `Underflow` on JDK 21+ where `'1e2147483648'` parses), so `try_cast`, legacy `CAST` and ANSI `CAST` all fail the task instead of returning NULL or raising `NUMERIC_OUT_OF_SUPPORTED_RANGE`. The check also counts integral digits for zero, so e.g. `'0e39'` is rejected as out of range although it fits any decimal type, while `'0e37'` returns `0.00`. Strings far below the target scale (e.g. `'1e-2147483647'`) and huge exponents under `spark.sql.legacy.allowNegativeScaleOfDecimal=true` fail later, in `Decimal.changePrecision`; they are fixed separately in SPARK-60119 (#59335). Other `BigDecimal` entry points of `Decimal` are tracked in SPARK-60132. ### Does this PR introduce _any_ user-facing change? Yes. Casting such strings to decimal now returns NULL (`try_cast` / non-ANSI `CAST`) or raises `NUMERIC_OUT_OF_SUPPORTED_RANGE` (ANSI `CAST`), the same as other out-of-range strings such as `'6E+38'`, instead of failing with `java.lang.ArithmeticException`. Zero with a large exponent, e.g. `'0e39'`, now returns `0` (precision 1, scale 0) instead of NULL / `NUMERIC_OUT_OF_SUPPORTED_RANGE`. ### How was this patch tested? New test cases in `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite` and `TryCastSuite` (inherited), covering interpreted and codegen evaluation. They fail without this fix. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) This pull request and its description were written by Isaac. Closes #59334 from viirya/SPARK-60118. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]> (cherry picked from commit 79ab768) Signed-off-by: Liang-Chi Hsieh <[email protected]>
… string to decimal ### What changes were proposed in this pull request? `Decimal.fromString` and `Decimal.fromStringANSI` fast-fail on strings whose number of integral digits exceeds `DecimalType.MAX_PRECISION`, computed as `precision - scale` of the parsed `java.math.BigDecimal`. This PR: - computes that value as a `Long` instead of an `Int`; - skips the check for zero, which fits any decimal type whatever its exponent, and drops a negative scale of zero first so that the result has precision 1 and scale 0 instead of a precision derived from the exponent; - moves the check into one helper shared by `fromString` and `fromStringANSI`. ### Why are the changes needed? For strings with an exponent near `Int.MaxValue`, e.g. `'1e2147483647'` (precision 1, scale -2147483647), `precision - scale` overflows `Int` to a negative number and the fast-fail check is skipped. The subsequent conversion then fails with a raw `java.lang.ArithmeticException` (`BigInteger would overflow supported range`, or `Underflow` on JDK 21+ where `'1e2147483648'` parses), so `try_cast`, legacy `CAST` and ANSI `CAST` all fail the task instead of returning NULL or raising `NUMERIC_OUT_OF_SUPPORTED_RANGE`. The check also counts integral digits for zero, so e.g. `'0e39'` is rejected as out of range although it fits any decimal type, while `'0e37'` returns `0.00`. Strings far below the target scale (e.g. `'1e-2147483647'`) and huge exponents under `spark.sql.legacy.allowNegativeScaleOfDecimal=true` fail later, in `Decimal.changePrecision`; they are fixed separately in SPARK-60119 (#59335). Other `BigDecimal` entry points of `Decimal` are tracked in SPARK-60132. ### Does this PR introduce _any_ user-facing change? Yes. Casting such strings to decimal now returns NULL (`try_cast` / non-ANSI `CAST`) or raises `NUMERIC_OUT_OF_SUPPORTED_RANGE` (ANSI `CAST`), the same as other out-of-range strings such as `'6E+38'`, instead of failing with `java.lang.ArithmeticException`. Zero with a large exponent, e.g. `'0e39'`, now returns `0` (precision 1, scale 0) instead of NULL / `NUMERIC_OUT_OF_SUPPORTED_RANGE`. ### How was this patch tested? New test cases in `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite` and `TryCastSuite` (inherited), covering interpreted and codegen evaluation. They fail without this fix. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) This pull request and its description were written by Isaac. Closes #59334 from viirya/SPARK-60118. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]> (cherry picked from commit 79ab768) Signed-off-by: Liang-Chi Hsieh <[email protected]>
… string to decimal ### What changes were proposed in this pull request? `Decimal.fromString` and `Decimal.fromStringANSI` fast-fail on strings whose number of integral digits exceeds `DecimalType.MAX_PRECISION`, computed as `precision - scale` of the parsed `java.math.BigDecimal`. This PR: - computes that value as a `Long` instead of an `Int`; - skips the check for zero, which fits any decimal type whatever its exponent, and drops a negative scale of zero first so that the result has precision 1 and scale 0 instead of a precision derived from the exponent; - moves the check into one helper shared by `fromString` and `fromStringANSI`. ### Why are the changes needed? For strings with an exponent near `Int.MaxValue`, e.g. `'1e2147483647'` (precision 1, scale -2147483647), `precision - scale` overflows `Int` to a negative number and the fast-fail check is skipped. The subsequent conversion then fails with a raw `java.lang.ArithmeticException` (`BigInteger would overflow supported range`, or `Underflow` on JDK 21+ where `'1e2147483648'` parses), so `try_cast`, legacy `CAST` and ANSI `CAST` all fail the task instead of returning NULL or raising `NUMERIC_OUT_OF_SUPPORTED_RANGE`. The check also counts integral digits for zero, so e.g. `'0e39'` is rejected as out of range although it fits any decimal type, while `'0e37'` returns `0.00`. Strings far below the target scale (e.g. `'1e-2147483647'`) and huge exponents under `spark.sql.legacy.allowNegativeScaleOfDecimal=true` fail later, in `Decimal.changePrecision`; they are fixed separately in SPARK-60119 (#59335). Other `BigDecimal` entry points of `Decimal` are tracked in SPARK-60132. ### Does this PR introduce _any_ user-facing change? Yes. Casting such strings to decimal now returns NULL (`try_cast` / non-ANSI `CAST`) or raises `NUMERIC_OUT_OF_SUPPORTED_RANGE` (ANSI `CAST`), the same as other out-of-range strings such as `'6E+38'`, instead of failing with `java.lang.ArithmeticException`. Zero with a large exponent, e.g. `'0e39'`, now returns `0` (precision 1, scale 0) instead of NULL / `NUMERIC_OUT_OF_SUPPORTED_RANGE`. ### How was this patch tested? New test cases in `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite` and `TryCastSuite` (inherited), covering interpreted and codegen evaluation. They fail without this fix. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) This pull request and its description were written by Isaac. Closes #59334 from viirya/SPARK-60118. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]> (cherry picked from commit 79ab768) Signed-off-by: Liang-Chi Hsieh <[email protected]>
… string to decimal ### What changes were proposed in this pull request? `Decimal.fromString` and `Decimal.fromStringANSI` fast-fail on strings whose number of integral digits exceeds `DecimalType.MAX_PRECISION`, computed as `precision - scale` of the parsed `java.math.BigDecimal`. This PR: - computes that value as a `Long` instead of an `Int`; - skips the check for zero, which fits any decimal type whatever its exponent, and drops a negative scale of zero first so that the result has precision 1 and scale 0 instead of a precision derived from the exponent; - moves the check into one helper shared by `fromString` and `fromStringANSI`. ### Why are the changes needed? For strings with an exponent near `Int.MaxValue`, e.g. `'1e2147483647'` (precision 1, scale -2147483647), `precision - scale` overflows `Int` to a negative number and the fast-fail check is skipped. The subsequent conversion then fails with a raw `java.lang.ArithmeticException` (`BigInteger would overflow supported range`, or `Underflow` on JDK 21+ where `'1e2147483648'` parses), so `try_cast`, legacy `CAST` and ANSI `CAST` all fail the task instead of returning NULL or raising `NUMERIC_OUT_OF_SUPPORTED_RANGE`. The check also counts integral digits for zero, so e.g. `'0e39'` is rejected as out of range although it fits any decimal type, while `'0e37'` returns `0.00`. Strings far below the target scale (e.g. `'1e-2147483647'`) and huge exponents under `spark.sql.legacy.allowNegativeScaleOfDecimal=true` fail later, in `Decimal.changePrecision`; they are fixed separately in SPARK-60119 (#59335). Other `BigDecimal` entry points of `Decimal` are tracked in SPARK-60132. ### Does this PR introduce _any_ user-facing change? Yes. Casting such strings to decimal now returns NULL (`try_cast` / non-ANSI `CAST`) or raises `NUMERIC_OUT_OF_SUPPORTED_RANGE` (ANSI `CAST`), the same as other out-of-range strings such as `'6E+38'`, instead of failing with `java.lang.ArithmeticException`. Zero with a large exponent, e.g. `'0e39'`, now returns `0` (precision 1, scale 0) instead of NULL / `NUMERIC_OUT_OF_SUPPORTED_RANGE`. ### How was this patch tested? New test cases in `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite` and `TryCastSuite` (inherited), covering interpreted and codegen evaluation. They fail without this fix. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) This pull request and its description were written by Isaac. Closes #59334 from viirya/SPARK-60118. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]> (cherry picked from commit 79ab768) Signed-off-by: Liang-Chi Hsieh <[email protected]>
… string to decimal ### What changes were proposed in this pull request? `Decimal.fromString` and `Decimal.fromStringANSI` fast-fail on strings whose number of integral digits exceeds `DecimalType.MAX_PRECISION`, computed as `precision - scale` of the parsed `java.math.BigDecimal`. This PR: - computes that value as a `Long` instead of an `Int`; - skips the check for zero, which fits any decimal type whatever its exponent, and drops a negative scale of zero first so that the result has precision 1 and scale 0 instead of a precision derived from the exponent; - moves the check into one helper shared by `fromString` and `fromStringANSI`. ### Why are the changes needed? For strings with an exponent near `Int.MaxValue`, e.g. `'1e2147483647'` (precision 1, scale -2147483647), `precision - scale` overflows `Int` to a negative number and the fast-fail check is skipped. The subsequent conversion then fails with a raw `java.lang.ArithmeticException` (`BigInteger would overflow supported range`, or `Underflow` on JDK 21+ where `'1e2147483648'` parses), so `try_cast`, legacy `CAST` and ANSI `CAST` all fail the task instead of returning NULL or raising `NUMERIC_OUT_OF_SUPPORTED_RANGE`. The check also counts integral digits for zero, so e.g. `'0e39'` is rejected as out of range although it fits any decimal type, while `'0e37'` returns `0.00`. Strings far below the target scale (e.g. `'1e-2147483647'`) and huge exponents under `spark.sql.legacy.allowNegativeScaleOfDecimal=true` fail later, in `Decimal.changePrecision`; they are fixed separately in SPARK-60119 (apache#59335). Other `BigDecimal` entry points of `Decimal` are tracked in SPARK-60132. ### Does this PR introduce _any_ user-facing change? Yes. Casting such strings to decimal now returns NULL (`try_cast` / non-ANSI `CAST`) or raises `NUMERIC_OUT_OF_SUPPORTED_RANGE` (ANSI `CAST`), the same as other out-of-range strings such as `'6E+38'`, instead of failing with `java.lang.ArithmeticException`. Zero with a large exponent, e.g. `'0e39'`, now returns `0` (precision 1, scale 0) instead of NULL / `NUMERIC_OUT_OF_SUPPORTED_RANGE`. ### How was this patch tested? New test cases in `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite` and `TryCastSuite` (inherited), covering interpreted and codegen evaluation. They fail without this fix. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) This pull request and its description were written by Isaac. Closes apache#59334 from viirya/SPARK-60118. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>
Calling the companion's private numDigitsInIntegralPart from the class makes it public in bytecode, which changes the computed serialVersionUID of Decimal$ pinned in UdfSerialization.auditedTransitions. Co-authored-by: Isaac <[email protected]>
… result is known from the magnitude
### What changes were proposed in this pull request?
In the BigDecimal branch of `Decimal.changePrecision`, when the scale change exceeds `DecimalType.MAX_PRECISION`, the value's number of integral digits (`precision - scale`, computed in Long) is checked before calling `setScale`:
- If the value is nonzero and has more integral digits than the target type allows, return `false` (overflow) directly. Rounding cannot bring it back into range.
- If the value is below 0.01 ulp of the target scale, the result depends only on the sign and the rounding mode, so a same-sign value of 0.01 ulp is rescaled instead. This gives the same result for every rounding mode.
Otherwise the remaining scale change is bounded by the number of digits of the value, so `setScale` is unchanged. The extra check costs one Int comparison when the scale change is small, which is the common case.
The Long branch of `changePrecision` now computes the scale change in Long. With negative scales allowed it can exceed the Int range and wrap, so `POW_10(diff)` threw `ArrayIndexOutOfBoundsException`, e.g. for a compact `1.00` changed to `DECIMAL(1, -2147483647)`. It now takes the existing rounding path or switches to BigDecimal.
`NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` now renders a decimal whose scale is below -1000 (only possible under `spark.sql.legacy.allowNegativeScaleOfDecimal`) with `toString` instead of `toPlainString`, since the plain form of e.g. `1E+2147483647` does not fit in a String. Other values are rendered as before. `QueryExecutionErrors.cannotChangeDecimalPrecisionError`, an identical copy, is removed and `CastUtils` calls `DataTypeErrors.cannotChangeDecimalPrecisionError` directly.
### Why are the changes needed?
`setScale` takes time that grows with the scale change, and throws once 10^change exceeds BigInteger's range or the change exceeds the Int range:
```sql
SELECT CAST('1e-2147483647' AS DECIMAL(10,2));
-- java.lang.ArithmeticException: BigInteger would overflow supported range (LEGACY, ANSI and TRY)
SELECT CAST('1e-100000000' AS DECIMAL(10,2));
-- 0.00, after ~40s
SET spark.sql.legacy.allowNegativeScaleOfDecimal=true;
SELECT CAST('1e2147483647' AS DECIMAL(10,2));
-- java.lang.ArithmeticException: Underflow
```
This is the small-side counterpart of the fast-fail added in SPARK-35841/SPARK-37451. With negative scales allowed, `Decimal.fromString` intentionally skips that fast-fail, so huge values reach `changePrecision` too.
### Does this PR introduce _any_ user-facing change?
Yes.
- Casting a string whose value is far below the target scale's ulp now returns the rounded value (e.g. 0.00) instead of throwing `ArithmeticException`, and returns quickly.
- With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, casting a string with a huge exponent returns NULL (non-ANSI, try_cast) or throws `NUMERIC_VALUE_OUT_OF_RANGE` (ANSI) instead of `ArithmeticException: Underflow`. In that error, a value whose scale is below -1000 (e.g. `1E+2147483647`) is shown in scientific notation, since its plain form is too long to build. Other values are shown as before.
- With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, changing a decimal stored as a long (precision up to 18, e.g. after an `UnsafeRow` round trip) by a scale change beyond the Int range no longer throws `ArrayIndexOutOfBoundsException`, and gives the same result as the same value stored as a BigDecimal.
No input that previously produced a value changes its result.
### How was this patch tested?
New tests in `DecimalSuite`, `CastSuiteBase` (so they run in `CastWithAnsiOffSuite`, `CastWithAnsiOnSuite` and `TryCastSuite`), `CastWithAnsiOffSuite` and `CastWithAnsiOnSuite`, covering interpreted and codegen evaluation. All of them fail without the fix. The ANSI test also checks the `value` parameter of the error, in scientific notation for `1e2147483647` and in plain notation for `1e40`. `DecimalSuite` also checks that compact (long) inputs, including scale changes beyond the Int range, round like the same values in BigDecimal form, and that values around the bounds of both shortcuts, with short and 81-digit mantissas, under every supported rounding mode and with and without negative scales, round exactly like `BigDecimal.setScale`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
This pull request and its description were written by Isaac.
Closes #59335 from viirya/SPARK-60119.
Authored-by: Liang-Chi Hsieh <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 9c3a51f)
Signed-off-by: Dongjoon Hyun <[email protected]>
|
I landed this to master/4.x . Please backport this if you want, @viirya . |
|
Thank you @dongjoon-hyun @HyukjinKwon ! |
…n the result is known from the magnitude ### What changes were proposed in this pull request? Backport of #59335 to branch-4.3. The cherry-pick applied cleanly. In the BigDecimal branch of `Decimal.changePrecision`, when the scale change exceeds `DecimalType.MAX_PRECISION`, the value's number of integral digits (`precision - scale`, computed in Long) is checked before calling `setScale`: - If the value is nonzero and has more integral digits than the target type allows, return `false` (overflow) directly. Rounding cannot bring it back into range. - If the value is below 0.01 ulp of the target scale, the result depends only on the sign and the rounding mode, so a same-sign value of 0.01 ulp is rescaled instead. This gives the same result for every rounding mode. The Long branch of `changePrecision` now computes the scale change in Long, so it no longer wraps and throws `ArrayIndexOutOfBoundsException` from `POW_10(diff)` when negative scales are allowed. `NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` now renders a decimal whose scale is below -1000 (only possible under `spark.sql.legacy.allowNegativeScaleOfDecimal`) with `toString` instead of `toPlainString`, since the plain form of e.g. `1E+2147483647` does not fit in a String. Other values are rendered as before. `QueryExecutionErrors.cannotChangeDecimalPrecisionError`, an identical copy, is removed and `CastUtils` calls `DataTypeErrors.cannotChangeDecimalPrecisionError` directly. ### Why are the changes needed? `setScale` takes time that grows with the scale change, and throws once 10^change exceeds BigInteger's range or the change exceeds the Int range: ```sql SELECT CAST('1e-2147483647' AS DECIMAL(10,2)); -- java.lang.ArithmeticException: BigInteger would overflow supported range (LEGACY, ANSI and TRY) SELECT CAST('1e-100000000' AS DECIMAL(10,2)); -- 0.00, after ~40s SET spark.sql.legacy.allowNegativeScaleOfDecimal=true; SELECT CAST('1e2147483647' AS DECIMAL(10,2)); -- java.lang.ArithmeticException: Underflow ``` This branch already has the related fix SPARK-60118, so backporting this keeps decimal casts consistent with master. ### Does this PR introduce _any_ user-facing change? Yes, the same as #59335: - Casting a string whose value is far below the target scale's ulp now returns the rounded value (e.g. 0.00) instead of throwing `ArithmeticException`, and returns quickly. - With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, casting a string with a huge exponent returns NULL (non-ANSI, try_cast) or throws `NUMERIC_VALUE_OUT_OF_RANGE` (ANSI) instead of `ArithmeticException: Underflow`, and a decimal stored as a long no longer throws `ArrayIndexOutOfBoundsException` for a scale change beyond the Int range. In that error, a value whose scale is below -1000 is shown in scientific notation. No input that previously produced a value changes its result. ### How was this patch tested? The tests from #59335 in `DecimalSuite`, `CastSuiteBase`, `CastWithAnsiOnSuite` and `CastWithAnsiOffSuite` (also run through `TryCastSuite`). On this branch with JDK 17, {'4.3': '512 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`, plus `UdfSerializationSuite` (16 tests)', '4.2': '410 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`', '4.1': '403 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`', '4.0': '354 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`'} pass, and scalastyle (`sql-api`, `catalyst`, `catalyst` tests) and the `sql/api` scalafmt check pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) Closes #59512 from viirya/SPARK-60119-4.3. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>
…n the result is known from the magnitude ### What changes were proposed in this pull request? Backport of #59335 to branch-4.2. The cherry-pick conflicted only on `CastUtils.java`, which does not exist on this branch: `Cast` calls `QueryExecutionErrors.cannotChangeDecimalPrecisionError` directly in both the interpreted and generated code. So instead of removing that method and switching `CastUtils`, this backport keeps it and makes it delegate to `DataTypeErrors.cannotChangeDecimalPrecisionError`, which gives the same error message. The rest of the change is identical to #59335. In the BigDecimal branch of `Decimal.changePrecision`, when the scale change exceeds `DecimalType.MAX_PRECISION`, the value's number of integral digits (`precision - scale`, computed in Long) is checked before calling `setScale`: - If the value is nonzero and has more integral digits than the target type allows, return `false` (overflow) directly. Rounding cannot bring it back into range. - If the value is below 0.01 ulp of the target scale, the result depends only on the sign and the rounding mode, so a same-sign value of 0.01 ulp is rescaled instead. This gives the same result for every rounding mode. The Long branch of `changePrecision` now computes the scale change in Long, so it no longer wraps and throws `ArrayIndexOutOfBoundsException` from `POW_10(diff)` when negative scales are allowed. `NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` now renders a decimal whose scale is below -1000 (only possible under `spark.sql.legacy.allowNegativeScaleOfDecimal`) with `toString` instead of `toPlainString`, since the plain form of e.g. `1E+2147483647` does not fit in a String. Other values are rendered as before. `QueryExecutionErrors.cannotChangeDecimalPrecisionError`, an identical copy, now delegates to `DataTypeErrors.cannotChangeDecimalPrecisionError`. ### Why are the changes needed? `setScale` takes time that grows with the scale change, and throws once 10^change exceeds BigInteger's range or the change exceeds the Int range: ```sql SELECT CAST('1e-2147483647' AS DECIMAL(10,2)); -- java.lang.ArithmeticException: BigInteger would overflow supported range (LEGACY, ANSI and TRY) SELECT CAST('1e-100000000' AS DECIMAL(10,2)); -- 0.00, after ~40s SET spark.sql.legacy.allowNegativeScaleOfDecimal=true; SELECT CAST('1e2147483647' AS DECIMAL(10,2)); -- java.lang.ArithmeticException: Underflow ``` This branch already has the related fix SPARK-60118, so backporting this keeps decimal casts consistent with master. ### Does this PR introduce _any_ user-facing change? Yes, the same as #59335: - Casting a string whose value is far below the target scale's ulp now returns the rounded value (e.g. 0.00) instead of throwing `ArithmeticException`, and returns quickly. - With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, casting a string with a huge exponent returns NULL (non-ANSI, try_cast) or throws `NUMERIC_VALUE_OUT_OF_RANGE` (ANSI) instead of `ArithmeticException: Underflow`, and a decimal stored as a long no longer throws `ArrayIndexOutOfBoundsException` for a scale change beyond the Int range. In that error, a value whose scale is below -1000 is shown in scientific notation. No input that previously produced a value changes its result. ### How was this patch tested? The tests from #59335 in `DecimalSuite`, `CastSuiteBase`, `CastWithAnsiOnSuite` and `CastWithAnsiOffSuite` (also run through `TryCastSuite`). On this branch with JDK 17, {'4.3': '512 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`, plus `UdfSerializationSuite` (16 tests)', '4.2': '410 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`', '4.1': '403 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`', '4.0': '354 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`'} pass, and scalastyle (`sql-api`, `catalyst`, `catalyst` tests) and the `sql/api` scalafmt check pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) Closes #59513 from viirya/SPARK-60119-4.2. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>
…n the result is known from the magnitude ### What changes were proposed in this pull request? Backport of #59335 to branch-4.1. The cherry-pick conflicted only on `CastUtils.java`, which does not exist on this branch: `Cast` calls `QueryExecutionErrors.cannotChangeDecimalPrecisionError` directly in both the interpreted and generated code. So instead of removing that method and switching `CastUtils`, this backport keeps it and makes it delegate to `DataTypeErrors.cannotChangeDecimalPrecisionError`, which gives the same error message. The rest of the change is identical to #59335. In the BigDecimal branch of `Decimal.changePrecision`, when the scale change exceeds `DecimalType.MAX_PRECISION`, the value's number of integral digits (`precision - scale`, computed in Long) is checked before calling `setScale`: - If the value is nonzero and has more integral digits than the target type allows, return `false` (overflow) directly. Rounding cannot bring it back into range. - If the value is below 0.01 ulp of the target scale, the result depends only on the sign and the rounding mode, so a same-sign value of 0.01 ulp is rescaled instead. This gives the same result for every rounding mode. The Long branch of `changePrecision` now computes the scale change in Long, so it no longer wraps and throws `ArrayIndexOutOfBoundsException` from `POW_10(diff)` when negative scales are allowed. `NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` now renders a decimal whose scale is below -1000 (only possible under `spark.sql.legacy.allowNegativeScaleOfDecimal`) with `toString` instead of `toPlainString`, since the plain form of e.g. `1E+2147483647` does not fit in a String. Other values are rendered as before. `QueryExecutionErrors.cannotChangeDecimalPrecisionError`, an identical copy, now delegates to `DataTypeErrors.cannotChangeDecimalPrecisionError`. ### Why are the changes needed? `setScale` takes time that grows with the scale change, and throws once 10^change exceeds BigInteger's range or the change exceeds the Int range: ```sql SELECT CAST('1e-2147483647' AS DECIMAL(10,2)); -- java.lang.ArithmeticException: BigInteger would overflow supported range (LEGACY, ANSI and TRY) SELECT CAST('1e-100000000' AS DECIMAL(10,2)); -- 0.00, after ~40s SET spark.sql.legacy.allowNegativeScaleOfDecimal=true; SELECT CAST('1e2147483647' AS DECIMAL(10,2)); -- java.lang.ArithmeticException: Underflow ``` This branch already has the related fix SPARK-60118, so backporting this keeps decimal casts consistent with master. ### Does this PR introduce _any_ user-facing change? Yes, the same as #59335: - Casting a string whose value is far below the target scale's ulp now returns the rounded value (e.g. 0.00) instead of throwing `ArithmeticException`, and returns quickly. - With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, casting a string with a huge exponent returns NULL (non-ANSI, try_cast) or throws `NUMERIC_VALUE_OUT_OF_RANGE` (ANSI) instead of `ArithmeticException: Underflow`, and a decimal stored as a long no longer throws `ArrayIndexOutOfBoundsException` for a scale change beyond the Int range. In that error, a value whose scale is below -1000 is shown in scientific notation. No input that previously produced a value changes its result. ### How was this patch tested? The tests from #59335 in `DecimalSuite`, `CastSuiteBase`, `CastWithAnsiOnSuite` and `CastWithAnsiOffSuite` (also run through `TryCastSuite`). On this branch with JDK 17, {'4.3': '512 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`, plus `UdfSerializationSuite` (16 tests)', '4.2': '410 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`', '4.1': '403 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`', '4.0': '354 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`'} pass, and scalastyle (`sql-api`, `catalyst`, `catalyst` tests) and the `sql/api` scalafmt check pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) Closes #59514 from viirya/SPARK-60119-4.1. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>
…n the result is known from the magnitude ### What changes were proposed in this pull request? Backport of #59335 to branch-4.0. The cherry-pick conflicted only on `CastUtils.java`, which does not exist on this branch: `Cast` calls `QueryExecutionErrors.cannotChangeDecimalPrecisionError` directly in both the interpreted and generated code. So instead of removing that method and switching `CastUtils`, this backport keeps it and makes it delegate to `DataTypeErrors.cannotChangeDecimalPrecisionError`, which gives the same error message. The rest of the change is identical to #59335. In the BigDecimal branch of `Decimal.changePrecision`, when the scale change exceeds `DecimalType.MAX_PRECISION`, the value's number of integral digits (`precision - scale`, computed in Long) is checked before calling `setScale`: - If the value is nonzero and has more integral digits than the target type allows, return `false` (overflow) directly. Rounding cannot bring it back into range. - If the value is below 0.01 ulp of the target scale, the result depends only on the sign and the rounding mode, so a same-sign value of 0.01 ulp is rescaled instead. This gives the same result for every rounding mode. The Long branch of `changePrecision` now computes the scale change in Long, so it no longer wraps and throws `ArrayIndexOutOfBoundsException` from `POW_10(diff)` when negative scales are allowed. `NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTION` now renders a decimal whose scale is below -1000 (only possible under `spark.sql.legacy.allowNegativeScaleOfDecimal`) with `toString` instead of `toPlainString`, since the plain form of e.g. `1E+2147483647` does not fit in a String. Other values are rendered as before. `QueryExecutionErrors.cannotChangeDecimalPrecisionError`, an identical copy, now delegates to `DataTypeErrors.cannotChangeDecimalPrecisionError`. ### Why are the changes needed? `setScale` takes time that grows with the scale change, and throws once 10^change exceeds BigInteger's range or the change exceeds the Int range: ```sql SELECT CAST('1e-2147483647' AS DECIMAL(10,2)); -- java.lang.ArithmeticException: BigInteger would overflow supported range (LEGACY, ANSI and TRY) SELECT CAST('1e-100000000' AS DECIMAL(10,2)); -- 0.00, after ~40s SET spark.sql.legacy.allowNegativeScaleOfDecimal=true; SELECT CAST('1e2147483647' AS DECIMAL(10,2)); -- java.lang.ArithmeticException: Underflow ``` This branch already has the related fix SPARK-60118, so backporting this keeps decimal casts consistent with master. ### Does this PR introduce _any_ user-facing change? Yes, the same as #59335: - Casting a string whose value is far below the target scale's ulp now returns the rounded value (e.g. 0.00) instead of throwing `ArithmeticException`, and returns quickly. - With `spark.sql.legacy.allowNegativeScaleOfDecimal=true`, casting a string with a huge exponent returns NULL (non-ANSI, try_cast) or throws `NUMERIC_VALUE_OUT_OF_RANGE` (ANSI) instead of `ArithmeticException: Underflow`, and a decimal stored as a long no longer throws `ArrayIndexOutOfBoundsException` for a scale change beyond the Int range. In that error, a value whose scale is below -1000 is shown in scientific notation. No input that previously produced a value changes its result. ### How was this patch tested? The tests from #59335 in `DecimalSuite`, `CastSuiteBase`, `CastWithAnsiOnSuite` and `CastWithAnsiOffSuite` (also run through `TryCastSuite`). On this branch with JDK 17, {'4.3': '512 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`, plus `UdfSerializationSuite` (16 tests)', '4.2': '410 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`', '4.1': '403 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`', '4.0': '354 tests across `DecimalSuite`, `CastWithAnsiOnSuite`, `CastWithAnsiOffSuite`, `TryCastSuite`, `DecimalPrecisionSuite`, `DecimalExpressionSuite` and `MathExpressionsSuite`'} pass, and scalastyle (`sql-api`, `catalyst`, `catalyst` tests) and the `sql/api` scalafmt check pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) Closes #59515 from viirya/SPARK-60119-4.0. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>
What changes were proposed in this pull request?
In the BigDecimal branch of
Decimal.changePrecision, when the scale change exceedsDecimalType.MAX_PRECISION, the value's number of integral digits (precision - scale, computed in Long) is checked before callingsetScale:false(overflow) directly. Rounding cannot bring it back into range.Otherwise the remaining scale change is bounded by the number of digits of the value, so
setScaleis unchanged. The extra check costs one Int comparison when the scale change is small, which is the common case.The Long branch of
changePrecisionnow computes the scale change in Long. With negative scales allowed it can exceed the Int range and wrap, soPOW_10(diff)threwArrayIndexOutOfBoundsException, e.g. for a compact1.00changed toDECIMAL(1, -2147483647). It now takes the existing rounding path or switches to BigDecimal.NUMERIC_VALUE_OUT_OF_RANGE.WITH_SUGGESTIONnow renders a decimal whose scale is below -1000 (only possible underspark.sql.legacy.allowNegativeScaleOfDecimal) withtoStringinstead oftoPlainString, since the plain form of e.g.1E+2147483647does not fit in a String. Other values are rendered as before.QueryExecutionErrors.cannotChangeDecimalPrecisionError, an identical copy, is removed andCastUtilscallsDataTypeErrors.cannotChangeDecimalPrecisionErrordirectly.Why are the changes needed?
setScaletakes time that grows with the scale change, and throws once 10^change exceeds BigInteger's range or the change exceeds the Int range:This is the small-side counterpart of the fast-fail added in SPARK-35841/SPARK-37451. With negative scales allowed,
Decimal.fromStringintentionally skips that fast-fail, so huge values reachchangePrecisiontoo.Does this PR introduce any user-facing change?
Yes.
ArithmeticException, and returns quickly.spark.sql.legacy.allowNegativeScaleOfDecimal=true, casting a string with a huge exponent returns NULL (non-ANSI, try_cast) or throwsNUMERIC_VALUE_OUT_OF_RANGE(ANSI) instead ofArithmeticException: Underflow. In that error, a value whose scale is below -1000 (e.g.1E+2147483647) is shown in scientific notation, since its plain form is too long to build. Other values are shown as before.spark.sql.legacy.allowNegativeScaleOfDecimal=true, changing a decimal stored as a long (precision up to 18, e.g. after anUnsafeRowround trip) by a scale change beyond the Int range no longer throwsArrayIndexOutOfBoundsException, and gives the same result as the same value stored as a BigDecimal.No input that previously produced a value changes its result.
How was this patch tested?
New tests in
DecimalSuite,CastSuiteBase(so they run inCastWithAnsiOffSuite,CastWithAnsiOnSuiteandTryCastSuite),CastWithAnsiOffSuiteandCastWithAnsiOnSuite, covering interpreted and codegen evaluation. All of them fail without the fix. The ANSI test also checks thevalueparameter of the error, in scientific notation for1e2147483647and in plain notation for1e40.DecimalSuitealso checks that compact (long) inputs, including scale changes beyond the Int range, round like the same values in BigDecimal form, and that values around the bounds of both shortcuts, with short and 81-digit mantissas, under every supported rounding mode and with and without negative scales, round exactly likeBigDecimal.setScale.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
This pull request and its description were written by Isaac.