[SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal - #59334

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?

This fails in changePrecision rather than in the fast-fail, so it's handled in #59335 (SPARK-60119), which bounds the small side there.

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?

Covered in #59335 (SPARK-60119), with a cast-level test under the legacy conf. I noted it in the PR description.

'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.

Good point. Done: the fast-fail skips zero now, and the tests check that '0e39', '0e2147483647' and '0.0e2147483648' cast to 0.00.

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.

Same fix as above in #59335; '1e-100000000' returns 0.00 without the setScale cost.

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.

Filed SPARK-60132 for this.

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 the withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key -> "true") block in the SPARK-37451 test above. Under the legacy conf, fromString itself succeeds and the exception comes from changePrecision, so it needs a cast-level check.

Both still fail with a raw ArithmeticException, and the current tests don't catch them.

The negative-exponent and legacy-conf cases are tested in #59335. I added zero cases here.

Blocking (P1): "1e2147483648" (and "0.0e2147483648" at line 461) only parse on newer JDKs. On JDK 17, which is Spark's java.version and the default CI JDK, new java.math.BigDecimal(s) throws NumberFormatException("Exponent overflow.") for an exponent outside Int range. That happens in stringToJavaBigDecimal, before the fast-fail runs. So on JDK 17:

• fromStringANSI("1e2147483648") throws SparkNumberFormatException (CAST_INVALID_INPUT), not SparkArithmeticException / NUMERIC_OUT_OF_SUPPORTED_RANGE.
• "0.0e2147483648" gives null / CAST_INVALID_INPUT instead of zero.

JDK 25 parses both strings (scale -2147483648 and -2147483647). I checked both JDKs with a standalone BigDecimal probe.

This is what fails CI on this commit:

• DecimalSuite: this test
• CastWithAnsiOnSuite: lines 478 and 483, and TryCastSuite, which inherits them
• CastWithAnsiOffSuite: line 565

The "1e2147483648" -> null checks still pass on JDK 17, but only through the NumberFormatException path, not the Long fast-fail they are meant to cover.

Could you drop these two inputs, or replace them with strings that parse on every supported JDK and still overflow Int? For example, "10e2147483647" (precision 2, scale -2147483647) for the non-zero side and "0.0e2147483647" for the zero side.

Verification:

• Regression: On JDK 17, the new Decimal string-conversion test and the cast-string-to-decimal fast-fail tests (ANSI on, ANSI off, and try_cast, interpreted and codegen) pass, where they currently fail on the exponent-2147483648 inputs.
• Behavior: Each non-zero test input parses with java.math.BigDecimal on JDK 17 and yields precision - scale greater than Int.MaxValue, and the same assertions fail if numDigitsInIntegralPart is reverted to Int arithmetic.

Good catch, thanks. My local runs used JDK 21, where new BigDecimal("1e2147483648") parses with scale Int.MinValue; JDK 17 rejects it with NumberFormatException before the fast-fail runs. I replaced '1e2147483648' with '12e2147483647' (precision 2, scale -2147483647) and dropped '0.0e2147483648'; '0e2147483647' still covers the zero case with an Int-overflowing digit count, so '0.0e2147483647' (whose count, 2147483647, fits in Int) wouldn't add coverage. On JDK 17, the updated tests pass, and all four fail again if numDigitsInIntegralPart is reverted to Int arithmetic. I also corrected the PR description, which mentioned the JDK 21-only Underflow failure.

With the zero exemption, a zero with a large exponent goes to Decimal(bigDecimal). Decimal.set then computes this._precision = decimal.precision - decimal.scale (L152), which is the same Int expression this PR moves to Long in numDigitsInIntegralPart:

0e39:         bd.precision=1 bd.scale=-39         -> Decimal._precision=40
-0E+100:      bd.precision=1 bd.scale=-100        -> Decimal._precision=101
0e2147483647: bd.precision=1 bd.scale=-2147483647 -> Decimal._precision=-2147483648

So Decimal.fromString("0e39") used to return null and now returns a Decimal whose precision is 40, above MAX_PRECISION. The '0e2147483647' case was already like this on master. Cast is not affected because changePrecision rescales the value, but fromString / fromStringANSI are public, and a caller that reads .precision or builds DecimalType(d.precision, d.scale) gets an invalid value.

SPARK-60132 tracks Decimal.set in general. Since this PR is what newly sends these zeros there, how about normalizing zero here? For example, Decimal(bigDecimal.setScale(math.max(bigDecimal.scale, 0))) for signum == 0, which keeps a non-negative scale such as '0.00' and gives precision 1 for '0e39'.

Done. When negative scales are not allowed, a zero's negative scale is dropped before Decimal.set, so the result has precision 1 and scale 0; '0.00' keeps scale 2.

Decimal.equals compares only the value (through compare), so === Decimal(0) passes for any precision and scale. It passes today although fromString("0e39") has precision 40 and fromString("0e2147483647") has precision -2147483648 (see the comment in Decimal.scala).

