You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2026/10/10 04:57:53 UTC
[PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
viirya opened a new pull request, #59334:
URL: https://github.com/apache/spark/pull/59334
### 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`.
### 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`), so `try_cast`, legacy `CAST` and ANSI `CAST` all fail the task instead of returning NULL or raising `NUMERIC_OUT_OF_SUPPORTED_RANGE`.
### 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`. `'0e2147483647'`, which previously slipped through the check and returned 0, is now handled like `'0e100'` and `'0e2147483646'`.
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya closed pull request #59334: [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal
URL: https://github.com/apache/spark/pull/59334
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4236865023
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ object Decimal {
}
}
- 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 =
Review 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?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
Review 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?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,19 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
Review 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 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.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ object Decimal {
}
}
- private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Int =
- bigDecimal.precision - bigDecimal.scale
+ // Computed in Long since `precision - scale` can overflow Int, e.g. for "1e2147483647".
Review 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.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ object Decimal {
}
}
- 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 =
Review 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.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -475,6 +475,11 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
checkExceptionInExpression[ArithmeticException](
cast("6E+37", DecimalType(38, 1)),
"cannot be represented as Decimal(38, 1)")
+ Seq("1e2147483647", "0e2147483647", "1.5e2147483647", "1e2147483648").foreach { s =>
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #59334:
URL: https://github.com/apache/spark/pull/59334#issuecomment-6100496414
Thanks for the thorough review, @dongjoon-hyun.
- Large negative exponents (`'1e-2147483647'`, and the slow `'1e-10000000'`) and the legacy negative-scale conf: these fail in `changePrecision` (`setScale` with a huge scale delta), not in the fast-fail check this PR fixes. They are handled in #59335 (SPARK-60119), together with cast-level tests for both cases, so I kept this PR scoped to the fast-fail.
- Zero: agreed, `'0e39'` returning NULL is the same false positive. The fast-fail now skips zero (`bigDecimal.signum != 0`), so `'0e2147483647'` keeps returning `0.00` and `'0e39'` is fixed. I updated the tests and the PR description.
- `Decimal.set` L152 and the other `BigDecimal` entry points: filed SPARK-60132. For CSV/JSON the exception currently becomes a bad record rather than a task failure, but it should still be handled consistently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #59334:
URL: https://github.com/apache/spark/pull/59334#issuecomment-6152720556
Thanks, @dongjoon-hyun. Fixed in 2e4ddb4: the continuation indent and the Javadoc wrap now pass `scalafmt` on `sql/api` (I verified with the same `scalafmt:format` validate command, which fails on e7dd3a4), and the tests now cover `'0e37'` and `'0e38'` in `DecimalSuite` and `'0e38'` in `CastSuiteBase`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #59334:
URL: https://github.com/apache/spark/pull/59334#issuecomment-6133279031
Thanks, @dongjoon-hyun. Addressed in e7dd3a4:
- The helper now reads the legacy conf only for a zero with a negative scale and for a value with too many integral digits, as before this PR, so an in-range string doesn't touch the conf. I used your suggested structure, which also removes the duplicate branch.
- The comment now says the precision doesn't grow with a positive exponent. I didn't cap the scale of zero: a nonzero value like `'1e-50'` gets precision 50 the same way, and `changePrecision` handles both.
- The Javadoc now says exactly when `null` is returned.
- `DecimalSuite` now also checks zero under the legacy conf (scale -39 is kept) and `'0.00'` through `fromStringANSI`.
- I added the zero case and the `Literal` example to SPARK-60132.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4247345658
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -648,8 +649,10 @@ object Decimal {
try {
val bigDecimal = stringToJavaBigDecimal(str)
// We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
+ // For example: Decimal("6.0790316E+25569151"). Zero fits any decimal type whatever its
+ // exponent, e.g. "0e39", so it is not checked.
+ if (bigDecimal.signum != 0 &&
Review Comment:
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'`.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -475,6 +475,14 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
checkExceptionInExpression[ArithmeticException](
cast("6E+37", DecimalType(38, 1)),
"cannot be represented as Decimal(38, 1)")
+ Seq("1e2147483647", "1.5e2147483647", "12e2147483647").foreach { s =>
+ checkExceptionInExpression[ArithmeticException](
+ cast(s, DecimalType(10, 2)),
+ "NUMERIC_OUT_OF_SUPPORTED_RANGE")
+ }
+ Seq("0e39", "0e2147483647").foreach { s =>
Review Comment:
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.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,24 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647",
+ "1.5e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === null)
+ checkError(
+ exception = intercept[SparkArithmeticException](
+ Decimal.fromStringANSI(UTF8String.fromString(string))),
+ condition = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+ parameters = Map("value" -> string))
+ }
+ // Zero fits any decimal type whatever its exponent.
+ Seq("0e39", "-0E+100", "0e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === Decimal(0))
Review Comment:
`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`?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -668,8 +671,10 @@ object Decimal {
try {
val bigDecimal = stringToJavaBigDecimal(str)
// We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
+ // For example: Decimal("6.0790316E+25569151"). Zero fits any decimal type whatever its
+ // exponent, e.g. "0e39", so it is not checked.
+ if (bigDecimal.signum != 0 &&
Review Comment:
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.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,24 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
Review Comment:
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")`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4242296137
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,24 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647", "1.5e2147483647",
+ "1e2147483648").foreach { string =>
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4251417666
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -648,8 +649,10 @@ object Decimal {
try {
val bigDecimal = stringToJavaBigDecimal(str)
// We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
+ // For example: Decimal("6.0790316E+25569151"). Zero fits any decimal type whatever its
+ // exponent, e.g. "0e39", so it is not checked.
+ if (bigDecimal.signum != 0 &&
Review Comment:
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.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,24 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647",
+ "1.5e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === null)
+ checkError(
+ exception = intercept[SparkArithmeticException](
+ Decimal.fromStringANSI(UTF8String.fromString(string))),
+ condition = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+ parameters = Map("value" -> string))
+ }
+ // Zero fits any decimal type whatever its exponent.
+ Seq("0e39", "-0E+100", "0e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === Decimal(0))
Review Comment:
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.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -668,8 +671,10 @@ object Decimal {
try {
val bigDecimal = stringToJavaBigDecimal(str)
// We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
+ // For example: Decimal("6.0790316E+25569151"). Zero fits any decimal type whatever its
+ // exponent, e.g. "0e39", so it is not checked.
+ if (bigDecimal.signum != 0 &&
Review Comment:
Done. Moved into one private helper, `stringBigDecimalToDecimal`, which also holds the zero rule.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -475,6 +475,14 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
checkExceptionInExpression[ArithmeticException](
cast("6E+37", DecimalType(38, 1)),
"cannot be represented as Decimal(38, 1)")
+ Seq("1e2147483647", "1.5e2147483647", "12e2147483647").foreach { s =>
+ checkExceptionInExpression[ArithmeticException](
+ cast(s, DecimalType(10, 2)),
+ "NUMERIC_OUT_OF_SUPPORTED_RANGE")
+ }
+ Seq("0e39", "0e2147483647").foreach { s =>
Review Comment:
Done. Moved to `CastSuiteBase`, so they run for ANSI on, ANSI off and `try_cast`.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,24 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4262662817
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is very slow. For
+ * example: Decimal("6.0790316E+25569151")
+ *
+ * This runs for every row of a cast, so the conf is read only for a zero with a negative scale
Review Comment:
Thanks. Scoped the sentence to this helper's own checks, as you suggested.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is very slow. For
+ * example: Decimal("6.0790316E+25569151")
+ *
+ * This runs for every row of a cast, so the conf is read only for a zero with a negative scale
+ * and for a value with too many integral digits.
+ */
+ 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 a positive 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) {
Review Comment:
Fixed, thanks. No worries about the snippet; I should have run scalafmt on sql/api myself.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is very slow. For
+ * example: Decimal("6.0790316E+25569151")
Review Comment:
Fixed with scalafmt's wrapping.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,42 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("SPARK-60118: UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647",
+ "1.5e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === null)
+ checkError(
+ exception = intercept[SparkArithmeticException](
+ Decimal.fromStringANSI(UTF8String.fromString(string))),
+ condition = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+ parameters = Map("value" -> string))
+ }
+ // Zero fits any decimal type whatever its exponent.
+ Seq("0e39", "-0E+100", "0e2147483647").foreach { string =>
Review Comment:
Good point. Added `'0e37'` and `'0e38'` here (both now assert precision 1 and scale 0) and `'0e38'` to the `CastSuiteBase` list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #59334:
URL: https://github.com/apache/spark/pull/59334#issuecomment-6158763943
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/79ab7682560912b19301818cb27f76b52bf98b2f
- merged into branch-4.x https://github.com/apache/spark/commit/770e31eaaf3b5c1b8e0703d1b776d85e457f584d
- merged into branch-4.3 https://github.com/apache/spark/commit/5e43899c2c43404c5010b24e5f313be17d5da64f
- merged into branch-4.2 https://github.com/apache/spark/commit/45ca8822abaa24af5daf9a2d1b5ee696ca81c032
- merged into branch-4.1 https://github.com/apache/spark/commit/9f65f1c1e560ed27a0229a746b0a78529ed6655d
- merged into branch-4.0 https://github.com/apache/spark/commit/a3a135a05a37d3e656fe9ea78bbc1eca773496db
*Posted by `merge_spark_pr.py`*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #59334:
URL: https://github.com/apache/spark/pull/59334#issuecomment-6158770566
Thanks @HyukjinKwon @dongjoon-hyun !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #59334:
URL: https://github.com/apache/spark/pull/59334#issuecomment-6130720849
Thanks again, @dongjoon-hyun. All five are addressed in e44b77f:
- Zero is normalized before `Decimal.set`: when negative scales are not allowed, a zero's negative scale is dropped first (`setScale(max(scale, 0))`), so `'0e39'` and `'0e2147483647'` now give precision 1 and scale 0, and `'0.00'` keeps scale 2. Under the legacy negative-scale conf zero is left as is, since a negative scale is valid there.
- The fast-fail is now one private helper used by both `fromString` and `fromStringANSI`.
- `DecimalSuite` checks the precision and scale of the zero results (the precision assertion fails without the normalization), the zero cast checks moved to `CastSuiteBase`, and the test name has the JIRA prefix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4238636780
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
Review 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.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ object Decimal {
}
}
- 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 =
Review Comment:
Covered in #59335 (SPARK-60119), with a cast-level test under the legacy conf. I noted it in the PR description.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -475,6 +475,11 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
checkExceptionInExpression[ArithmeticException](
cast("6E+37", DecimalType(38, 1)),
"cannot be represented as Decimal(38, 1)")
+ Seq("1e2147483647", "0e2147483647", "1.5e2147483647", "1e2147483648").foreach { s =>
Review Comment:
Good point. Done: the fast-fail skips zero now, and the tests check that `'0e39'`, `'0e2147483647'` and `'0.0e2147483648'` cast to `0.00`.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ object Decimal {
}
}
- private def numDigitsInIntegralPart(bigDecimal: JavaBigDecimal): Int =
- bigDecimal.precision - bigDecimal.scale
+ // Computed in Long since `precision - scale` can overflow Int, e.g. for "1e2147483647".
Review Comment:
Same fix as above in #59335; `'1e-100000000'` returns `0.00` without the `setScale` cost.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,8 +635,9 @@ object Decimal {
}
}
- 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 =
Review Comment:
Filed SPARK-60132 for this.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,19 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
Review Comment:
The negative-exponent and legacy-conf cases are tested in #59335. I added zero cases here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4252551518
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
+ * type can hold it. We fast fail because constructing a very large JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
Review Comment:
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 `ConfigReader`s 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:
```scala
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.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
+ * type can hold it. We fast fail because constructing a very large JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
+ } else if (bigDecimal.signum == 0) {
Review Comment:
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?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
+ * type can hold it. We fast fail because constructing a very large JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
+ } else 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.
Review Comment:
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?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,29 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("SPARK-60118: UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647",
+ "1.5e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === null)
+ checkError(
+ exception = intercept[SparkArithmeticException](
+ Decimal.fromStringANSI(UTF8String.fromString(string))),
+ condition = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+ parameters = Map("value" -> string))
+ }
+ // Zero fits any decimal type whatever its exponent.
+ Seq("0e39", "-0E+100", "0e2147483647").foreach { string =>
Review Comment:
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:
```scala
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.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
Review Comment:
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.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
+ * type can hold it. We fast fail because constructing a very large JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4256056610
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is very slow. For
+ * example: Decimal("6.0790316E+25569151")
Review Comment:
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:
```suggestion
* Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if its
* integral part has more than `DecimalType.MAX_PRECISION` digits and negative scales are not
* allowed. We fast fail because constructing a very large JavaBigDecimal to Decimal is very
* slow. For example: Decimal("6.0790316E+25569151")
```
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is very slow. For
+ * example: Decimal("6.0790316E+25569151")
+ *
+ * This runs for every row of a cast, so the conf is read only for a zero with a negative scale
+ * and for a value with too many integral digits.
+ */
+ 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 a positive 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) {
Review Comment:
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
```
```suggestion
!SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
```
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.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,42 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("SPARK-60118: UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647",
+ "1.5e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === null)
+ checkError(
+ exception = intercept[SparkArithmeticException](
+ Decimal.fromStringANSI(UTF8String.fromString(string))),
+ condition = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+ parameters = Map("value" -> string))
+ }
+ // Zero fits any decimal type whatever its exponent.
+ Seq("0e39", "-0E+100", "0e2147483647").foreach { string =>
Review Comment:
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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4255550610
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,45 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
- def fromString(str: UTF8String): Decimal = {
- try {
- val bigDecimal = stringToJavaBigDecimal(str)
- // We fast fail because constructing a very large JavaBigDecimal to Decimal is very slow.
- // For example: Decimal("6.0790316E+25569151")
- if (numDigitsInIntegralPart(bigDecimal) > DecimalType.MAX_PRECISION &&
- !SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
- null
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if its integral
+ * part has more than `DecimalType.MAX_PRECISION` digits and negative scales are not allowed. We
+ * fast fail because constructing a very large JavaBigDecimal to Decimal is very slow. For
+ * example: Decimal("6.0790316E+25569151")
+ *
+ * This runs for every row of a cast, so the conf is read only for a zero with a negative scale
Review Comment:
**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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4252646145
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
+ * type can hold it. We fast fail because constructing a very large JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
Review Comment:
Good catch, that was a regression from the previous commit. Done with your suggested structure; an in-range string no longer reads the conf.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
+ * type can hold it. We fast fail because constructing a very large JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
+ } else if (bigDecimal.signum == 0) {
Review Comment:
Added the zero case and this `Literal` example to SPARK-60132.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
+ * type can hold it. We fast fail because constructing a very large JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
+ } else 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.
Review Comment:
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.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,29 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("SPARK-60118: UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647",
+ "1.5e2147483647").foreach { string =>
+ assert(Decimal.fromString(UTF8String.fromString(string)) === null)
+ checkError(
+ exception = intercept[SparkArithmeticException](
+ Decimal.fromStringANSI(UTF8String.fromString(string))),
+ condition = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+ parameters = Map("value" -> string))
+ }
+ // Zero fits any decimal type whatever its exponent.
+ Seq("0e39", "-0E+100", "0e2147483647").foreach { string =>
Review Comment:
Done, added the legacy block and the `fromStringANSI` check for `'0.00'`.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
+ * type can hold it. We fast fail because constructing a very large JavaBigDecimal to Decimal is
+ * very slow. For example: Decimal("6.0790316E+25569151")
+ */
+ private def stringBigDecimalToDecimal(bigDecimal: JavaBigDecimal): Decimal = {
+ if (SqlApiConf.get.allowNegativeScaleOfDecimalEnabled) {
+ Decimal(bigDecimal)
Review Comment:
Done as part of the restructuring above.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -635,26 +635,38 @@ object Decimal {
}
}
- 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 =
+ bigDecimal.precision.toLong - bigDecimal.scale
private def stringToJavaBigDecimal(str: UTF8String): JavaBigDecimal = {
// According the benchmark test, `s.toString.trim` is much faster than `s.trim.toString`.
// Please refer to https://github.com/apache/spark/pull/26640
new JavaBigDecimal(str.toString.trim)
}
+ /**
+ * Converts a `JavaBigDecimal` parsed from a string to `Decimal`, or returns null if no decimal
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60118][SQL] Fix Int overflow in the fast-fail check of casting string to decimal [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #59334:
URL: https://github.com/apache/spark/pull/59334#discussion_r4244140029
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DecimalSuite.scala:
##########
@@ -446,6 +446,24 @@ class DecimalSuite extends SparkFunSuite with PrivateMethodTester with SQLHelper
}
}
+ test("UTF8String to Decimal with exponent near Int.MaxValue") {
+ // The number of integral digits (precision - scale) of these values overflows Int.
+ Seq("1e2147483647", "-1E+2147483647", "12e2147483647", "1.5e2147483647",
+ "1e2147483648").foreach { string =>
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org