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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
