Fix ResolvedKeySpacePath.equals and hashCode (#3591)

This fixes `ResolvedKeySpace.equals` and `hashCode` so that they follow
the spec and are consistent.
Notably there were a few issues:
- They did not correctly handle `KeyType.BYTES`
- They disagreed about what data to use (hashCode used to look at the
`PathValue`, while equals looked at the value)
- `PathValue` did not implement `equals`/`hashCode`
- The `remainder` was not included.
- KeySpacePathImpl redundantly checked the value from the directory
- It did not check the parent of the ResolvedKeySpacePath.

Fixes: #3594

Author: ScottDugas

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java

@@ -710,7 +710,8 @@ public Object getValue() {
         return value;
     }
 
-    protected static boolean areEqual(Object o1, Object o2) {
+    @SuppressWarnings("PMD.CompareObjectsWithEquals") // we use ref
+    protected static boolean areEqual(@Nullable Object o1, @Nullable Object o2) {
         if (o1 == null) {
             return o2 == null;
         } else {
@@ -719,6 +720,12 @@ protected static boolean areEqual(Object o1, Object o2) {
             }
         }
 
+        // Handle ANY_VALUE specially - typeOf does not support ANY_VALUE
+        boolean isAnyValue = (o1 == ANY_VALUE || o2 == ANY_VALUE);
+        if (isAnyValue) {
+            return Objects.equals(o1, o2);
+        }
+
         KeyType o1Type = KeyType.typeOf(o1);
         if (o1Type != KeyType.typeOf(o2)) {
             return false;
@@ -740,6 +747,31 @@ protected static boolean areEqual(Object o1, Object o2) {
         }
     }
 
+    protected static int valueHashCode(@Nullable Object value) {
+        if (value == null) {
+            return 0;
+        }
+
+        // Handle ANY_VALUE specially
+        if (value == ANY_VALUE) {
+            return System.identityHashCode(value);
+        }
+
+        switch (KeyType.typeOf(value)) {
+            case BYTES:
+                return Arrays.hashCode((byte[]) value);
+            case LONG:
+            case STRING:
+            case FLOAT:
+            case DOUBLE:
+            case BOOLEAN:
+            case UUID:
+                return Objects.hashCode(value);
+            default:
+                throw new RecordCoreException("Unexpected key type " + KeyType.typeOf(value));
+        }
+    }
+
     /**
      * Returns the path that leads up to this directory (including this directory), and returns it as a string
      * that looks something like a filesystem path.

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java

@@ -276,27 +276,21 @@ public boolean equals(Object obj) {
         }
         KeySpacePath that = (KeySpacePath) obj;
 
-        // Check that the KeySpaceDirectories of the two paths are "equal enough".
-        // Even this is probably overkill since the isCompatible check in KeySpaceDirectory
-        // will keep us from doing anything too bad. We could move this check into KeySpaceDirectory
-        // but comparing two directories by value would necessitate traversing the entire directory
-        // tree, so instead we will use a narrower definition of equality here.
-        boolean directoriesEqual = Objects.equals(this.getDirectory().getKeyType(), that.getDirectory().getKeyType()) &&
-                                   Objects.equals(this.getDirectory().getName(), that.getDirectory().getName()) &&
-                                   Objects.equals(this.getDirectory().getValue(), that.getDirectory().getValue());
+        // Directories use reference equality, because the expected usage is that they go into a
+        // singleton KeySpace.
+        boolean directoriesEqual = this.getDirectory().equals(that.getDirectory());
 
+        // the values might be byte[]
         return directoriesEqual &&
-               Objects.equals(this.getValue(), that.getValue()) &&
-               Objects.equals(this.getParent(), that.getParent());
+                KeySpaceDirectory.areEqual(this.getValue(), that.getValue()) &&
+                Objects.equals(this.getParent(), that.getParent());
     }
 
     @Override
     public int hashCode() {
         return Objects.hash(
-                getDirectory().getKeyType(),
-                getDirectory().getName(),
-                getDirectory().getValue(),
-                getValue(),
+                getDirectory(),
+                KeySpaceDirectory.valueHashCode(getValue()),
                 parent);
     }
 

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValue.java

@@ -24,13 +24,14 @@
 
 import javax.annotation.Nullable;
 import java.util.Arrays;
+import java.util.Objects;
 
 /**
  * A class to represent the value stored at a particular element of a {@link KeySpacePath}. The <code>resolvedValue</code>
  * is the object that will appear in the {@link com.apple.foundationdb.tuple.Tuple} when
  * {@link KeySpacePath#toTuple(com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext)} is invoked.
  * The <code>metadata</code> is left null by {@link KeySpaceDirectory} but other implementations may make use of
- * it (e.g. {@link DirectoryLayerDirectory}.
+ * it (e.g. {@link DirectoryLayerDirectory}).
  */
 @API(API.Status.UNSTABLE)
 public class PathValue {
@@ -69,4 +70,22 @@ public Object getResolvedValue() {
     public byte[] getMetadata() {
         return metadata == null ? null : Arrays.copyOf(metadata, metadata.length);
     }
+
+    @Override
+    public boolean equals(Object other) {
+        if (this == other) {
+            return true;
+        }
+        if (!(other instanceof PathValue)) {
+            return false;
+        }
+        PathValue that = (PathValue) other;
+        return KeySpaceDirectory.areEqual(this.resolvedValue, that.resolvedValue) &&
+                Arrays.equals(this.metadata, that.metadata);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(KeySpaceDirectory.valueHashCode(resolvedValue), Arrays.hashCode(metadata));
+    }
 }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java

@@ -229,13 +229,15 @@ public boolean equals(Object other) {
         }
 
         ResolvedKeySpacePath otherPath = (ResolvedKeySpacePath) other;
-        return this.inner.equals(otherPath.inner)
-               && Objects.equals(this.getResolvedValue(), otherPath.getResolvedValue());
+        return Objects.equals(this.getResolvedPathValue(), otherPath.getResolvedPathValue()) &&
+                Objects.equals(this.getParent(), otherPath.getParent()) &&
+                this.inner.equals(otherPath.inner) &&
+                Objects.equals(this.remainder, otherPath.remainder);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(inner, getResolvedPathValue());
+        return Objects.hash(inner, getResolvedPathValue(), remainder, getParent());
     }
 
     @Override

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java

@@ -110,11 +110,16 @@ public KeyTypeValue(KeyType keyType, @Nullable Object value, @Nullable Object va
             assertTrue(keyType.isMatch(value));
             assertTrue(keyType.isMatch(generator.get()));
         }
+
+        @Override
+        public String toString() {
+            return "KeyTypeValue{" + keyType + '}';
+        }
     }
 
-    private final Random random = new Random();
+    private static final Random random = new Random();
 
-    private final List<KeyTypeValue> valueOfEveryType = new ImmutableList.Builder<KeyTypeValue>()
+    private static final List<KeyTypeValue> valueOfEveryType = new ImmutableList.Builder<KeyTypeValue>()
             .add(new KeyTypeValue(KeyType.NULL, null, null, () -> null))
             .add(new KeyTypeValue(KeyType.BYTES, new byte[] { 0x01, 0x02 }, new byte[] { 0x03, 0x04 }, () -> {
                 int size = random.nextInt(10) + 1;
@@ -1224,12 +1229,6 @@ public TestWrapper1(KeySpacePath inner) {
         }
     }
 
-    private static class TestWrapper2 extends KeySpacePathWrapper {
-        public TestWrapper2(KeySpacePath inner) {
-            super(inner);
-        }
-    }
-
     @Test
     public void testListConstantValue() {
         // Create a root directory called "a" with subdirs of every type and a constant value
@@ -1493,6 +1492,65 @@ public void testPathCompareByValue() {
         assertEquals(p1.hashCode(), sameAsP1.hashCode(), "they have the same hash code");
     }
 
+    /**
+     * {@code KeySpaceDirectory}s are supposed to be inserted into a singleton {@link KeySpace}, thus we can use
+     * reference equality to do comparisons. This is particularly important for the efficiency of
+     * {@link KeySpacePathImpl#equals(Object)}, because we don't want it to have to re-compare all of the children of the
+     * directory as you go up through the parents. If using reference equality turns out to be problematic,
+     * we'll want to look at other solutions, such as ignoring the hierarchy, or something more tricky.
+     */
+    @Test
+    void testKeySpaceDirectoryEqualsUsesReferenceEquality() {
+        // Create two directories with identical properties
+        KeySpaceDirectory dir1 = new KeySpaceDirectory("test", KeyType.STRING, "value");
+        KeySpaceDirectory dir2 = new KeySpaceDirectory("test", KeyType.STRING, "value");
+
+        // KeySpaceDirectory.equals should use reference equality
+        assertEquals(dir1, dir1, "Directory should equal itself");
+        assertNotEquals(dir1, dir2, "Directories with same properties should not be equal (reference equality)");
+
+        // Test with different properties
+        KeySpaceDirectory dir3 = new KeySpaceDirectory("different", KeyType.LONG, 42L);
+        assertNotEquals(dir1, dir3, "Directories with different properties should not be equal");
+
+        // Test with null
+        assertNotEquals(dir1, null, "Directory should not equal null, and calling with null shouldn't error");
+
+        // Test with different object type
+        assertNotEquals(dir1, "not a directory", "Directory should not equal a different type");
+    }
+
+    @Test
+    void testKeySpaceDirectoryHashCodeFollowsReferenceSemantics() {
+        // Create two directories with identical properties
+        KeySpaceDirectory dir1 = new KeySpaceDirectory("test", KeyType.STRING, "value");
+        KeySpaceDirectory dir2 = new KeySpaceDirectory("test", KeyType.STRING, "value");
+
+        // Since equals uses reference equality, hashCode should be consistent with that
+        // (i.e., objects that are equal should have the same hashCode, but since these
+        // objects are not equal by reference, their hashCodes may differ)
+
+        // The same object should always have the same hashCode
+        int hashCode1 = dir1.hashCode();
+        assertEquals(hashCode1, dir1.hashCode(), "Same object should produce same hashCode");
+
+        // Different instances (even with same properties) may have different hashCodes
+        // We can't assert they're different, but we can verify the hashCode is stable
+        int hashCode2 = dir2.hashCode();
+        assertEquals(hashCode2, dir2.hashCode(), "Same object should produce same hashCode");
+
+        // Test that hashCode is consistent across multiple calls
+        for (int i = 0; i < 10; i++) {
+            assertEquals(hashCode1, dir1.hashCode(), "hashCode should be stable across calls");
+            assertEquals(hashCode2, dir2.hashCode(), "hashCode should be stable across calls");
+        }
+        // two difference references may have the same hash code, but eventually we should find a different one, even
+        // though all properties are the same
+        for (int i = 0; i < 100; i++) {
+            assertNotEquals(hashCode1, new KeySpaceDirectory("test", KeyType.STRING, "value").hashCode());
+        }
+    }
+
     private List<Long> resolveBatch(FDBRecordContext context, String... names) {
         List<CompletableFuture<Long>> futures = new ArrayList<>();
         for (String name : names) {

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java

@@ -0,0 +1,96 @@
+/*
+ * PathValueTest.java
+ *
+ * This source file is part of the FoundationDB open source project
+ *
+ * Copyright 2015-2025 Apple Inc. and the FoundationDB project authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.apple.foundationdb.record.provider.foundationdb.keyspace;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+/**
+ * Tests for {@link PathValue}.
+ */
+class PathValueTest {
+
+    /**
+     * Test data for PathValue equality tests.
+     */
+    static Stream<Arguments> equalPathValuePairs() {
+        return Stream.of(
+                Arguments.of("null values", null, null, null, null),
+                Arguments.of("same string values", "test", null, "test", null),
+                Arguments.of("same long values", 42L, null, 42L, null),
+                Arguments.of("same boolean values", true, null, true, null),
+                Arguments.of("same byte[] values", new byte[] {1, 2, 3}, null, new byte[] {1, 2, 3}, null),
+                Arguments.of("same metadata", "test", new byte[]{1, 2, 3}, "test", new byte[]{1, 2, 3})
+        );
+    }
+
+    /**
+     * Test data for PathValue inequality tests.
+     */
+    static Stream<Arguments> unequalPathValuePairs() {
+        return Stream.of(
+                Arguments.of("different string values", "test1", null, "test2", null),
+                Arguments.of("different long values", 42L, null, 100L, null),
+                Arguments.of("different boolean values", true, null, false, null),
+                Arguments.of("different types", "string", null, 42L, null),
+                Arguments.of("different metadata", "test", new byte[]{1, 2, 3}, "test", new byte[]{4, 5, 6}),
+                Arguments.of("one null metadata", "test", new byte[]{1, 2, 3}, "test", null),
+                Arguments.of("one null value", null, null, "test", null),
+                Arguments.of("different value with same metadata", "test1", new byte[]{1, 2, 3}, "test2", new byte[]{1, 2, 3})
+        );
+    }
+
+    @ParameterizedTest(name = "{0}")
+    @MethodSource("equalPathValuePairs")
+    void testEqualsAndHashCodeForEqualValues(String description, Object resolvedValue1, byte[] metadata1, 
+                                             Object resolvedValue2, byte[] metadata2) {
+        PathValue value1 = new PathValue(resolvedValue1, metadata1);
+        PathValue value2 = new PathValue(resolvedValue2, metadata2);
+        
+        assertEquals(value1, value2, "PathValues should be equal: " + description);
+        assertEquals(value1.hashCode(), value2.hashCode(), "Equal PathValues should have equal hash codes: " + description);
+    }
+
+    @ParameterizedTest(name = "{0}")
+    @MethodSource("unequalPathValuePairs")
+    void testNotEqualsForUnequalValues(String description, Object resolvedValue1, byte[] metadata1, 
+                                       Object resolvedValue2, byte[] metadata2) {
+        PathValue value1 = new PathValue(resolvedValue1, metadata1);
+        PathValue value2 = new PathValue(resolvedValue2, metadata2);
+        
+        assertNotEquals(value1, value2, "PathValues should not be equal: " + description);
+    }
+
+    @Test
+    void testTrivialEquality() {
+        PathValue value1 = new PathValue("Foo", null);
+
+        assertEquals(value1, value1, "Cover reference equality shortcut");
+        assertNotEquals("Foo", value1, "Check it doesn't fail with non-PathValue");
+    }
+}