SEPURI-SAI-KRISHNA commented on PR #11987: URL: https://github.com/apache/seatunnel/pull/11987#issuecomment-5438147537
Thanks for both comments, and for the concrete example. You asked for my thoughts directly, so here they are. **On readability, you are right** I am not going to argue that `HashUtils.nonNegativeMod(splitId.hashCode(), numReaders)` reads better than `(splitId.hashCode() & Integer.MAX_VALUE) % numReaders`. It does not. At that call site the change trades one line of self-contained arithmetic for one line plus a jump into a utility, and for a reader who already knows the idiom that is a small net loss. That objection is fair and it applies to most of the sites in this PR. **What I would put on the other side of the scale** Only one thing, and it is empirical rather than a general appeal to DRY: the idiom is not as universally known as any single call site makes it look. It currently appears in three spellings across the tree. Nine sites use `& Integer.MAX_VALUE`, three use `0x7FFFFFFF`, and one uses `Math.abs(hash) % n`, which is simply wrong because `Math.abs(Integer.MIN_VALUE)` overflows and the index stays negative. That last one is the live bug in `MultiTableSinkWriter` being fixed in #11721. So the question I would ask is not "is this line more readable" but "how did a wrong spelling get written and reviewed in the first place". My read is that it happened because there was no canonical thing to call and no place where the reasoning was written down. That is what the helper is really buying, and it is also why the Javadoc and `testNotEquivalentToFloorModForNonPowerOfTwo` matter more than the call-site migrations do. I take your point that this does not guarantee nobody writes the raw spelling again. It does not. It only makes the correct one nameable, which is a precondition for ever enforcing it in review or with a static check, not a guarantee on its own. **Where I land** I think the honest summary is that this PR trades a small, real, per-call-site readability cost for a smaller chance of repeating a bug the project has already hit once. That is a genuine judgement call and I do not think you are wrong to weigh it the other way. So I am happy either way: 1. Keep it as is. 2. Close it, and I will instead put the `floorMod` warning as a short comment at the fix site in #11721. That addresses your objection fully and still records the one piece of reasoning that is currently written down nowhere. You have the better read on what this codebase should carry long term, so tell me which you prefer and I will do that. #11721 carries the actual bug and does not depend on this landing, so closing costs nothing. -- 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]
