SEPURI-SAI-KRISHNA commented on PR #11721:
URL: https://github.com/apache/seatunnel/pull/11721#issuecomment-5424738916
Follow-up filed as requested: #11976
@SEZ9, that is the paper trail you asked for, and it covers the
`Integer.MIN_VALUE` unit test you specified, plus an equivalence test at every
migrated site so the refactor is provably behaviour-preserving rather than
merely reviewed as such. @DanielLeens, this is the item you named as the last
one outstanding.
Two things surfaced while writing it up that are worth stating here rather
than only in the issue, because they changed what I am proposing.
**There are four spellings of this idiom in `dev`, not two.** Beyond the
`Math.abs` form this PR removes and the 12 sites I listed earlier,
`connector-file` already has a shared, guarded helper,
`FileSourceDocumentRouting.routeBucket`, which uses `Math.floorMod` with an
explicit `routeParallelism <= 0` check. That is a useful precedent for
placement and shape, and I would not have proposed a competing helper had I not
found it.
**Masking and `floorMod` are not the same function.** They agree only when
the divisor is a power of two:
```
h = Integer.MIN_VALUE n=3 mask=0 floorMod=1 DIFFER
h = -5 n=3 mask=0 floorMod=1 DIFFER
h = -5 n=4 mask=3 floorMod=3 same
```
Both land in `[0, n)`, so both fix the crash, but they do not route to the
same bucket. That matters because the obvious cleanup ("standardise on
`Math.floorMod`") would silently reshuffle split ownership at nine source
enumerators. For an ephemeral sink queue that is harmless; for split assignment
across an upgrade it is a behavioural change, and the project treats those as
needing deliberate handling.
So the issue proposes the **masking** semantics for the shared helper,
precisely because 9 of the 12 sites already use it and migrating them is then
byte-for-byte behaviour-preserving, a real refactor rather than a rename over a
semantics change. `routeBucket` stays as it is.
None of this affects the diff under review here, which remains the one-line
fix plus its regression test.
--
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]
