Repository navigation
Conversation
… string to decimal Compute the number of integral digits in `Decimal.fromString`/`fromStringANSI` as a Long, so that strings with an exponent near Int.MaxValue (e.g. '1e2147483647') are rejected by the fast-fail check instead of failing with a raw java.lang.ArithmeticException. Co-authored-by: Isaac <no-reply@databricks.com>
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for the fix, @viirya. Computing the number of integral digits in Long looks right to me. I left 6 inline comments. In summary:
-
Large negative exponents still fail with a raw
ArithmeticException'1e-2147483647'skips the fast-fail, andchangePrecisionthrows insetScalefortry_cast, non-ANSICASTand ANSICAST. -
The legacy
spark.sql.legacy.allowNegativeScaleOfDecimal=truepath is not coveredThe fast-fail is skipped under the legacy conf, so
'1e2147483646'still fails withArithmeticException: Underflow. -
'0e2147483647'changes from0.00toNULLor an errorZero fits any decimal type. Skipping the fast-fail for zero would keep
0.00and also fix'0e39'. -
No fast-fail for large negative exponents
try_cast('1e-10000000' AS DECIMAL(10,2))takes about 1.9 seconds per row. This was already the case before this PR. -
The same
precision - scaleInt expression inDecimal.setOther
BigDecimalentry points, such asDecimal(String)and the CSV/JSON readers, don't go through this bound. This can be a separate JIRA. -
Test coverage
The tests don't cover the negative-exponent case or the legacy conf.
I checked these behaviors with spark-shell on a current master build. The code paths for 1, 2 and 4 are the same with this PR.
| bigDecimal.precision - bigDecimal.scale | ||
| // Computed in Long since `precision - scale` can overflow Int, e.g. for "1e2147483647". | ||
| private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Long = | ||
| bigDecimal.precision.toLong - bigDecimal.scale |
There was a problem hiding this comment.
This covers large positive exponents, but the mirror case with a large negative exponent still fails with a raw ArithmeticException. For '1e-2147483647', precision - scale is 1 - 2147483647, so the fast-fail is not taken. Decimal.set then sets _precision = _scale = 2147483647, and changePrecision calls dv.setScale(2, ...), which throws.
On the current master build (the path is the same with this PR):
try_cast('1e-2147483647' AS DECIMAL(10,2)) => ArithmeticException: BigInteger would overflow supported range
CAST('1e-2147483647' AS DECIMAL(10,2)), ansi.enabled=false => same
CAST('1e-2147483647' AS DECIMAL(10,2)), ansi.enabled=true => same
This value rounds to 0.00, the same as '1e-100'. Since it is the same failure as the one in the PR description, could you handle this case in this PR too?
There was a problem hiding this comment.
This fails in changePrecision rather than in the fast-fail, so it's handled in #59335 (SPARK-60119), which bounds the small side there.
| private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Int = | ||
| bigDecimal.precision - bigDecimal.scale | ||
| // Computed in Long since `precision - scale` can overflow Int, e.g. for "1e2147483647". | ||
| private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Long = |
There was a problem hiding this comment.
The fast-fail is skipped when spark.sql.legacy.allowNegativeScaleOfDecimal is true, so this fix does not apply under the legacy conf. Decimal.set keeps the negative scale, and changePrecision fails in setScale:
spark.sql.legacy.allowNegativeScaleOfDecimal=true
try_cast('1e2147483646' AS DECIMAL(10,2)) => ArithmeticException: Underflow
This was already the case before this PR. Should it be covered here, or should the PR description say that the legacy conf is out of scope?
There was a problem hiding this comment.
Covered in #59335 (SPARK-60119), with a cast-level test under the legacy conf. I noted it in the PR description.
| checkExceptionInExpression[ArithmeticException]( | ||
| cast("6E+37", DecimalType(38, 1)), | ||
| "cannot be represented as Decimal(38, 1)") | ||
| Seq("1e2147483647", "0e2147483647", "1.5e2147483647", "1e2147483648").foreach { s => |
There was a problem hiding this comment.
'0e2147483647' is exactly zero, and it fits DECIMAL(10,2). On master it returns 0.00. With this PR it becomes NULL (non-ANSI) or NUMERIC_OUT_OF_SUPPORTED_RANGE (ANSI), and these tests lock that in.
It is consistent with '0e39' and '0e100', as the PR description says, but those look like the same false positive: the integral-digit check counts precision - scale even for zero.
spark.sql.ansi.enabled=false, master
CAST(s AS DECIMAL(10,2)) for '0e2147483647', '0e39', '0e37' => 0.00, null, 0.00
How about skipping the fast-fail for zero, e.g. bigDecimal.signum != 0 && numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION? Then '0e2147483647' keeps returning 0.00, and '0e39' is fixed too.
There was a problem hiding this comment.
Good point. Done: the fast-fail skips zero now, and the tests check that '0e39', '0e2147483647' and '0.0e2147483648' cast to 0.00.
|
|
||
| private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Int = | ||
| bigDecimal.precision - bigDecimal.scale | ||
| // Computed in Long since `precision - scale` can overflow Int, e.g. for "1e2147483647". |
There was a problem hiding this comment.
The fast-fail exists because converting a huge value is slow (see the Decimal("6.0790316E+25569151") comment below), but it only bounds large positive exponents. A large negative exponent goes through the same slow path in setScale:
try_cast('1e-10000000' AS DECIMAL(10,2)), 1 row => 0.00 (1867 ms)
try_cast('1e-10000000' AS DECIMAL(10,2)), 5 rows => 0.00 (8779 ms)
This was already the case before this PR, so it can be a separate JIRA. A bound on the negative side could fix this and the '1e-2147483647' exception together. For example, a value below 10^-(MAX_SCALE + 1) rounds to zero for any target scale.
There was a problem hiding this comment.
Same fix as above in #59335; '1e-100000000' returns 0.00 without the setScale cost.
| private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Int = | ||
| bigDecimal.precision - bigDecimal.scale | ||
| // Computed in Long since `precision - scale` can overflow Int, e.g. for "1e2147483647". | ||
| private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Long = |
There was a problem hiding this comment.
Decimal.set(BigDecimal) has the same Int expression, this._precision = decimal.precision - decimal.scale (L152). Other entry points that build a Decimal from a BigDecimal don't go through this bound either. For example, Decimal(String), and the CSV/JSON readers with a decimal schema (UnivocityParser L226, JacksonParser L441) call Decimal(bd, precision, scale), which goes straight to setScale.
Would it make sense to move the bound (in Long) into Decimal.set or a shared helper, so these paths don't depend on the string-cast fast path? This can be a separate JIRA.
| } | ||
| } | ||
|
|
||
| test("UTF8String to Decimal with exponent near Int.MaxValue") { |
There was a problem hiding this comment.
Depending on how you handle the comments in Decimal.scala, could you add these cases?
- The negative-exponent case,
'1e-2147483647'. - A cast case with
spark.sql.legacy.allowNegativeScaleOfDecimal=true, like thewithSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key -> "true")block in the SPARK-37451 test above. Under the legacy conf,fromStringitself succeeds and the exception comes fromchangePrecision, so it needs a cast-level check.
Both still fail with a raw ArithmeticException, and the current tests don't catch them.
There was a problem hiding this comment.
The negative-exponent and legacy-conf cases are tested in #59335. I added zero cases here.
Zero fits any decimal type whatever its exponent, so strings like '0e39' should not be rejected as out of range. Co-authored-by: Isaac <no-reply@databricks.com>
|
Thanks for the thorough review, @dongjoon-hyun.
|
What changes were proposed in this pull request?
Decimal.fromStringandDecimal.fromStringANSIfast-fail on strings whose number of integral digits exceedsDecimalType.MAX_PRECISION, computed asprecision - scaleof the parsedjava.math.BigDecimal. This PR:Longinstead of anInt;Why are the changes needed?
For strings with an exponent near
Int.MaxValue, e.g.'1e2147483647'(precision 1, scale -2147483647),precision - scaleoverflowsIntto a negative number and the fast-fail check is skipped. The subsequent conversion then fails with a rawjava.lang.ArithmeticException(BigInteger would overflow supported rangeorUnderflow), sotry_cast, legacyCASTand ANSICASTall fail the task instead of returning NULL or raisingNUMERIC_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'returns0.00.Strings far below the target scale (e.g.
'1e-2147483647') and huge exponents underspark.sql.legacy.allowNegativeScaleOfDecimal=truefail later, inDecimal.changePrecision; they are fixed separately in SPARK-60119 (#59335). OtherBigDecimalentry points ofDecimalare tracked in SPARK-60132.Does this PR introduce any user-facing change?
Yes. Casting such strings to decimal now returns NULL (
try_cast/ non-ANSICAST) or raisesNUMERIC_OUT_OF_SUPPORTED_RANGE(ANSICAST), the same as other out-of-range strings such as'6E+38', instead of failing withjava.lang.ArithmeticException. Zero with a large exponent, e.g.'0e39', now returns0instead of NULL /NUMERIC_OUT_OF_SUPPORTED_RANGE.How was this patch tested?
New test cases in
DecimalSuite,CastWithAnsiOnSuite,CastWithAnsiOffSuiteandTryCastSuite(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.