Copilot commented on code in PR #12351:
URL: https://github.com/apache/gluten/pull/12351#discussion_r3465122660
##########
gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java:
##########
@@ -201,6 +201,7 @@ public boolean ringContain(long slot) {
private boolean add(T node) {
boolean added = false;
if (node != null && !nodes.containsKey(node)) {
+ Preconditions.checkArgument(node.key() != null, "Node key must not be
null: %s", node);
Set<Partition<T>> partitions =
Review Comment:
`add()` checks `!nodes.containsKey(node)` before validating `node.key()`. If
a Node's `equals`/`hashCode` implementation depends on `key()` (common), a null
key can trigger an NPE inside `containsKey` before the intended
`IllegalArgumentException` fail-fast is thrown. Validate the key before any Map
lookup that may invoke `hashCode`/`equals`.
##########
gluten-core/src/main/java/org/apache/gluten/hash/ConsistentHash.java:
##########
@@ -236,7 +237,10 @@ public Partition(T node, int index) {
}
public String getPartitionKey() {
- return String.format("%s:%d", node, index);
+ // Hash the node by its logical key() so the ring is stable and
reproducible across JVMs.
+ // Avoid node.toString() (which may default to the identity hash) and
String.format (slow on
+ // this hot path, called per virtual node during add()).
+ return node.key() + ":" + index;
Review Comment:
`Partition.getPartitionKey()` recomputes `node.key() + ":" + index` each
time (and will call `key()` repeatedly inside the collision-retry loop). Since
the partition key is immutable, cache it once in the constructor to reduce
repeated `key()` calls and string allocations on this hot path.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
