SEZ9 commented on PR #12605: URL: https://github.com/apache/seatunnel/pull/12605#issuecomment-5988315333
Thanks @SEPURI-SAI-KRISHNA for the follow-up and the measured table — the `5.0` vs `5` behaviour being decided by the string form rather than the fraction is exactly the subtlety the doc needed to capture. **Docs wording.** The wording you quoted for `c5b287cc5` — naming `TINYINT`, `SMALLINT`, `BYTE` and `INT` | `INTEGER` explicitly, with `CAST(3000000000 AS INT)` as the example and `BIGINT` deliberately absent — reads as resolving the overclaim on both pages, and linking the `BIGINT` gap from the incompatible-changes entry so it doesn't read as an oversight is the right call. I'll check the diff to confirm and then mark it resolved. **Javadoc rationale paragraph.** Fine to keep the TINYINT/SMALLINT rationale. My only remaining ask is on the contract sentence itself, which ties into the next point. **`numberToInt` and the `longValue()` contract.** Still open. `Number.longValue()` on a `BigDecimal`/`BigInteger` source silently wraps for values beyond the range of a long, so the "guaranteed not to have wrapped" wording in the Javadoc isn't true on the COALESCE/IFNULL path this PR brings into scope. Two acceptable ways to close it: - use an exact conversion for those sources and let the out-of-range case fall into the same range error, or - if you'd rather keep the narrower fix, reword the Javadoc to state the real precondition (the caller already has a value within long range) rather than a guarantee the method doesn't enforce. Either is fine; please just say which one you went with. A small test with a `BigDecimal` beyond the long range flowing through COALESCE would pin it. **Error message / error code.** Also still open: the message always says "CAST of ... to INT" even when the narrowing was triggered by COALESCE/IFNULL, and `UNSUPPORTED_OPERATION` is a bit misleading for a per-row data-range failure. Could you either pass the operation name into the message or make it neutral (e.g. "value ... is out of range for INT"), and consider a data-oriented error code if one is already available in the transform error set? If `UNSUPPORTED_OPERATION` is the established convention for this class of failure, point me at the precedent and I'll accept it. Once those two are addressed I'm happy to approve. <!-- streview-comment:1531 --> -- 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]
