SEPURI-SAI-KRISHNA commented on issue #12571: URL: https://github.com/apache/seatunnel/issues/12571#issuecomment-5945212718
PR is up: #12605. First, your count correction is right and mine was wrong: six cells change, not four. `-2147483649`, `2147483648` and `3000000000` each flip for both `CAST` and `TRY_CAST`, so three rows times two columns. The exact-boundary rows and `0` stay as they are. One deviation from your wording that I want to put in front of you rather than bury in the diff. You asked for the range check at the shared integral cast boundary covering `TINYINT`, `SMALLINT` and `INT`. I built that first, then measured what it actually did and backed most of it out. `TINYINT`, `SMALLINT` and `BYTE` convert with `Byte.parseByte` and `Short.parseShort`, which already reject an out-of-range value, so they never had the defect. Routing them through a shared numeric helper would have made them *looser*, not stricter: | | before | with the shared helper | | --- | --- | --- | | `Double 5.7` to `TINYINT` | `NumberFormatException` | `5` | | `Double 5.7` to `SMALLINT` | `NumberFormatException` | `5` | | `Double 5.7` to `INT` | `5` | `5` | So the PR changes one line: the numeric branch of the `INT` case. That is the only path that ever wrapped. If you would still rather see a single shared helper across all the integral targets, say so and I will add it with an explicit integrality check so the fractional behaviour above does not move. Two other things worth knowing before review. `COALESCE` and `IFNULL` reach the same `castAs` boundary, and `ZetaSQLType` infers their result type from the first non-null argument rather than the widest. So `COALESCE(int_col, bigint_col)` targets `INT`, and an out-of-range value from the `BIGINT` argument was truncated in exactly the same silent way. Measured on pristine `dev`: `-1294967296` before, `TransformException` after. It is the same defect in another expression, so I left it fixed and pinned it with a test rather than special-casing `castAs` to keep `COALESCE` wrapping. It is also recorded in the incompatible-changes entry. `CASE` reaches that boundary too, but infers the widest branch type, so it cannot narrow. I checked both branch orders; both widen to `BIGINT`. There is a test pinning that so a later change to the inference does not quietly start failing `CASE`. On your second note, direct numeric coverage for `TINYINT` and `SMALLINT`: that is a good catch and I have added it, though it ends up documenting why those two are left alone rather than covering a new check. `SystemFunctionTest` now calls `SystemFunction.castAs` directly with out-of-range numeric input for every integral target: - `testCastAsIntRejectsOutOfRangeNumericInput` for the path this PR changes, including `Long.MAX_VALUE` and both exact boundaries. - `testCastAsTinyintAndSmallintAlreadyRejectOutOfRangeNumericInput` for the two targets that already reject, with `300L`, `70000`, `Long.MAX_VALUE`, and the fractional case that a shared helper would have started accepting. Everything else you asked for is in: the complete `MIN-1` / `MIN` / `MAX` / `MAX+1` matrix for all three integral targets, the exact `BIGINT -> INT` boundaries, the widening and identity controls, both language references, and the incompatible-changes record. `FLOAT` and `Infinity` are out, and I will file those separately. `seatunnel-transforms-v2` is green at 1167 tests. Reverting the single changed line fails three of the new tests, including `TRY_CAST` returning `2147483647` where it should be null. Agreed this should not be treated as fixed until tests and review are complete. -- 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]
