HyukjinKwon commented on code in PR #59335:
URL: https://github.com/apache/spark/pull/59335#discussion_r4242393049
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -458,6 +458,19 @@ final class Decimal extends Ordered[Decimal] with
Serializable {
if (dv.ne(null)) {
// We get here if either we started with a BigDecimal, or we switched to
one because we would
// have overflowed our Long; in either case we must rescale dv to the
new scale.
+ if (math.abs(dv.scale.toLong - scale) > DecimalType.MAX_PRECISION) {
+ // setScale takes time proportional to the scale change and fails once
the change
Review Comment:
**Nit (P3):** The example here doesn't match the stated condition. Rescaling
`1e-2147483647` (scale 2147483647) to scale 2 is a change of 2147483645, which
fits in an Int. On the JDK, `new BigDecimal("1e-2147483647").setScale(2,
RoundingMode.HALF_UP)` throws `BigInteger would overflow supported range`,
because `10^change` is too large for BigInteger. The Int-range check fails
differently (`Underflow`), and only for other inputs, e.g. `1e2147483647` to
scale 2 with negative scales allowed. Could you reword it to something like:
```scala
// setScale's cost grows with the scale change, and it throws once 10^change
exceeds
// BigInteger's range (e.g. for 1e-2147483647) or the change exceeds the Int
range.
// Skip it when the result is known.
```
**Verification:**
- **Inspection:** The revised comment matches BigDecimal.setScale's behavior
for each cited example, and the diff touches only comment lines.
##########
sql/api/src/main/scala/org/apache/spark/sql/errors/DataTypeErrors.scala:
##########
@@ -31,6 +31,10 @@ import org.apache.spark.unsafe.types.UTF8String
* into [[CompilationErrors]].
*/
private[sql] object DataTypeErrors extends DataTypeErrorsBase {
+ // Above this many trailing zeros, a decimal value in an error message is
rendered in
Review Comment:
**Nit (P3):** This describes the threshold as a count of trailing zeros, but
the check in `numericValueOutOfRange` is `value.scale <
-MAX_PLAIN_STRING_TRAILING_ZEROS`, which counts only the zeros implied by a
negative scale. For example, `1000e1000` parses to unscaled 1000 with scale
-1000. Its plain form has 1003 trailing zeros, but it is still rendered in
plain notation. The behavior itself looks right for keeping the string bounded,
so only the wording needs to match, e.g.:
```scala
// A decimal whose scale is below -MAX_PLAIN_STRING_TRAILING_ZEROS (that is,
with more than
// this many zeros implied by a negative scale) is rendered in scientific
notation in error
// messages instead of plain notation.
```
**Verification:**
- **Inspection:** The revised comment describes the same condition as the
check in numericValueOutOfRange, and the diff touches only comment lines.
--
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]
