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: [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]
