SEPURI-SAI-KRISHNA commented on PR #11987:
URL: https://github.com/apache/seatunnel/pull/11987#issuecomment-5442332173
Thanks, this is a much better shape than what I had. I have pushed all of it.
**1. Renamed to `bucketIndex`.**
I want to be clear that I do not think of this as a concession.
`nonNegativeMod` promises modular arithmetic, which invites exactly the
assumption that it should equal `Math.floorMod`. It does not, and that
difference silently reassigns partitions. `bucketIndex` promises a bucket index
and nothing more, so the wrong assumption never forms. Your name removes the
trap that my javadoc was working around. Also made the class `final` as in your
sketch.
**2. Added the `long` overload, and migrated the one call site it unblocks.**
You were right that the utility did not cover all existing variants. I went
back through the tree, and the gap reduces to exactly one site:
`ShardRouter#getShard` in connector-clickhouse, which spelled out `(int) ((hash
& Long.MAX_VALUE) % shardWeightCount)` inline over an xxHash64. That is your
overload verbatim, so it is now:
```java
int offset =
HashUtils.bucketIndex(
HASH_INSTANCE.hash(
ByteBuffer.wrap(
shardValue.toString().getBytes(StandardCharsets.UTF_8)),
0),
shardWeightCount);
```
Shard assignment is unchanged. There is a test asserting the helper matches
the previous inline spelling exactly, including `Long.MIN_VALUE`, because this
one routes user data and a behaviour change here would be silent.
The two overloads are documented as separate mappings, not a widening. A
64-bit hash and its truncation to 32 bits land in different buckets, so a call
site must not be moved between them. There is a test pinning that too.
**3. On `@Deprecated` for legacy algorithms: I do not think there is
anything left to deprecate.**
Here is the full inventory after this PR:
| Spelling | Sites remaining |
|---|---|
| `(h & Integer.MAX_VALUE) % n` | 0, all migrated |
| `(h & Long.MAX_VALUE) % n` | 0, clickhouse migrated in this PR |
| `Math.abs(h) % n` | 1, `MultiTableSinkWriter`, the original bug, being
fixed in #11721 |
| `Math.floorMod(h, n)` | 1, `FileSourceDocumentRouting` |
So once #11721 lands and that site moves onto the helper, the only surviving
spelling in the repo is `FileSourceDocumentRouting`. I would argue that one is
not fragmentation and should not be deprecated: `floorMod` over a SHA-256
digest is a genuinely different and correct function, and converting it would
change which reader owns which document. It is a different tool, not an older
version of this one.
If I have missed a variant, tell me which file and I will fold it in. But I
would rather not add `@Deprecated` methods with zero callers, since that is
dead code that implies a migration nobody needs to do.
**4. On making this the convention.**
This is the part of your point 1 that I think is genuinely unresolved, and
you are right that a utility class alone does not fix a review oversight.
`docs/en/developer/coding-guide.md` and `docs/zh/developer/coding-guide.md`
look like the right home for a short rule saying that hash-to-bucket routing
goes through `HashUtils` and that `floorMod` and masking are not
interchangeable. I did not add it here to keep the PR focused, but say the word
and I will push it in this PR rather than a follow-up.
Verified locally: all 17 touched files compile with spotless enforced,
`HashUtilsTest` 10/10, plus `JdbcSourceSplitEnumeratorTest` and
`BigtableSourceSplitEnumeratorTest` green.
--
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]
