SEPURI-SAI-KRISHNA commented on PR #12215: URL: https://github.com/apache/seatunnel/pull/12215#issuecomment-5595339860
Hi @zhangshenghang, thanks for picking this up. Flagging an overlap before either of us spends more time on it: #12182 also fixes #12179, and was opened on 09-07 at 15:12 UTC, about 23 hours before this one. It is cross-referenced on the issue. The production change is effectively identical in both. Same two `instanceof` branches, in the same position ahead of the `Integer` check in `NumericFunction.mod`, returning `res[1].byteValue()` and `res[1].shortValue()`. I think that is a good sign rather than a problem: two people reading the same code independently arrived at the same shape, which is decent evidence it is the right one. The difference is in what the tests pin down. #12182 also: - documents at the call site why no range check is needed, since `divideAndRemainder` bounds the remainder below the divisor in magnitude. That is the non-obvious part, and the thing a later reader is most likely to try to "fix" by adding a guard. - asserts the planner side as well as the runtime: that `typeMapping()` declares `BYTE_TYPE` and `SHORT_TYPE` for the two projections, and that `transformBySQL` then returns values of exactly those types. The bug was a disagreement between `ZetaSQLType` and the runtime, so pinning both halves against each other is what stops half of it regressing on its own. - covers `Byte.MIN_VALUE` and `Short.MIN_VALUE` divisors, which are the widest of each type and the inputs that would overflow first if the no-range-check reasoning were wrong. It also has a review from @DanielLeens with no blockers. I have no attachment to whose PR lands, only to us not carrying two. Would you be willing to close this one in favour of #12182? If you would rather yours land, I am glad to close #12182 instead, though I would ask that the three points above come across, since the min-value cases and the `typeMapping` assertion are what would catch a regression in either half of the contract. For what it is worth, both PRs are currently red on e2e rather than on unit tests. #12182's only remaining failure is `TestFilterRowKindIT`, which is the known flake tracked in #12116. -- 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]
