SEPURI-SAI-KRISHNA commented on issue #12571: URL: https://github.com/apache/seatunnel/issues/12571#issuecomment-5924419217
Thanks @DanielLeens and @SEZ9. Taking option 1, scoped as Daniel described: integral targets only, the source types the planner already accepts, both `CAST` failing and `TRY_CAST` returning null, and floating point left out as a separate decision. Here is the focused matrix before the PR. Everything below is measured on `dev` at `53733a112c9e935c5d97f7f24be249caa43b4fd5` by running the SQL through `ZetaSQLEngine`, not read off the source. **Which sources the planner accepts per integral target.** I enumerated this rather than assuming it from `CastFunction`: | target | TINYINT | SMALLINT | INT | BIGINT | STRING | | --- | --- | --- | --- | --- | --- | | `TINYINT` | accept | reject | reject | reject | accept | | `SMALLINT` | accept | accept | reject | reject | accept | | `INT` | accept | accept | accept | accept | accept | So within the agreed scope there is exactly one numeric narrowing reachable today: `BIGINT -> INT`. Every other accepted numeric pair is widening or identity. I will still put the range check at the shared boundary for all three integral targets, so it is correct if the planner later accepts more sources, but only the `BIGINT -> INT` row changes behaviour now. **`BIGINT -> INT` boundaries, current behaviour.** Sign flips at both ends: | input | `CAST` today | `TRY_CAST` today | same value from STRING today | | --- | --- | --- | --- | | `-2147483649` | **`2147483647`** | **`2147483647`** | CAST error | | `-2147483648` | `-2147483648` | `-2147483648` | `-2147483648` | | `0` | `0` | `0` | `0` | | `2147483647` | `2147483647` | `2147483647` | `2147483647` | | `2147483648` | **`-2147483648`** | **`-2147483648`** | CAST error | | `3000000000` | **`-1294967296`** | **`-1294967296`** | CAST error | `INT_MIN - 1` becoming `+2147483647` is the one I would point at: the value is negative, the result is the largest positive int, and nothing reports it. **After the change**, the four bold cells become `CAST` error and `TRY_CAST` null, matching the STRING column exactly. The two exact-boundary rows and the `0` row stay as they are. **STRING boundaries, which already behave correctly and must not regress:** | target | MIN-1 | MIN | MAX | MAX+1 | | --- | --- | --- | --- | --- | | `TINYINT` | error / null | `-128` | `127` | error / null | | `SMALLINT` | error / null | `-32768` | `32767` | error / null | | `INT` | error / null | `-2147483648` | `2147483647` | error / null | Cells read `CAST` / `TRY_CAST`. **Widening and identity, unchanged and asserted as controls:** `TINYINT -> SMALLINT/INT/BIGINT`, `SMALLINT -> INT/BIGINT`, `INT -> BIGINT`, and the identity casts `TINYINT -> TINYINT`, `SMALLINT -> SMALLINT`, `INT -> INT`, `BIGINT -> BIGINT` all return their input today and will be pinned so they still do. On @SEZ9's point about `INT -> SMALLINT` and `INT -> TINYINT` being refused at plan time while the same targets accept `STRING`: I agree it reads as a gap rather than deliberate protection, but Daniel asked to keep the first change to the sources the planner already accepts, so I am leaving it out of this PR. Happy to do it as a follow-up with the same range checks applied, if you both want it. Two notes on scope, so the PR matches what you asked for. The implementation goes in `SystemFunction.castAs`, which is the shared cast boundary and not connector specific. The `FLOAT` cases returning `Infinity` are excluded; I will file them separately so the compatibility decision there is not buried in this one. I will also update `docs/en` and `docs/zh` in the same PR, since this changes behaviour for jobs that rely on wraparound. If that matrix looks right I will open the PR against it. -- 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]
