SEPURI-SAI-KRISHNA commented on PR #11937:
URL: https://github.com/apache/seatunnel/pull/11937#issuecomment-5392566745
Thanks @goutamadwant and @DanielLeens — the finding is correct and it's now
fixed in this PR.
I verified the mechanism rather than taking it on trust, because the claim
is narrow enough to be worth pinning down exactly. Under `Locale("tr", "TR")`:
| Simple class name | `toUpperCase()` | `toUpperCase(Locale.ROOT)` | Matches
its case label |
|---|---|---|---|
| `BigDecimal` | `BİGDECİMAL` | `BIGDECIMAL` | **no** |
| `Byte` | `BYTE` | `BYTE` | yes |
| `Short` | `SHORT` | `SHORT` | yes |
| `Integer` | `INTEGER` | `INTEGER` | yes |
| `Long` | `LONG` | `LONG` | yes |
| `Double` | `DOUBLE` | `DOUBLE` | yes |
| `Float` | `FLOAT` | `FLOAT` | yes |
So `BigDecimal` really is the only affected label, exactly as you both said
— `Integer` survives because its only `i` is already the capital.
And the merge-readiness framing is the part that matters: on the parent
commit this mismatch fell through the switch and returned the value unrounded,
which is silently wrong. With the `default` branch it throws. Trading a silent
wrong answer for a crash is not a fix, so pinning the locale belongs in the
same commit as the `default` branch rather than in a follow-up.
## What changed
`NumericFunction.java` — `t.toUpperCase(Locale.ROOT)` at the rounding
switch, plus a comment saying why so it isn't "simplified" back later.
I also pinned the second switch, `convertTo` at `NumericFunction.java:336`.
It is **not** reachable today — its four labels are
`BYTE`/`INTEGER`/`SHORT`/`LONG` and none contains a lowercase `i`, and it is
only ever called from the integral branch, so no `BigDecimal` reaches it. I
pinned it anyway because leaving one locale-sensitive `toUpperCase` next to a
fixed one is how the bug comes back when a type is added. Flagging it
explicitly so it doesn't read as an unexplained extra hunk.
## Regression test
`testRoundingDispatchIsLocaleIndependent` sets the Turkish default locale,
asserts `ROUND(BigDecimal("1.25"), 1)` is `1.3` and `CEIL(BigDecimal("1.25"))`
is `2`, and restores the previous locale in a `finally`. Asserting the *rounded
result* rather than just "does not throw" rules out both failure modes with one
assertion: the pre-#11937 silent `1.25` and the post-`default` exception.
The module has no `junit-platform.properties` and no surefire parallel
configuration, so tests run single-threaded in the fork and the save/restore
cannot leak into a sibling test.
## Verification
- `./mvnw -pl seatunnel-transforms-v2 test` → **1102 tests, 0 failures, 0
errors** (1101 before, +1).
- `spotless:check` clean; both files ASCII-only (the dotted capital I in the
test comment is written as a `\u0130` escape).
- Mutation check: reverting *only* `Locale.ROOT` on the rounding switch,
leaving everything else in place, fails **exactly one** test —
`testRoundingDispatchIsLocaleIndependent` — with `TransformException`, and no
other test in the 1102 moves. That is the failure mode @goutamadwant described,
reproduced and then killed.
## On Issue 2 (`MOD` result-type dispatch)
Agreed, and thanks for tracing it — `mod()`'s operand widening handles
`Byte`/`Short` via `toBigDecimal`, but its result-type dispatch doesn't, so
`MOD` on a `TINYINT` still throws today, before and after this PR. It's the
same bug class and the same reachability argument.
I'd rather not widen this PR's scope to it, for the reason you noted
yourself: this PR only exists because a reviewer-found gap in #11927 became its
own issue and PR, and that has worked well. I'll file it the same way once this
one settles, so it gets its own repro and its own before/after table rather
than riding along unexamined. If you'd prefer it folded in here instead, say so
and I'll do that.
Also worth recording: the apache-side `Build` shows red, but that is a
single `cancelled` job, not a failure. The fork run for this head is 78
succeeded / 10 skipped / 1 cancelled, and the one cancelled job is
`paimon-connector-it (11, ubuntu-latest)` — a module this PR doesn't touch,
whose JDK-8 twin passed. Every `seatunnel-transforms-v2` job, including all
four `unit-test` legs and both `transform-v2-it` shards, is green.
--
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]