Could you also assert the precision and scale, e.g. precision within [1, DecimalType.MAX_PRECISION] and scale == 0?

Done. The test now also asserts precision 1 and scale 0 for these zeros, and fails with "40 did not equal 1" without the normalization.

This three-line condition and its three-line comment are a copy of the ones in fromString (L651-656). This PR already had to change both copies the same way for the zero exemption.

How about a shared private predicate, e.g. exceedsMaxPrecision(bigDecimal)? Another option is to fold the zero rule into numDigitsInIntegralPart, since zero has one integral digit. Either way the rule would be defined in one place.

Done. Moved into one private helper, stringBigDecimalToDecimal, which also holds the zero rule.

These zero checks are identical to the ones added in CastWithAnsiOffSuite (L565-567), because the expected 0.00 doesn't depend on ANSI mode.

Could they move to CastSuiteBase? Then they run once for ANSI on, ANSI off and try_cast, and there is only one copy to maintain.

Done. Moved to CastSuiteBase, so they run for ANSI on, ANSI off and try_cast.

nit: The neighboring regression tests use the JIRA prefix (SPARK-37451: ..., SPARK-41554: ...). How about test("SPARK-60118: UTF8String to Decimal with exponent near Int.MaxValue")?

Done.

This reads SqlApiConf.get.allowNegativeScaleOfDecimalEnabled before anything else, so every call pays for a conf lookup. On master and at the previous commit, the && order read the conf only when numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION, that is, only for out-of-range strings.

This is a per-row path: Cast calls the helper per row (L1380/L1385 interpreted, L1900/L1912 codegen), and so does the protobuf uint64-to-decimal converter. The lookup isn't cheap there:

• On executors, a normal task has no existingConf (only SQLExecutionRDD sets one), so SQLConf.get returns new ReadOnlySQLConf(TaskContext.get()). That builds two ConfigReaders with their maps and providers, then looks the key up in the task's local properties, once per row.
• On the driver (for example ConvertToLocalRelation or constant folding), SQLConf.get calls SparkContext.getActive, which takes SPARK_CONTEXT_CONSTRUCTOR_LOCK on every call.

I traced this in the code and didn't benchmark it. This path has been tuned for speed before, though (the s.toString.trim note in stringToJavaBigDecimal above, and the fast-fail itself).

How about reading the conf only on the two rare paths that need it? For example:

  private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
    if (bigDecimal.signum == 0) {
      // Zero fits any decimal type whatever its exponent, e.g. "0e39". Drop a negative scale so
      // that the precision does not grow with the exponent.
      if (bigDecimal.scale < 0 && !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
        Decimal(bigDecimal.setScale(0))
      } else {
        Decimal(bigDecimal)
      }
    } else if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
        !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
      null
    } else {
      Decimal(bigDecimal)
    }
  }

This gives the same results as the current helper for every input, and an in-range string never touches the conf.

Good catch, that was a regression from the previous commit. Done with your suggested structure; an in-range string no longer reads the conf.

The precision that grows with the exponent comes from Decimal.set(BigDecimal) (L150-152, this._precision = decimal.precision - decimal.scale), so the other BigDecimal entry points still have it. From the code, Literal(new java.math.BigDecimal("0E+39")) (literals.scala L83-85) builds Decimal(d) with precision 40. DecimalType.fromDecimal then throws DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION, while Literal(java.math.BigDecimal.ZERO) works. With 0E+2147483647, the precision is -2147483648, and DecimalType fails with DECIMAL_SCALE_EXCEEDS_PRECISION.

I know SPARK-60132 tracks the other entry points, so this doesn't need to block this PR. If set(BigDecimal) handled zero itself (precision 1 and scale 0 for a zero with a negative scale when negative scales are not allowed), every entry point would be covered, and this branch would only need to skip the fast-fail. Could you add the zero case to SPARK-60132 so it isn't lost?

Added the zero case and this Literal example to SPARK-60132.

nit: This keeps a non-negative scale as it is, so a zero with a large negative exponent still gets a precision that grows with the exponent:

0e-50:         bd.precision=1 bd.scale=50         -> Decimal._precision=50         _scale=50
0e-2147483647: bd.precision=1 bd.scale=2147483647 -> Decimal._precision=2147483647 _scale=2147483647

(I checked this with java.math.BigDecimal on JDK 21, mirroring this branch and Decimal.set.) Cast still returns 0.00 because BigDecimal.setScale returns a zero right away, so nothing fails. But this comment says the precision no longer grows with the exponent, and the test comment says "whatever its exponent", while both only cover positive exponents.

Could you either cap the scale for zero, e.g. math.min(math.max(bigDecimal.scale, 0), DecimalType.MAX_SCALE), or narrow the comment to positive exponents?

Narrowed the comment to positive exponents. I'd rather not cap the scale only for zero, since a nonzero value like '1e-50' gets precision 50 the same way and changePrecision handles both.

