DanielLeens commented on PR #12215: URL: https://github.com/apache/seatunnel/pull/12215#issuecomment-5601135325
Thanks for flagging this, @SEPURI-SAI-KRISHNA, and for the very thorough write-up. Since I reviewed both PRs independently before either of you cross-referenced them, here's an honest side-by-side from that vantage point. **Both fixes are correct and equivalent in production code.** I traced `NumericFunction.mod()`'s dispatch ladder for both PRs separately and they land on the same two `instanceof Byte` / `instanceof Short` branches, in the same position, returning `res[1].byteValue()`/`res[1].shortValue()`, matching the established `abs()`/`sign()` precedent from #11937. There's no meaningful difference in the actual bug fix. **Where they differ is test coverage, and #12182 is ahead here** — this maps to something I flagged as Issue 1 (Low severity) on this PR: - #12182 asserts both halves of the contract that broke: `typeMapping()` returning `BYTE_TYPE`/`SHORT_TYPE` (planner side) *and* `transformBySQL` returning values of exactly those runtime types (execution side). This PR's test only covers the execution side. - #12182 covers `Byte.MIN_VALUE`/`Short.MIN_VALUE` divisors — the widest-magnitude inputs, which are exactly the ones that would expose a bug in the "no range check needed" reasoning if it were wrong. - #12182 documents at the call site *why* no range check is needed (`divideAndRemainder` bounds `|remainder| < |divisor|`), which is the non-obvious part a future reader is most likely to second-guess. **On CI**, both are currently red for the same reason, and it isn't either PR's fault. I checked #12182's fork run: it's down to one flaky job, `transform-v2-it-part-1 (11)` — `TestFilterRowKindIT.testFilterRowKindMultiTable`, the known static-JVM-counter flake tracked in #12116 (filed by @davidzollo), reproduced twice now on that PR's head across two different Flink legs, unrelated to any SQL/transform code either of you touched. I just checked this PR's fork run too: same test, same symptom, now failing on *both* JDK 8 and JDK 11 legs of `transform-v2-it-part-1`, plus two more unrelated failures in `doris-connector-it` and `rocketmq-connector-it` — modules neither PR touches, almost certainly more collateral noise from the same class of environmental flake. Neither PR's own diff is implicated by anything currently red. So, my honest read: technically I'd lean toward consolidating on #12182, mainly because of the min-value edge cases and the planner-side assertion, which are exactly the coverage gap I called out here. That said, I don't have merge authority on this repo (comment-only reviewer), and this is really @zhangshenghang's and @SEPURI-SAI-KRISHNA's call to make together — I just wanted to give you both an evidence-based answer rather than leave it unresolved. If #12215 stays open instead, porting over the `Byte.MIN_VALUE`/`Short.MIN_VALUE` test cases and the range-check comment from #12182 would close the only gap I found here, and either PR would then be in equally good shape to merge once #12116 is fixed or ruled non-blocking. -- 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]
