spark-test commented on code in PR #59889:
URL: https://github.com/apache/spark/pull/59889#discussion_r4419695574
##########
sql/api/src/main/scala/org/apache/spark/sql/types/Decimal.scala:
##########
@@ -556,9 +556,29 @@ final class Decimal extends Ordered[Decimal] with
Serializable {
if (that.isZero) null
else Decimal(toJavaBigDecimal.divide(that.toJavaBigDecimal, scale,
RoundingMode.HALF_UP))
- def %(that: Decimal): Decimal =
- if (that.isZero) null
- else Decimal(toJavaBigDecimal.remainder(that.toJavaBigDecimal,
MATH_CONTEXT))
+ def %(that: Decimal): Decimal = {
+ if (that.isZero) {
+ null
+ } else {
+ val dividend = toJavaBigDecimal
+ val divisor = that.toJavaBigDecimal
+ // Try the context first for speed and for huge trailing-zero quotients
that exact division
+ // cannot materialize. On failure, retry without its quotient precision
limit: a quotient
+ // wider than the context may still have a representable remainder.
+ val remainder = try {
+ dividend.remainder(divisor, MATH_CONTEXT)
+ } catch {
+ case error: ArithmeticException =>
+ // Exact division materializes the integral quotient. Two maximum
SQL decimal
+ // precisions cover ordinary decimal operands; keep the original
failure for larger
+ // legacy scale gaps instead of attempting a potentially enormous
allocation.
Review Comment:
**Nit (P3):** This comment says the original failure is kept for "larger
legacy scale gaps", but the guard below never reads the legacy config. It
rethrows whenever `dividend.precision - dividend.scale + divisor.scale` exceeds
76. That also covers non-legacy `Decimal` values outside SQL bounds:
- `Decimal(1L) % Decimal("3E-100")`: `Decimal(String)` keeps scale 100
without any config, so the estimate is 101. The new `decimal remainder bounds
exact retries by divisor scale` test asserts this rethrow outside any
`withSQLConf`.
- `Decimal("1" + "0" * 77) % Decimal(3)`: precision 78 and scale 0 give an
estimate of 78.
The divisor-scale case matters. For a divisor such as `3E-100000000`, the
exact retry would materialize a quotient of about 10^8 digits. The test's own
comment states that role ("exact division would succeed here and allow enormous
quotients for smaller divisors"), but the production comment presents the bound
as a legacy-only safeguard. A maintainer who trusts it could narrow the guard
or gate it on the legacy config. For SQL-typed operands the legacy wording does
hold, because only the legacy negative-scale config lets them exceed the bound.
The gap is direct `Decimal` values. The divisor-scale test would catch such a
change, so this is documentation only, and `%` behavior does not need to change.
Suggested comment-only change:
```suggestion
// precisions cover in-range SQL operands; wider estimates (legacy
negative scales or
// values beyond SQL bounds) keep the original failure to avoid a
huge allocation.
```
**Verification:**
- **Inspection:** The comment above the quotient-size guard names the
condition the guard tests, and it no longer presents the rejected inputs as
limited to legacy negative-scale gaps.
- **Inspection:** The change touches only comment text, leaving the guard
expression and the surrounding remainder logic byte-for-byte unchanged.
--
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]
