DanielLeens commented on PR #12007: URL: https://github.com/apache/seatunnel/pull/12007#issuecomment-5476963785
Thanks for the deep independent pass, @SEZ9 — really useful to have a second set of eyes on this file. Went through all eight points against the current head (`034a3f47`) and want to reconcile them with my own review above rather than just say "thanks": **Corroborates my Issue 2 (SMALLINT overflow-wrap unguarded) — your Issues 6 and 7.** Same finding, independently reached: the new `case "BYTE"` in `convertTo()` range-checks before narrowing, but the adjacent `case "SHORT"`/`case "INTEGER"` still narrow unchecked, so `ROUND(smallint_col, -1)` on `32767` silently wraps to a negative value instead of throwing. Two independent reviewers landing on the identical `NumericFunction.java:324` vicinity is good confirmation this needs fixing regardless of which PR ends up carrying the fix (more on that below). **Corroborates my Issue 3 (missing `incompatible-changes.md` entry) — your Issues 2 and 5.** Agreed on the underlying gap (the new `default: throw` in `round()`'s switch, and the `TINYINT` pass-through-to-rounded change generally, are real behavior breaks on upgrade), though I'd fold your two into the same numbered issue as mine rather than count them separately — they're the same "needs a documented breaking-change entry" ask, just illustrated with different call sites. **I'd push back on Issues 1 and 3 (ZetaSQLType mismatch), with evidence.** I checked `ZetaSQLType.java` at this head before agreeing this was a gap, and I don't think it is one: - `ABS`/`ROUND`/`CEIL`/`CEILING`/`FLOOR`/`TRUNC`/`TRUNCATE` all fall into the same `case` block at `ZetaSQLType.java:463-473`, which returns `getExpressionType(function.getParameters().getExpressions().get(0))` — i.e. "the type of whatever the first argument's schema type already is." There's even an existing code comment right above it explaining why: *"These functions all return the type of their first argument... declaring INT/DOUBLE for them would truncate BIGINT and DECIMAL results."* For a `TINYINT` column, `getExpressionType()` already resolves to `BasicType.BYTE_TYPE`, which is exactly what this PR's `abs()`/`round()` now return at runtime (`(byte) Math.abs(...)`, `column.byteValue()`). No mismatch — this dispatch was already generic before this PR, unlike #11712's BIGINT/DECIMAL fix, which needed a `ZetaSQLType` change for a different reason (that one wasn't a pass-through case). - `SIGN` is hardcoded at `ZetaSQLType.java:376` to `BasicType.INT_TYPE`, independent of the argument type. This PR's `sign()` still returns `Integer.signum(...)` (an `Integer`) for the new `Byte`/`Short` branches, and the `BigDecimal` branch's `signum()` also returns `int`/`Integer`. So the declared type (`INT`) and the runtime type (`Integer`) still agree. So I don't think `ZetaSQLType.java` needs a change here — happy to be shown a concrete failing case if I've missed one, but I checked both the pass-through functions and `SIGN` specifically and both line up. **I'd also push back on Issue 4 (missing docs), with a path correction.** The file doesn't exist at `docs/transform-v2/sql-functions.md` — the real path is `docs/en/transforms/sql-functions.md` (and its `docs/zh` counterpart), which is the same file I checked in my own review. Its `ABS` section (line ~413) already reads: *"Note that TINYINT, SMALLINT, INT, and BIGINT data types cannot represent absolute values of their minimum negative values... It leads to an exception."* — that already documents both the TINYINT/SMALLINT support and the exact `ABS(Byte.MIN_VALUE)` failure mode you flagged, and it predates this PR (this PR's diff doesn't touch `docs/`). `ROUND`'s section similarly already says "same type as argument" generically. So I don't think this file needs an update either — the only doc gap I found is the `incompatible-changes.md` one above. **Issue 8 (deprecated error code) — fair, taking it as a reasonable low-severity addition.** No disagreement there. One thing your review doesn't touch on that I'd like your read on: the bigger-picture Issue 1 from my own review — this PR duplicates the scope of the already twice-approved `#11937` (opened 8 days earlier, same file, same methods, stacked on `#11927`'s shared `checkIntegralRange` helper which already range-checks `BYTE`/`SHORT`/`INTEGER`/`LONG` uniformly — which would resolve your/my SMALLINT-overflow finding in one shot — plus it already has an `incompatible-changes.md` entry and a `Locale.ROOT` fix this PR doesn't have). Given that, I'd rather we get the author and maintainers to settle which PR moves forward before we keep stacking incremental fixes onto this one — several of the open items here (SMALLINT overflow guard, incompatible-changes.md doc) are already solved on the other PR. Would appreciate your take on that before we ask for more changes here. -- 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]
