SEPURI-SAI-KRISHNA commented on PR #11724:
URL: https://github.com/apache/seatunnel/pull/11724#issuecomment-5294127957
Thanks for catching this, and for saying plainly that it was a carryover
from your earlier pass — but I don't think the miss is mainly yours. You named
the scale gap in writing in the 2026-08-10 review and I read that sentence,
agreed with "pre-existing, out of scope", and moved on without tracing where
the value goes either. You wrote it down; I had the same information and drew
the same wrong conclusion. The difference is that you went back and checked.
**Issue 1 is confirmed.** I reproduced it before changing anything, by
asking the transform for its own declared output type and comparing it against
what it actually emits, on `DECIMAL(38,2)` operands `10.00 * 3.00`:
| column | declared | emitted | |
|---|---|---|---|
| `a + b` | `Decimal(38, 2)` | `13.00` scale 2 | ok |
| `a - b` | `Decimal(38, 2)` | `7.00` scale 2 | ok |
| `a * b` | `Decimal(38, 2)` | `30.0000` scale 4 | **mismatch** |
| `a / b` | `Decimal(38, 2)` | `3.33` scale 2 | ok |
Your reading of the before/after is exactly right, and I checked that too
rather than assuming. Under the old lossy conversion `10.00 * 3.00` went
through `BigDecimal.valueOf(10.0)` → `"10.0"` (scale 1), so the product landed
on scale 2 and happened to match; but `10.25 * 3.75` produced scale 4 and
mismatched even then. So this was intermittent and value-dependent before, and
deterministic after. Not a new class of bug, but "guaranteed" is what breaks
jobs that were running, and your Parquet/Avro trace is the path that turns it
into a hard failure rather than a formatting difference.
## Fix: Option A
I took Option A — normalize the product to the declared scale, the way
`Division` already does:
```java
if (binaryExpression instanceof Multiplication) {
// BigDecimal.multiply returns a value whose scale is leftScale +
rightScale,
// but the column type declared for this expression is
// DECIMAL(max(precision), max(scale)). Normalise to the declared scale,
as
// Division already does, so the emitted value matches the schema the
transform
// advertises: a sink that encodes against that schema (Parquet through
Avro,
// for example) rejects a value whose scale differs from the declared
one.
DecimalType decimalType = (DecimalType) resultType;
return leftBigDecimal
.multiply(rightBigDecimal)
.setScale(decimalType.getScale(), RoundingMode.HALF_UP);
}
```
Reasoning for A over B, since you asked for the choice to be explicit rather
than implicit:
Option B is the more principled fix in isolation — `sLeft + sRight` is what
standard SQL decimal multiplication does, and it keeps the value exact. But it
changes the **declared output schema** of every existing multiplication job,
and under this project's backward-compatibility rules a schema change is a
strictly bigger breakage than a value change: a sink whose target column was
created from the old declared type now gets a wider type, and precision would
also need a widening rule with an answer for what happens past `DECIMAL(38,
…)`. It would also mean touching `ZetaSQLType`, which #11697 modified two days
ago. That is a real design change to decimal type inference and it deserves its
own PR, its own `incompatible-changes.md` entry, and a maintainer's decision —
not a fourth-round amendment to a precision bug fix.
On the tension you flagged with this PR's "keep full precision" framing —
I'd argue Option A doesn't actually give any of it back. What changed is
*where* the rounding happens. The old code rounded the **operands** and
multiplied the damaged values; the new code multiplies exactly and rounds the
**result** once. For the existing test case the old code emitted
`1234567890123456.8` where the true product is `1234567890123456.7899`; Option
A emits `1234567890123456.79`. That is the correctly rounded value rather than
an error-propagated one, which is the same relationship `Division` already has
to its inputs. The PR's claim is that operands are converted exactly, and that
still holds.
## Regression test
I added `testEmittedScaleMatchesDeclaredType`, which is your recommended
test: it pulls the transformed `CatalogTable`'s column type and asserts it
agrees with the emitted `BigDecimal`'s scale, across all four operators in one
query.
I verified it actually catches the bug rather than just passing — reverting
the one-line fix and re-running gives:
```
declared and emitted scale differ for column mul_val (value 38.4375) ==>
expected: <2> but was: <4>
```
That is the guard you asked for: whichever way a future change goes,
schema/value agreement is now pinned rather than assumed, and the failure
message names the column and the value.
## Docs
Both `docs/en` and `docs/zh` `incompatible-changes.md` gain a bullet on the
multiplication result scale — that it now rounds to the declared scale with
`HALF_UP`, why (the declared type is `max(scale)`, not the sum), and a worked
example. It is user-visible independently of the sink failure, so it belongs
there whichever option had been chosen.
## Issue 2 — `toBigDecimal` duplication
Agreed, and agreed the deferral reason has expired now that #11697 is in.
I'd rather not fold a refactor into this PR at round four, since it would put a
non-behavioural change into a diff you have now reviewed carefully several
times — but you're right that "later" needs to be a PR rather than an
intention, so I'll open it against `dev` once this lands and link it here.
While tracing `NumericFunction` for this I also noticed `abs()` returns
`Integer.MIN_VALUE` for `ABS(-2147483648)` (`Math.abs` overflow, same shape as
the bug in #11721) and that `round()`'s type switch has no `BYTE` case, so
`ROUND(tinyint_col, -1)` is a silent no-op. Those are separate bugs and I'll
file them separately rather than smuggle them into a unification PR.
## Current state
Full `seatunnel-transforms-v2` suite: 1087 tests, 0 failures, on JDK 11
locally. `spotless:check` clean.
On CI: I won't claim green. Run `31773166956` on the previous head had
`unit-test (8, windows-latest)` fail for the third consecutive time, and
because that matrix is fail-fast it cancelled all three sibling legs —
including both ubuntu ones, which are where `SQLDecimalArithmeticTest` actually
runs. So CI has not yet executed these tests on any push; the numbers above are
local only. I'll open the Windows/JDK 8 flake issue as you suggested — with
that fail-fast masking effect included, since it's costing every PR that
touches this repo a matrix, not just this one.
--
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]
