SEZ9 commented on PR #12605:
URL: https://github.com/apache/seatunnel/pull/12605#issuecomment-5965700380
Thanks for the detailed write-up and for keeping the fix as a follow-up
commit rather than a force-push — that makes the delta much easier to follow.
**Issue 1 (PR12605-F1):** Good to see the reproduction confirmed:
`18446744073709551621` → `5`, `18446744073709551616` → `0`, and `NaN` → `0`
were exactly the wrapping cases I was worried about on the `COALESCE(c_int,
c_dec)` path. The per-family approach (`toBigInteger()` + `BigInteger` bounds
comparison for `BigDecimal`/`BigInteger`, explicit `NaN`/infinity rejection for
`Double`/`Float`, `long` check for the exact integral types) is the right
shape. Keeping the redundant infinity check for readability is fine by me.
**Deviation 1 (no `intValueExact()`):** Agreed. Changing fraction handling
alongside range handling would widen the behavioural change of this PR, and
you're right that it would leave `BigDecimal` and `Double` disagreeing on
`5.7`. Keeping truncation and only fixing range behaviour is the correct scope.
**Deviation 2 (truncate first, then range-check):** Also agreed — the
`2147483647.5` / `-2147483648.5` example is a convincing counter-case to my
original wording, and
`testCastAsIntTruncatesBeforeRangeCheckingAFractionalSource` pinning both the
in-range truncations and the one-step-beyond failures is exactly what I'd want
to prevent a future reordering. Thanks for the inline comment explaining the
order.
Remaining asks:
1. Your comment appears to be cut off mid-sentence at "so the order does
not" — could you finish that thought (or confirm nothing was lost) so the
record is complete?
2. You say all four points are addressed, but only Issue 1 is described.
Could you briefly summarise what changed for the other three so I can verify
against the follow-up commit:
- **PR12605-F2** – the revised CAST note in
`docs/en/transforms/sql-functions.md`: does it now describe the current
behaviour as reference documentation (INT/TINYINT/SMALLINT range-check, BIGINT
still wraps for wide numerics) rather than a changelog-style "before/after"?
- **PR12605-F3** – the error message: does it now reflect the actual
trigger (COALESCE/IFNULL vs. CAST) and has the error code moved off
`UNSUPPORTED_OPERATION` to something appropriate for a per-row data-range
failure?
- **PR12605-F4** – the `numberToInt` Javadoc: is it now a contract
description (input families, truncation semantics, range behaviour, what is
thrown) rather than history?
Once I have that summary I'll do a final pass over the follow-up commit.
<!-- streview-comment:1491 -->
--
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]