The helper checks the legacy conf before the zero branch. So under spark.sql.legacy.allowNegativeScaleOfDecimal=true, a zero such as '0e39' keeps its negative scale (precision 1, scale -39) and is not normalized. I think that's intended, but no test covers it, so reordering the branches (as in the suggestion at Decimal.scala L654) could change it silently.

How about a legacy block like the one in the SPARK-37451 test above? For example:

    withSQLConf(SQLConf.LEGACY_ALLOW_NEGATIVE_SCALE_OF_DECIMAL_ENABLED.key -> "true") {
      Seq(Decimal.fromString(UTF8String.fromString("0e39")),
        Decimal.fromStringANSI(UTF8String.fromString("0e39"))).foreach { d =>
        assert(d === Decimal(0))
        assert(d.scale === -39)
      }
    }

Also, the '0.00' check at L469 runs only through fromString. It could cover fromStringANSI too, like the loop above.

Done, added the legacy block and the fromStringANSI check for '0.00'.

nit: This branch is the same as the last else branch. The conf matters only for a zero with a negative scale and for an out-of-range value. So the four branches could be the two conditions in the suggestion at L654, which also keeps the conf off the common path.

Done as part of the restructuring above.

nit: The helper returns null only when the integral part has more than MAX_PRECISION digits and negative scales are not allowed. A value whose scale no decimal type can hold, such as '1e-50' (precision 50, scale 50), comes back as a non-null Decimal, and changePrecision deals with it later.

How about something like "or returns null if its integral part has more than DecimalType.MAX_PRECISION digits"? Then a new caller that skips changePrecision won't take a non-null result for a valid decimal.

Done.

Nit (P3): The fall-through Decimal(bigDecimal) goes through Decimal.set(BigDecimal), which still evaluates decimal.scale < 0 && !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled (L151). So an in-range string with a negative scale, such as '1e5' or '1.2E+3', still reads the conf on every row. That matches master, so it's not a regression, but this sentence says the conversion reads the conf only for the two rare cases.

Could you scope it to this helper's own checks? For example, "so this helper reads the conf only for a zero with a negative scale and for a value with too many integral digits".

Verification:

• Inspection: The revised comment is consistent with the code: the helper's guards read the conf only on its two rare branches, and any statement about the conversion as a whole accounts for Decimal.set reading the conf for negative-scale values.
• Compile: The sql/api module still compiles and scalastyle passes on the edited comment (ASCII only, lines within 100 characters).

Thanks. Scoped the sentence to this helper's own checks, as you suggested.

This line fails the scalafmt check in CI. At e7dd3a4, the Run / Linters, licenses, and dependencies job fails in the Scala linter step with Scalafmt: Unformatted files found on spark-sql-api. The previous commit passed this job. For sql/api, scalafmt wants a 2-space continuation indent here, as in the original code on master, not 4. Sorry, the 4-space indent came from the snippet I suggested in my previous review.

I ran the formatter on this commit, and it changed only this line and the Javadoc at L649-652 (see the other comment):

./build/mvn scalafmt:format -Dscalafmt.skip=false -Dscalafmt.validateOnly=false -Dscalafmt.changedOnly=false -pl sql/api

Because this step failed, the later lint steps (the structured logging check, the Java linter, the Connect JVM client MiMa check, and the Python, JS and R linters) were skipped, so they haven't run on this commit yet.

Fixed, thanks. No worries about the snippet; I should have run scalafmt on sql/api myself.

L649 is 99 characters, over the maxColumn = 98 in dev/.scalafmt.conf that applies to sql/api. It is the other spot behind the scalafmt failure. Scalastyle allows 100 characters, so it doesn't catch this. This is how scalafmt wraps the paragraph:

Fixed with scalafmt's wrapping.

The precision and scale asserts below only cover exponents above 38, which returned null before. This PR also changes two nearby cases that no test covers. '0e38' is the boundary, the first positive exponent that master rejected (1 - (-38) = 39 > 38). An in-range zero with a negative scale, such as '0e37' or '0e5', now gets precision 1 instead of a precision derived from the exponent. I checked this with java.math.BigDecimal on JDK 21, mirroring master and this PR followed by Decimal.set:

0e5:  master precision 6,  scale 0 -> this PR precision 1, scale 0
0e37: master precision 38, scale 0 -> this PR precision 1, scale 0
0e38: master null                  -> this PR precision 1, scale 0

Suppose a later change normalizes zero only when the fast-fail would have fired, or moves the rule into Decimal.set for SPARK-60132 and misses this case. Then '0e37' could go back to precision 38, and every current test would still pass. How about adding "0e38" and "0e37" to this list, and "0e38" to the CastSuiteBase list (L334)?

Good point. Added '0e37' and '0e38' here (both now assert precision 1 and scale 0) and '0e38' to the CastSuiteBase list.