SEPURI-SAI-KRISHNA commented on PR #12605: URL: https://github.com/apache/seatunnel/pull/12605#issuecomment-5965862542
Thanks. Taking the truncation first, then the three summaries, and flagging up front that two of them are not a plain "yes". **The cut sentence.** Nothing was lost after it; it ended there. In full: "and there is a comment at the line so the order does not get 'simplified' back later." That is the whole thought. This is the fourth time a comment of mine has arrived clipped on your end, so if a later one stops mid-sentence again, assume there is a short tail and ask. **PR12605-F2, the CAST note.** Partly. It is now reference documentation rather than a changelog, and the before/after history is gone from both language pages and lives only in `incompatible-changes.md`. The note reads: > Casting to `TINYINT`, `SMALLINT`, `BYTE` or `INT` | `INTEGER` throws a `TransformException` when the value is outside the target's range, for example `CAST(3000000000 AS INT)`. Use `TRY_CAST` to get `NULL` instead of an error. A fractional source is truncated towards zero rather than rejected. What it does **not** do is state positively that `BIGINT` still wraps. I scoped the sentence so `BIGINT` is no longer falsely covered, which fixes the overclaim you raised, but I did not add a warning about it. I left that out because it documents a defect in a reference page without a fix behind it, and because the `BIGINT` question I asked earlier is still open. Happy to add a sentence if you would rather the gap were explicit. **PR12605-F3, the error.** Half. The message is now: ``` Value %s cannot be converted to %s: out of range [-2147483648, 2147483647] ``` so it names the conversion rather than the `CAST` keyword, and the redundant `(long)` casts are gone. The error code is **still** `CommonErrorCodeDeprecated.UNSUPPORTED_OPERATION`. I did not move it, because your own suggested fix did not include it, you described it as a nit, and every neighbouring branch in `castAs` uses the same code, so introducing a distinct one for this single branch would make the file less consistent rather than more. If you do want a dedicated code for per-row data-range failures I think that is a good idea, but a better one as its own change covering all the branches rather than just this one. Say the word either way. **PR12605-F4, the Javadoc.** Yes. It is now the contract: what the method does, how each numeric family is handled, that truncation is unchanged, that `NaN` and the infinities are rejected, plus `@param`, `@return` and `@throws TransformException`, with a link to #12571 instead of the history. There is one paragraph of design rationale left, explaining why each family is checked before narrowing rather than after a single `longValue()` widening. That is not before/after narration, it is the reason the method is shaped the way it is, and it is the thing most likely to be "tidied" away by someone who has not read this thread. I would rather keep it, but I will cut it if you think it still reads as history. **Still open from my side:** whether `BIGINT` gets the same treatment. It has the identical defect (`BigDecimal` 2^64+5 arrives as `5`, `NaN` as `0`, `1e30` as `Long.MAX_VALUE`) and neither you nor @DanielLeens has said whether you want it folded in here or split out. My own preference is a separate PR against a separate issue, since the agreed scope on #12571 was `TINYINT`, `SMALLINT` and `INT`, but I will do whichever you prefer and I will file the issue immediately if it is to be separate. -- 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]
