SEPURI-SAI-KRISHNA opened a new pull request, #12899: URL: https://github.com/apache/seatunnel/pull/12899
Yes, and thank you for maintaining it: this is a contribution to Apache SeaTunnel. ### Scope agreed before submission Per this repository's contribution workflow, the three questions were put to the developer and answered before this went up: - **Issue and acceptance criteria**: #12612, scoped as agreed with @SEZ9 on that issue. `BigDecimal` and `BigInteger` range-checked against `Long.MIN_VALUE`..`Long.MAX_VALUE` rather than relying on `longValue()` truncation; `NaN` and both infinities rejected; the exact integral wrappers passed through unchanged; fraction handling untouched. Tests for 2^64+5, 2^64, `NaN` and 1e30, plus the exact long boundaries and one step beyond each, plus an in-range `BigDecimal` control. - **Modules and compatibility**: only `seatunnel-transforms-v2`. No option, public API, SPI, serialization or checkpoint surface changes. The helpers involved are all private; the rename is described at the end of this body. - **Required validation**: the full module unit run plus a mutation matrix and `spotless:check`, matching what #12605 did for `INT`. No E2E is added, for the reason given below. ### Purpose of this pull request Closes #12612. `CAST(... AS BIGINT)` silently wrapped a numeric value that `BIGINT` cannot represent, the same defect #12605 fixed for `INT`. The `BIGINT` | `LONG` numeric branch converted with `((Number) v1).longValue()`, which keeps only the low-order 64 bits of a `BigDecimal` or `BigInteger`, maps `NaN` to `0`, and saturates an out-of-range `Double`. Measured on `dev`: | input | before | after | | --- | --- | --- | | `BigDecimal` `18446744073709551621` (2^64+5) | `5` | fails | | `BigDecimal` `18446744073709551616` (2^64) | `0` | fails | | `Double` `NaN` | `0` | fails | | `Double` `1e30` | `9223372036854775807` | fails | ### One thing in the agreed test list turned out not to be achievable The review on #12612 asked for all four inputs under `CAST` (expects failure) and `TRY_CAST` (expects `NULL`). The `TRY_CAST` half cannot be written, and nor can the `CAST` half: `CastFunction.LONG_CAST_TYPES` admits only `TINYINT`, `SMALLINT`, `INT`, `BIGINT` and `STRING` as sources for a `BIGINT` target. All four integral families widen to a long exactly, and a `STRING` already reported its own overflow through `Long.parseLong`. So **no explicit cast of any kind can carry these four values to this branch**: `CAST(decimal_col AS BIGINT)` and `TRY_CAST(double_col AS BIGINT)` are both refused while the statement is prepared, with `Unsupported CAST FROM ... AS type: BIGINT`. `TRY_CAST` catches per-row failures, and that path is never reached. The route that does reach it, named in the issue body, is `COALESCE` and `IFNULL`, whose result type is inferred from the first non-null argument rather than the widest. So `COALESCE(bigint_col, decimal_col)` targets `BIGINT` and narrowed the decimal silently. Both facts are pinned by tests rather than left as prose: `testBigintTargetCannotBeReachedByAnExplicitCastFromDecimalOrDouble` asserts the plan-time rejection for all four cast forms, so if that type list is ever widened the claim fails loudly. `TRY_CAST` returning `NULL` is covered where it genuinely works, an overflowing `STRING` source, which was the one integral target missing from the existing string-source matrix. ### The fix The per-family range check now runs for the long bounds through the same helper as `INT`, not a copy of it. The bounds live in a small `IntegralBounds` enum carrying both the primitive and `BigInteger` forms, so neither is allocated per row. One case needed care. A narrowing cast to long saturates rather than overflowing, and `Long.MAX_VALUE` is 2^63-1, which no double can represent. So a double magnitude at or beyond 2^63 arrives as `Long.MAX_VALUE` and would pass a long range check: that is exactly how `1e30` became `9223372036854775807`. The guard rejects before the cast. Its asymmetry is deliberate and tested: `-2^63` is exactly `Long.MIN_VALUE`, so it is representable and still converts, while `+2^63` is one above the bound and is rejected. Fraction handling is untouched, as in #12605: a value reaching `BIGINT` through `COALESCE` or `IFNULL` is still truncated towards zero. ### Does this PR introduce any user-facing change? Yes, and it is documented in `docs/en` and `docs/zh` `incompatible-changes.md` plus the `CAST` note in `sql-functions.md`. A job that relied on the wraparound now fails at the offending row instead of writing a wrong number. This change also corrects one bullet in #12605's own entry, which stated that `BIGINT` keeps its existing conversion and is not range-checked. With both changes on `dev` that sentence would have been false. ### How was this patch tested? Checks run locally on the rebased head, per this repository's validation requirements: - `./mvnw spotless:apply` then `spotless:check`: clean - `./mvnw -q -DskipTests verify -pl seatunnel-transforms-v2 -am`: exit 0, which covers the changed module and its dependency closure rather than the whole repository - `./mvnw test -pl seatunnel-transforms-v2`: **1418 tests, 0 failures, 0 errors** Seven new tests, four in `SystemFunctionTest` and three in `ZetaSQLEngineTest`. Mutation matrix, to show which tests carry weight rather than asserting that they do: | mutation | tests that fail | | --- | --- | | `BIGINT` branch restored to `longValue()` (the original defect) | 5 | | the 2^63 saturation guard removed | 4, on `1e30` and `2^63` | | both bounds made exclusive | 8, of which 5 are the `INT` tests #12605 put on `dev` | The third row is the evidence that the helper is shared rather than duplicated: one changed comparison breaks both targets' tests. Stated plainly, two of the seven new tests do **not** catch the original defect and pass against it by design: the string-source matrix gap-fill, because that route already worked, and the plan-time rejection pin, because it documents why the `TRY_CAST` cases above cannot exist. Refactoring note: the helpers are now `narrowToIntegral`, `fromExact`, `checkRange` and `outOfRange`, rather than the `numberToInt` / `intFromExact` / `outOfIntRange` names I referred to on the issue. Same code, parameterized by target instead of hard-coded to `INT`. No E2E test is added. `TestSQLIT` and `func_system.conf` already drive `cast(id as INT)`, `coalesce` and `ifnull` through a real job, so they exercise the shared helper unchanged; every `BIGINT` narrowing case is a failure case, which cannot go in a config whose `Assert` sink requires the job to succeed, and an in-range case would need a new `DECIMAL` source column while pinning nothing the tests above do not. #12605 shipped the identical change for `INT` with no E2E change. Happy to add one if a reviewer would rather have it. ### Check list * [x] If any new Jar binary package adding in your PR, please add License Notice according [New License Guide](https://github.com/apache/seatunnel/blob/dev/docs/en/developer/new-license.md) * [x] If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs * [x] If necessary, please update `incompatible-changes.md` to describe the incompatibility caused by this PR. * [x] If you are contributing the connector code, please check that the following files are updated: -- 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]
