[GitHub] [ignite-3] isapego commented on a diff in pull request #2302: IGNITE-19935 Allow combining individual colocation hashes [DRAFT]

[via GitHub]

isapego commented on code in PR #2302:
URL: https://github.com/apache/ignite-3/pull/2302#discussion_r1258583924


##########
modules/core/src/main/java/org/apache/ignite/internal/util/HashCalculator.java:
##########
@@ -170,8 +244,19 @@ public void appendNumber(BigInteger v) {
      * @param v Value to update hash.
      */
     public void appendUuid(UUID v) {
-        appendLong(v.getMostSignificantBits());
-        appendLong(v.getLeastSignificantBits());
+        hash = combine(hash, hashUuid(v));
+    }
+
+    /**
+     * Get value hash.
+     *
+     * @param v Value to hash.
+     */
+    public static int hashUuid(UUID v) {
+        int hash1 = hashLong(v.getMostSignificantBits());
+        int hash2 = hashLong(v.getLeastSignificantBits());
+
+        return combine(hash1, hash2);

Review Comment:
   Do we need to use `combine` here? I'd avoid using it where it is possible.
   ```suggestion
           int hash = HashUtils.hash32(v.getMostSignificantBits());
           hash = HashUtils.hash32(v.getLeastSignificantBits(), hash);
   
           return hash;
   ```



##########
modules/core/src/main/java/org/apache/ignite/internal/util/HashUtils.java:
##########
@@ -120,12 +105,50 @@ public static int hash32(int data, int seed) {
      * @param data The input long value.
      * @return The 32-bit hash.
      */
-    public static int hash32(long data, int seed) {
-        long hash = hash64(data, seed);
+    public static int hash32(long data) {
+        long hash = hash64(data, 0);
 
         return (int) (hash ^ (hash >>> 32));
     }
 
+    /**
+     * Combines two hash values.
+     *
+     * @param hash1 The first hash.
+     * @param hash2 The second hash.
+     * @return The combined 32-bit hash.
+     */
+    public static int combine(int hash1, int hash2) {
+        long hash = hash64(hash1, hash2);
+
+        return (int) (hash ^ (hash >>> 32));

Review Comment:
   Are there any reasons why it is implemented this way?
   Also, does `combine(a,b) == combine(b,a)`?
   More descriptive javadoc probably is needed here.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org