SEPURI-SAI-KRISHNA commented on PR #12605:
URL: https://github.com/apache/seatunnel/pull/12605#issuecomment-5946998265
Thanks, this is a careful review and Issue 1 was a real defect. All four are
addressed, as a follow-up commit rather than a force-push so the delta is
reviewable.
Two parts of your suggested fix I did not take. Both are deliberate, and I
would rather set out the reasoning and let you decide than quietly diverge.
## Issue 1, confirmed and fixed
Reproduced before changing anything. On the previous head:
| input to `CAST(... AS INT)` | result |
| --- | --- |
| `BigDecimal` `18446744073709551621` (2^64+5) | `5` |
| `BigDecimal` `18446744073709551616` (2^64) | `0` |
| `Double` `NaN` | `0` |
Reachable exactly as you said: `COALESCE(c_int, c_dec)` with a
`DECIMAL(38,0)` second argument types as `INT` and emitted `5`.
The helper now truncates towards zero per numeric family and range-checks
the truncated value:
- `BigDecimal` and `BigInteger` through `toBigInteger()` and a `BigInteger`
comparison against the int bounds.
- `Double` and `Float` reject `NaN` and the infinities explicitly, then
narrow to `long`.
- `Byte`, `Short`, `Integer`, `Long` keep the `long` check, which is exact
for them.
The infinity check is strictly redundant, since narrowing an infinity to
`long` saturates outside the int range anyway, but I kept it so the intent is
readable rather than implied by two conversions.
## Deviation 1: not `intValueExact()`
`intValueExact()` throws on a fractional part as well as on overflow.
`CAST(5.7 AS INT)` returns `5` today, so adopting it would change fraction
handling alongside range handling. It would also leave `BigDecimal` rejecting
`5.7` while `Double` still truncated it, which replaces one source-type
inconsistency with another.
So this PR keeps truncation and changes only range behaviour.
## Deviation 2: truncate first, then range-check
Your Double/Float guidance was to compare the `double` against the bounds
**before narrowing**. I do the opposite, because comparing first rejects a
fractional value whose truncation is in range. My first attempt followed the
wording literally and had that bug:
```
CAST(2147483647.5 AS INT) today: 2147483647
compare-before-narrow: error <- wrong
truncate-then-check: 2147483647
```
`-2147483648.5` and the `BigDecimal` equivalents behaved the same way.
`testCastAsIntTruncatesBeforeRangeCheckingAFractionalSource` pins all four,
plus the four one-step-beyond values that must still fail, and there is a
comment at the line so the order does not get "simplified" back later.
## Issues 2, 3 and 4
**Issue 2.** Both reference pages are cut to the current contract and scoped
to the types that enforce it, so `BIGINT` is no longer covered by the sentence.
The history is gone from the function reference and stays in
`incompatible-changes.md`, which now also records the wide-numeric and `NaN`
cases, the unchanged fraction behaviour, and that `BIGINT` is not range-checked
by this change.
**Issue 3.** The message is now `Value %s cannot be converted to %s: out of
range [%d, %d]`, from a small `outOfIntRange` helper whose javadoc records why
it does not say `CAST`. The redundant `(long)` casts are gone. I left
`UNSUPPORTED_OPERATION` as you suggested, since it matches the neighbouring
branches; happy to introduce a dedicated code if you would rather, though that
felt like a wider change than one branch warrants.
**Issue 4.** Javadoc is now the contract plus `@throws TransformException`
and a pointer to #12571.
## Three questions
1. **Fractions.** You asked me to decide explicitly, so: this PR keeps
truncation, which still differs from the string path where
`Integer.parseInt("5.7")` fails. Would you prefer them aligned, with `CAST(5.7
AS INT)` failing? I did not do it because it is a second behaviour change on
top of the range one, but if you want it I would rather do it here than leave
the inconsistency documented and open.
2. **`BIGINT` has the same defect.** Your Issue 2 made me check it, and it
does, which I had not realised:
| input to `CAST(... AS BIGINT)` | result on `dev` today |
| --- | --- |
| `BigDecimal` 2^64+5 | `5` |
| `BigDecimal` 2^64 | `0` |
| `Double` `NaN` | `0` |
| `Double` `1e30` | `9223372036854775807` |
The same helper shape would fix it. I left it out because the agreed
scope on #12571 was `TINYINT`, `SMALLINT` and `INT`, and because it widens the
behaviour change again. Do you want it folded in here, or as a separate PR
against a separate issue?
3. **Scope check.** With `BIGINT` excluded, the incompatible-changes entry
has to say so, which rather invites "why not". If your answer to 2 is "separate
PR", I will file the issue straight away and link it from the entry so it does
not read as an oversight.
## Verification
`seatunnel-transforms-v2` is green at 1169 tests, reproduced across repeat
runs, `spotless:check` passes. Two mutation checks: reverting to the plain
`longValue()` widening fails
`testCastAsIntRejectsWideAndNonFiniteNumericSources` on `BigDecimal` 2^64+5,
and comparing the raw double instead of the truncated one fails the new
boundary test on `2147483647.5`.
--
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]
