From f153d1e8685c6748cba4c402938157668f994cb0 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Thu, 11 Sep 2025 15:25:25 -0400 Subject: [PATCH 01/14] Test and fix ResolvedKeySpacePath.equals/hashCode Required tests on PathValue too --- .../keyspace/KeySpaceDirectory.java | 22 +- .../keyspace/KeySpacePathImpl.java | 11 +- .../foundationdb/keyspace/PathValue.java | 21 +- .../keyspace/ResolvedKeySpacePath.java | 2 +- .../foundationdb/keyspace/PathValueTest.java | 86 ++++++++ .../keyspace/ResolvedKeySpacePathTest.java | 208 ++++++++++++++++++ 6 files changed, 342 insertions(+), 8 deletions(-) create mode 100644 fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java create mode 100644 fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java index a92b366b4c..5982f31d2d 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java @@ -710,7 +710,7 @@ public Object getValue() { return value; } - protected static boolean areEqual(Object o1, Object o2) { + protected static boolean areEqual(@Nullable Object o1, @Nullable Object o2) { if (o1 == null) { return o2 == null; } else { @@ -740,6 +740,26 @@ protected static boolean areEqual(Object o1, Object o2) { } } + protected static int valueHashCode(@Nullable Object value) { + if (value == null) { + return Objects.hashCode(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. diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java index 7102da8936..b5135a02fd 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java @@ -283,11 +283,12 @@ public boolean equals(Object obj) { // 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()); + KeySpaceDirectory.areEqual(this.getDirectory().getValue(), that.getDirectory().getValue()); + // 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 @@ -295,8 +296,8 @@ public int hashCode() { return Objects.hash( getDirectory().getKeyType(), getDirectory().getName(), - getDirectory().getValue(), - getValue(), + KeySpaceDirectory.valueHashCode(getDirectory().getValue()), + KeySpaceDirectory.valueHashCode(getValue()), parent); } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValue.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValue.java index 5de09bd396..2582654572 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValue.java +++ b/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 resolvedValue * 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 metadata 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)); + } } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java index f07629954e..4007fdcf19 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java @@ -230,7 +230,7 @@ public boolean equals(Object other) { ResolvedKeySpacePath otherPath = (ResolvedKeySpacePath) other; return this.inner.equals(otherPath.inner) - && Objects.equals(this.getResolvedValue(), otherPath.getResolvedValue()); + && Objects.equals(this.getResolvedPathValue(), otherPath.getResolvedPathValue()); } @Override diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java new file mode 100644 index 0000000000..c06644a927 --- /dev/null +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java @@ -0,0 +1,86 @@ +/* + * 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.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} equals() and hashCode() methods. + */ +class PathValueTest { + + /** + * Test data for PathValue equality tests. + */ + static Stream 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 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("different resolved values", "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); + } +} diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java new file mode 100644 index 0000000000..9522299682 --- /dev/null +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java @@ -0,0 +1,208 @@ +/* + * ResolvedKeySpacePathTest.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 com.apple.foundationdb.record.provider.foundationdb.FDBDatabase; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext; +import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpaceDirectory.KeyType; +import com.apple.foundationdb.record.test.FDBDatabaseExtension; +import com.apple.foundationdb.tuple.Tuple; +import com.apple.test.Tags; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +import java.util.Map; +import java.util.UUID; +import java.util.function.Supplier; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; + +/** + * Tests for {@link ResolvedKeySpacePath} equals() and hashCode() methods. + */ +@Tag(Tags.RequiresFDB) +class ResolvedKeySpacePathTest { + @RegisterExtension + final FDBDatabaseExtension dbExtension = new FDBDatabaseExtension(); + + /** + * Test value pairs for each KeyType. + */ + private static final Map TYPE_TEST_VALUES = Map.of( + KeyType.STRING, new TestValuePair(() -> "value1", () -> "value2"), + KeyType.LONG, new TestValuePair(() -> 100L, () -> 200L), + KeyType.BYTES, new TestValuePair(() -> new byte[]{1, 2, 3}, () -> new byte[]{4, 5, 6}), + KeyType.UUID, new TestValuePair(() -> new UUID(1, 1), () -> new UUID(2, 2)), + KeyType.BOOLEAN, new TestValuePair(() -> true, () -> false), + KeyType.NULL, new TestValuePair(() -> null, () -> null), + KeyType.FLOAT, new TestValuePair(() -> 1.5f, () -> 2.5f), + KeyType.DOUBLE, new TestValuePair(() -> 1.5d, () -> 2.5d) + ); + + /** + * Test equals and hashCode contracts for depth 1 directories. + */ + @ParameterizedTest + @EnumSource(KeyType.class) + void testEqualsHashCodeDepth1(KeyType keyType) { + TestValuePair values = TYPE_TEST_VALUES.get(keyType); + final FDBDatabase database = dbExtension.getDatabase(); + + try (FDBRecordContext context = database.openContext()) { + // Create two identical paths + ResolvedKeySpacePath path1 = createResolvedPath(context, keyType, values.getValue1(), null); + ResolvedKeySpacePath path2 = createResolvedPath(context, keyType, values.getValue1(), null); + + // Test equality contracts + assertEquals(path1, path2, "Identical paths should be equal"); + assertEquals(path2, path1, "Symmetry: path2.equals(path1)"); + assertEquals(path1.hashCode(), path2.hashCode(), "Equal objects must have equal hash codes"); + + // Test inequality when values differ (except NULL type which only has null values) + if (keyType != KeyType.NULL && values.getValue2() != null) { + ResolvedKeySpacePath path3 = createResolvedPath(context, keyType, values.getValue2(), null); + assertNotEquals(path1, path3, "Paths with different values should not be equal"); + } + + // Test basic contracts + assertEquals(path1, path1, "Reflexivity"); + assertNotEquals(path1, null, "Null comparison"); + assertNotEquals(path1, "not a path", "Type safety"); + } + } + + /** + * Test equals and hashCode with hierarchical paths (parent-child). + */ + @ParameterizedTest + @EnumSource(KeyType.class) + void testEqualsHashCodeWithParent(KeyType childKeyType) { + final FDBDatabase database = dbExtension.getDatabase(); + + try (FDBRecordContext context = database.openContext()) { + TestValuePair childValues = TYPE_TEST_VALUES.get(childKeyType); + + // Create parent path (always STRING type) + ResolvedKeySpacePath parent1 = createResolvedPath(context, KeyType.STRING, "parent1", null); + ResolvedKeySpacePath parent2 = createResolvedPath(context, KeyType.STRING, "parent2", null); + + // Create child paths with same parent + ResolvedKeySpacePath child1 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent1); + ResolvedKeySpacePath child2 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent1); + + // Test equality - should be equal with same parent and value + assertEquals(child1, child2, "Children with same parent and value should be equal"); + assertEquals(child1.hashCode(), child2.hashCode(), "Equal children should have equal hash codes"); + + // Test with different parents but same child value + ResolvedKeySpacePath child3 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent2); + + // Current implementation: equals() doesn't compare parent, only inner path and resolved value + // This test documents the current behavior + assertEquals(child1, child3, "Current implementation: parent not compared in equals()"); + } + } + + /** + * Test that demonstrates the actual equals/hashCode behavior with different PathValue metadata. + */ + @Test + void testEqualsHashCodeWithDifferentMetadata() { + final FDBDatabase database = dbExtension.getDatabase(); + + try (FDBRecordContext context = database.openContext()) { + // Create two paths with same inner path and resolved value but different metadata + KeySpacePath innerPath = createKeySpacePath(context, KeyType.STRING, "test"); + PathValue value1 = new PathValue("resolved", new byte[]{1, 2, 3}); + PathValue value2 = new PathValue("resolved", new byte[]{4, 5, 6}); + + ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(null, innerPath, value1, null); + ResolvedKeySpacePath path2 = new ResolvedKeySpacePath(null, innerPath, value2, null); + + assertNotEquals(path1, path2, "Objects should be equal (same inner path and resolved value, metadata ignored)"); + assertNotEquals(path1.hashCode(), path2.hashCode(), + "Hash codes differ due to different PathValue metadata"); + } + } + + /** + * Test remainder field behavior in equals. + */ + @Test + void testRemainderNotComparedInEquals() { + final FDBDatabase database = dbExtension.getDatabase(); + + try (FDBRecordContext context = database.openContext()) { + KeySpacePath innerPath = createKeySpacePath(context, KeyType.STRING, "test"); + PathValue value = new PathValue("resolved", null); + + ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder1")); + ResolvedKeySpacePath path2 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder2")); + + // Current implementation: remainder is not compared in equals() + assertEquals(path1, path2, "Current implementation: remainder not compared in equals()"); + } + } + + /** + * Helper to create a resolved path for testing. + */ + private ResolvedKeySpacePath createResolvedPath(FDBRecordContext context, KeyType keyType, + Object value, ResolvedKeySpacePath parent) { + KeySpacePath innerPath = createKeySpacePath(context, keyType, value); + PathValue pathValue = new PathValue(value, null); + return new ResolvedKeySpacePath(parent, innerPath, pathValue, null); + } + + /** + * Helper to create a KeySpacePath for testing. + */ + private KeySpacePath createKeySpacePath(FDBRecordContext context, KeyType keyType, Object value) { + KeySpaceDirectory rootDir = new KeySpaceDirectory("test", keyType, value); + KeySpace keySpace = new KeySpace(rootDir); + return keySpace.path("test"); + } + + /** + * Test value pair for each KeyType. + */ + private static class TestValuePair { + private final Supplier value1Supplier; + private final Supplier value2Supplier; + + TestValuePair(Supplier value1Supplier, Supplier value2Supplier) { + this.value1Supplier = value1Supplier; + this.value2Supplier = value2Supplier; + } + + Object getValue1() { + return value1Supplier.get(); + } + + Object getValue2() { + return value2Supplier.get(); + } + } +} From f6bdc4096c28a424b7a2c3a6245d259652f27d09 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Thu, 11 Sep 2025 16:27:23 -0400 Subject: [PATCH 02/14] Test with constant directories, and AnyValue --- .../keyspace/KeySpaceDirectory.java | 2 +- .../keyspace/KeySpacePathImpl.java | 4 +- .../keyspace/ResolvedKeySpacePathTest.java | 83 +++++++++++++------ 3 files changed, 60 insertions(+), 29 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java index 5982f31d2d..a6686dc8e0 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java @@ -742,7 +742,7 @@ protected static boolean areEqual(@Nullable Object o1, @Nullable Object o2) { protected static int valueHashCode(@Nullable Object value) { if (value == null) { - return Objects.hashCode(value); + return 0; } switch (KeyType.typeOf(value)) { diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java index b5135a02fd..0f396ac06b 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java @@ -282,8 +282,7 @@ public boolean equals(Object obj) { // 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()) && - KeySpaceDirectory.areEqual(this.getDirectory().getValue(), that.getDirectory().getValue()); + Objects.equals(this.getDirectory().getName(), that.getDirectory().getName()); // the values might be byte[] return directoriesEqual && @@ -296,7 +295,6 @@ public int hashCode() { return Objects.hash( getDirectory().getKeyType(), getDirectory().getName(), - KeySpaceDirectory.valueHashCode(getDirectory().getValue()), KeySpaceDirectory.valueHashCode(getValue()), parent); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java index 9522299682..2d7ef3549c 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java @@ -25,16 +25,20 @@ import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpaceDirectory.KeyType; import com.apple.foundationdb.record.test.FDBDatabaseExtension; import com.apple.foundationdb.tuple.Tuple; +import com.apple.test.BooleanSource; +import com.apple.test.ParameterizedTestUtils; import com.apple.test.Tags; import org.junit.jupiter.api.Tag; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import java.util.Arrays; import java.util.Map; import java.util.UUID; import java.util.function.Supplier; +import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -47,6 +51,16 @@ class ResolvedKeySpacePathTest { @RegisterExtension final FDBDatabaseExtension dbExtension = new FDBDatabaseExtension(); + /** + * Provides test parameters for KeyType and constantDirectory combinations. + */ + static Stream keyTypeAndConstantDirectory() { + return ParameterizedTestUtils.cartesianProduct( + Arrays.stream(KeyType.values()), + ParameterizedTestUtils.booleans("constantDirectory") + ); + } + /** * Test value pairs for each KeyType. */ @@ -65,15 +79,15 @@ KeyType.DOUBLE, new TestValuePair(() -> 1.5d, () -> 2.5d) * Test equals and hashCode contracts for depth 1 directories. */ @ParameterizedTest - @EnumSource(KeyType.class) - void testEqualsHashCodeDepth1(KeyType keyType) { + @MethodSource("keyTypeAndConstantDirectory") + void testEqualsHashCodeDepth1(KeyType keyType, boolean constantDirectory) { TestValuePair values = TYPE_TEST_VALUES.get(keyType); final FDBDatabase database = dbExtension.getDatabase(); try (FDBRecordContext context = database.openContext()) { // Create two identical paths - ResolvedKeySpacePath path1 = createResolvedPath(context, keyType, values.getValue1(), null); - ResolvedKeySpacePath path2 = createResolvedPath(context, keyType, values.getValue1(), null); + ResolvedKeySpacePath path1 = createResolvedPath(context, keyType, values.getValue1(), null, constantDirectory); + ResolvedKeySpacePath path2 = createResolvedPath(context, keyType, values.getValue1(), null, constantDirectory); // Test equality contracts assertEquals(path1, path2, "Identical paths should be equal"); @@ -82,7 +96,7 @@ void testEqualsHashCodeDepth1(KeyType keyType) { // Test inequality when values differ (except NULL type which only has null values) if (keyType != KeyType.NULL && values.getValue2() != null) { - ResolvedKeySpacePath path3 = createResolvedPath(context, keyType, values.getValue2(), null); + ResolvedKeySpacePath path3 = createResolvedPath(context, keyType, values.getValue2(), null, constantDirectory); assertNotEquals(path1, path3, "Paths with different values should not be equal"); } @@ -97,27 +111,27 @@ void testEqualsHashCodeDepth1(KeyType keyType) { * Test equals and hashCode with hierarchical paths (parent-child). */ @ParameterizedTest - @EnumSource(KeyType.class) - void testEqualsHashCodeWithParent(KeyType childKeyType) { + @MethodSource("keyTypeAndConstantDirectory") + void testEqualsHashCodeWithParent(KeyType childKeyType, boolean constantDirectory) { final FDBDatabase database = dbExtension.getDatabase(); try (FDBRecordContext context = database.openContext()) { TestValuePair childValues = TYPE_TEST_VALUES.get(childKeyType); // Create parent path (always STRING type) - ResolvedKeySpacePath parent1 = createResolvedPath(context, KeyType.STRING, "parent1", null); - ResolvedKeySpacePath parent2 = createResolvedPath(context, KeyType.STRING, "parent2", null); + ResolvedKeySpacePath parent1 = createResolvedPath(context, KeyType.STRING, "parent1", null, constantDirectory); + ResolvedKeySpacePath parent2 = createResolvedPath(context, KeyType.STRING, "parent2", null, constantDirectory); // Create child paths with same parent - ResolvedKeySpacePath child1 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent1); - ResolvedKeySpacePath child2 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent1); + ResolvedKeySpacePath child1 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent1, constantDirectory); + ResolvedKeySpacePath child2 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent1, constantDirectory); // Test equality - should be equal with same parent and value assertEquals(child1, child2, "Children with same parent and value should be equal"); assertEquals(child1.hashCode(), child2.hashCode(), "Equal children should have equal hash codes"); // Test with different parents but same child value - ResolvedKeySpacePath child3 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent2); + ResolvedKeySpacePath child3 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent2, constantDirectory); // Current implementation: equals() doesn't compare parent, only inner path and resolved value // This test documents the current behavior @@ -128,13 +142,14 @@ void testEqualsHashCodeWithParent(KeyType childKeyType) { /** * Test that demonstrates the actual equals/hashCode behavior with different PathValue metadata. */ - @Test - void testEqualsHashCodeWithDifferentMetadata() { + @ParameterizedTest + @BooleanSource("constantDirectory") + void testEqualsHashCodeWithDifferentMetadata(boolean constantDirectory) { final FDBDatabase database = dbExtension.getDatabase(); try (FDBRecordContext context = database.openContext()) { // Create two paths with same inner path and resolved value but different metadata - KeySpacePath innerPath = createKeySpacePath(context, KeyType.STRING, "test"); + KeySpacePath innerPath = createKeySpacePath(context, KeyType.STRING, "resolved", constantDirectory); PathValue value1 = new PathValue("resolved", new byte[]{1, 2, 3}); PathValue value2 = new PathValue("resolved", new byte[]{4, 5, 6}); @@ -150,12 +165,13 @@ void testEqualsHashCodeWithDifferentMetadata() { /** * Test remainder field behavior in equals. */ - @Test - void testRemainderNotComparedInEquals() { + @ParameterizedTest + @BooleanSource("constantDirectory") + void testRemainderNotComparedInEquals(boolean constantDirectory) { final FDBDatabase database = dbExtension.getDatabase(); try (FDBRecordContext context = database.openContext()) { - KeySpacePath innerPath = createKeySpacePath(context, KeyType.STRING, "test"); + KeySpacePath innerPath = createKeySpacePath(context, KeyType.STRING, "resolved", constantDirectory); PathValue value = new PathValue("resolved", null); ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder1")); @@ -170,8 +186,8 @@ void testRemainderNotComparedInEquals() { * Helper to create a resolved path for testing. */ private ResolvedKeySpacePath createResolvedPath(FDBRecordContext context, KeyType keyType, - Object value, ResolvedKeySpacePath parent) { - KeySpacePath innerPath = createKeySpacePath(context, keyType, value); + Object value, ResolvedKeySpacePath parent, boolean constantDirectory) { + KeySpacePath innerPath = createKeySpacePath(context, keyType, value, constantDirectory); PathValue pathValue = new PathValue(value, null); return new ResolvedKeySpacePath(parent, innerPath, pathValue, null); } @@ -179,10 +195,27 @@ private ResolvedKeySpacePath createResolvedPath(FDBRecordContext context, KeyTyp /** * Helper to create a KeySpacePath for testing. */ - private KeySpacePath createKeySpacePath(FDBRecordContext context, KeyType keyType, Object value) { - KeySpaceDirectory rootDir = new KeySpaceDirectory("test", keyType, value); + private KeySpacePath createKeySpacePath(FDBRecordContext context, KeyType keyType, Object value, boolean constantDirectory) { + // Always create same root directory + KeySpaceDirectory rootDir = new KeySpaceDirectory("root", KeyType.STRING, "root"); + + // Create child directory based on constantDirectory parameter + KeySpaceDirectory childDir; + if (constantDirectory) { + childDir = new KeySpaceDirectory("test", keyType, value); + } else { + childDir = new KeySpaceDirectory("test", keyType); + } + rootDir.addSubdirectory(childDir); + KeySpace keySpace = new KeySpace(rootDir); - return keySpace.path("test"); + KeySpacePath rootPath = keySpace.path("root"); + + if (constantDirectory) { + return rootPath.add("test"); + } else { + return rootPath.add("test", value); + } } /** From ffc000c87748931cbff6ba76ecd5d7bb90874e14 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Thu, 11 Sep 2025 16:29:02 -0400 Subject: [PATCH 03/14] Explain reason for suppliers --- .../foundationdb/keyspace/ResolvedKeySpacePathTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java index 2d7ef3549c..fe85fe1464 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java @@ -220,6 +220,8 @@ private KeySpacePath createKeySpacePath(FDBRecordContext context, KeyType keyTyp /** * Test value pair for each KeyType. + * We use {@link Supplier} here to make sure that if it is falling back to reference equality (e.g. byte[]), + * we want to catch if it doesn't consider those equal. */ private static class TestValuePair { private final Supplier value1Supplier; From a84876b7f6a9747a492f6f6b384586615af31b76 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 12 Sep 2025 13:14:06 -0400 Subject: [PATCH 04/14] Ensure parent ResolvedKeySpacePaths are equal --- .../keyspace/ResolvedKeySpacePath.java | 5 +- .../keyspace/ResolvedKeySpacePathTest.java | 177 ++++++++---------- 2 files changed, 81 insertions(+), 101 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java index 4007fdcf19..d73e50d824 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java @@ -229,8 +229,9 @@ public boolean equals(Object other) { } ResolvedKeySpacePath otherPath = (ResolvedKeySpacePath) other; - return this.inner.equals(otherPath.inner) - && Objects.equals(this.getResolvedPathValue(), otherPath.getResolvedPathValue()); + return Objects.equals(this.getResolvedPathValue(), otherPath.getResolvedPathValue()) && + Objects.equals(this.getParent(), otherPath.getParent()) && + this.inner.equals(otherPath.inner); } @Override diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java index fe85fe1464..400914e359 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java @@ -20,8 +20,6 @@ package com.apple.foundationdb.record.provider.foundationdb.keyspace; -import com.apple.foundationdb.record.provider.foundationdb.FDBDatabase; -import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext; import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpaceDirectory.KeyType; import com.apple.foundationdb.record.test.FDBDatabaseExtension; import com.apple.foundationdb.tuple.Tuple; @@ -34,6 +32,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import javax.annotation.Nonnull; import java.util.Arrays; import java.util.Map; import java.util.UUID; @@ -54,10 +53,11 @@ class ResolvedKeySpacePathTest { /** * Provides test parameters for KeyType and constantDirectory combinations. */ - static Stream keyTypeAndConstantDirectory() { + static Stream testEqualsHashCode() { return ParameterizedTestUtils.cartesianProduct( Arrays.stream(KeyType.values()), - ParameterizedTestUtils.booleans("constantDirectory") + ParameterizedTestUtils.booleans("constantDirectory"), + ParameterizedTestUtils.booleans("differenceInParent") ); } @@ -79,64 +79,35 @@ KeyType.DOUBLE, new TestValuePair(() -> 1.5d, () -> 2.5d) * Test equals and hashCode contracts for depth 1 directories. */ @ParameterizedTest - @MethodSource("keyTypeAndConstantDirectory") - void testEqualsHashCodeDepth1(KeyType keyType, boolean constantDirectory) { + @MethodSource("testEqualsHashCode") + void testEqualsHashCode(KeyType keyType, boolean constantDirectory, boolean differenceInParent) { TestValuePair values = TYPE_TEST_VALUES.get(keyType); - final FDBDatabase database = dbExtension.getDatabase(); - - try (FDBRecordContext context = database.openContext()) { - // Create two identical paths - ResolvedKeySpacePath path1 = createResolvedPath(context, keyType, values.getValue1(), null, constantDirectory); - ResolvedKeySpacePath path2 = createResolvedPath(context, keyType, values.getValue1(), null, constantDirectory); - - // Test equality contracts - assertEquals(path1, path2, "Identical paths should be equal"); - assertEquals(path2, path1, "Symmetry: path2.equals(path1)"); - assertEquals(path1.hashCode(), path2.hashCode(), "Equal objects must have equal hash codes"); - - // Test inequality when values differ (except NULL type which only has null values) - if (keyType != KeyType.NULL && values.getValue2() != null) { - ResolvedKeySpacePath path3 = createResolvedPath(context, keyType, values.getValue2(), null, constantDirectory); - assertNotEquals(path1, path3, "Paths with different values should not be equal"); - } - - // Test basic contracts - assertEquals(path1, path1, "Reflexivity"); - assertNotEquals(path1, null, "Null comparison"); - assertNotEquals(path1, "not a path", "Type safety"); - } - } + // Test case 1: Same logical and resolved values (existing test) + ResolvedKeySpacePath path1 = createResolvedPath(keyType, values.getValue1(), values.getValue1(), + createRootParent(), constantDirectory, differenceInParent); + ResolvedKeySpacePath path2 = createResolvedPath(keyType, values.getValue1(), values.getValue1(), + createRootParent(), constantDirectory, differenceInParent); - /** - * Test equals and hashCode with hierarchical paths (parent-child). - */ - @ParameterizedTest - @MethodSource("keyTypeAndConstantDirectory") - void testEqualsHashCodeWithParent(KeyType childKeyType, boolean constantDirectory) { - final FDBDatabase database = dbExtension.getDatabase(); - - try (FDBRecordContext context = database.openContext()) { - TestValuePair childValues = TYPE_TEST_VALUES.get(childKeyType); - - // Create parent path (always STRING type) - ResolvedKeySpacePath parent1 = createResolvedPath(context, KeyType.STRING, "parent1", null, constantDirectory); - ResolvedKeySpacePath parent2 = createResolvedPath(context, KeyType.STRING, "parent2", null, constantDirectory); - - // Create child paths with same parent - ResolvedKeySpacePath child1 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent1, constantDirectory); - ResolvedKeySpacePath child2 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent1, constantDirectory); + // Test equality contracts + assertEquals(path1, path2, "Identical paths should be equal"); + assertEquals(path2, path1, "Symmetry: path2.equals(path1)"); + assertEquals(path1.hashCode(), path2.hashCode(), "Equal objects must have equal hash codes"); - // Test equality - should be equal with same parent and value - assertEquals(child1, child2, "Children with same parent and value should be equal"); - assertEquals(child1.hashCode(), child2.hashCode(), "Equal children should have equal hash codes"); - - // Test with different parents but same child value - ResolvedKeySpacePath child3 = createResolvedPath(context, childKeyType, childValues.getValue1(), parent2, constantDirectory); - - // Current implementation: equals() doesn't compare parent, only inner path and resolved value - // This test documents the current behavior - assertEquals(child1, child3, "Current implementation: parent not compared in equals()"); + // Test inequality when values differ (except NULL type which only has null values) + if (keyType != KeyType.NULL && values.getValue2() != null) { + ResolvedKeySpacePath path3 = createResolvedPath(keyType, values.getValue2(), createRootParent(), constantDirectory); + assertNotEquals(path1, path3, "Paths with different values should not be equal"); + + assertNotEquals(createResolvedPath(keyType, values.getValue1(), values.getValue2(), createRootParent(), constantDirectory, differenceInParent), + path1, "Paths with different resolved values should not be equal"); + assertNotEquals(createResolvedPath(keyType, values.getValue2(), values.getValue1(), createRootParent(), constantDirectory, differenceInParent), + path1, "Paths with different logical values should not be equal"); } + + // Test basic contracts + assertEquals(path1, path1, "Reflexivity"); + assertNotEquals(path1, null, "Null comparison"); + assertNotEquals(path1, "not a path", "Type safety"); } /** @@ -145,21 +116,17 @@ void testEqualsHashCodeWithParent(KeyType childKeyType, boolean constantDirector @ParameterizedTest @BooleanSource("constantDirectory") void testEqualsHashCodeWithDifferentMetadata(boolean constantDirectory) { - final FDBDatabase database = dbExtension.getDatabase(); - - try (FDBRecordContext context = database.openContext()) { - // Create two paths with same inner path and resolved value but different metadata - KeySpacePath innerPath = createKeySpacePath(context, KeyType.STRING, "resolved", constantDirectory); - PathValue value1 = new PathValue("resolved", new byte[]{1, 2, 3}); - PathValue value2 = new PathValue("resolved", new byte[]{4, 5, 6}); + // Create two paths with same inner path and resolved value but different metadata + KeySpacePath innerPath = createKeySpacePath(createRootParent(), KeyType.STRING, "resolved", constantDirectory); + PathValue value1 = new PathValue("resolved", new byte[]{1, 2, 3}); + PathValue value2 = new PathValue("resolved", new byte[]{4, 5, 6}); - ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(null, innerPath, value1, null); - ResolvedKeySpacePath path2 = new ResolvedKeySpacePath(null, innerPath, value2, null); + ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(null, innerPath, value1, null); + ResolvedKeySpacePath path2 = new ResolvedKeySpacePath(null, innerPath, value2, null); - assertNotEquals(path1, path2, "Objects should be equal (same inner path and resolved value, metadata ignored)"); - assertNotEquals(path1.hashCode(), path2.hashCode(), - "Hash codes differ due to different PathValue metadata"); - } + assertNotEquals(path1, path2, "Objects should be equal (same inner path and resolved value, metadata ignored)"); + assertNotEquals(path1.hashCode(), path2.hashCode(), + "Hash codes differ due to different PathValue metadata"); } /** @@ -168,37 +135,43 @@ void testEqualsHashCodeWithDifferentMetadata(boolean constantDirectory) { @ParameterizedTest @BooleanSource("constantDirectory") void testRemainderNotComparedInEquals(boolean constantDirectory) { - final FDBDatabase database = dbExtension.getDatabase(); - - try (FDBRecordContext context = database.openContext()) { - KeySpacePath innerPath = createKeySpacePath(context, KeyType.STRING, "resolved", constantDirectory); - PathValue value = new PathValue("resolved", null); + KeySpacePath innerPath = createKeySpacePath(createRootParent(), KeyType.STRING, "resolved", constantDirectory); + PathValue value = new PathValue("resolved", null); - ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder1")); - ResolvedKeySpacePath path2 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder2")); + ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder1")); + ResolvedKeySpacePath path2 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder2")); - // Current implementation: remainder is not compared in equals() - assertEquals(path1, path2, "Current implementation: remainder not compared in equals()"); - } + // Current implementation: remainder is not compared in equals() + assertEquals(path1, path2, "Current implementation: remainder not compared in equals()"); } - /** - * Helper to create a resolved path for testing. - */ - private ResolvedKeySpacePath createResolvedPath(FDBRecordContext context, KeyType keyType, - Object value, ResolvedKeySpacePath parent, boolean constantDirectory) { - KeySpacePath innerPath = createKeySpacePath(context, keyType, value, constantDirectory); - PathValue pathValue = new PathValue(value, null); - return new ResolvedKeySpacePath(parent, innerPath, pathValue, null); + private ResolvedKeySpacePath createResolvedPath(KeyType keyType, Object value, + ResolvedKeySpacePath parent, boolean constantDirectory) { + return createResolvedPath(keyType, value, value, parent, constantDirectory, false); + } + + private ResolvedKeySpacePath createResolvedPath(KeyType keyType, + Object logicalValue, Object resolvedValue, + @Nonnull ResolvedKeySpacePath parent, + boolean constantDirectory, final boolean addConstantChild) { + KeySpacePath innerPath = createKeySpacePath(parent, keyType, logicalValue, constantDirectory); + PathValue pathValue = new PathValue(resolvedValue, null); + final ResolvedKeySpacePath resolvedKeySpacePath = new ResolvedKeySpacePath(parent, innerPath, pathValue, null); + if (addConstantChild) { + final ResolvedKeySpacePath resolvedPath = createResolvedPath(KeyType.STRING, "Constant", "Constant", resolvedKeySpacePath, true, false); + System.out.println(resolvedPath); + return resolvedPath; + } else { + System.out.println(resolvedKeySpacePath); + return resolvedKeySpacePath; + } } /** * Helper to create a KeySpacePath for testing. */ - private KeySpacePath createKeySpacePath(FDBRecordContext context, KeyType keyType, Object value, boolean constantDirectory) { - // Always create same root directory - KeySpaceDirectory rootDir = new KeySpaceDirectory("root", KeyType.STRING, "root"); - + private KeySpacePath createKeySpacePath(@Nonnull ResolvedKeySpacePath parent, KeyType keyType, Object value, + boolean constantDirectory) { // Create child directory based on constantDirectory parameter KeySpaceDirectory childDir; if (constantDirectory) { @@ -206,18 +179,24 @@ private KeySpacePath createKeySpacePath(FDBRecordContext context, KeyType keyTyp } else { childDir = new KeySpaceDirectory("test", keyType); } - rootDir.addSubdirectory(childDir); - - KeySpace keySpace = new KeySpace(rootDir); - KeySpacePath rootPath = keySpace.path("root"); + parent.getDirectory().addSubdirectory(childDir); if (constantDirectory) { - return rootPath.add("test"); + return parent.toPath().add("test"); } else { - return rootPath.add("test", value); + return parent.toPath().add("test", value); } } + @Nonnull + private static ResolvedKeySpacePath createRootParent() { + KeySpacePath parent; + final KeySpaceDirectory parentDir = new KeySpaceDirectory("root", KeyType.STRING, "root"); + KeySpace keySpace = new KeySpace(parentDir); + parent = keySpace.path("root"); + return new ResolvedKeySpacePath(null, parent, new PathValue("root", null), null); + } + /** * Test value pair for each KeyType. * We use {@link Supplier} here to make sure that if it is falling back to reference equality (e.g. byte[]), From c35d1d47b5faed0a47368a4a8169a2ef36eda88c Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 12 Sep 2025 13:26:14 -0400 Subject: [PATCH 05/14] Add @Nonnull/@Nullable to test class --- .../keyspace/ResolvedKeySpacePathTest.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java index 400914e359..6555cb4081 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java @@ -33,6 +33,7 @@ import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.Arrays; import java.util.Map; import java.util.UUID; @@ -53,6 +54,7 @@ class ResolvedKeySpacePathTest { /** * Provides test parameters for KeyType and constantDirectory combinations. */ + @Nonnull static Stream testEqualsHashCode() { return ParameterizedTestUtils.cartesianProduct( Arrays.stream(KeyType.values()), @@ -80,8 +82,8 @@ KeyType.DOUBLE, new TestValuePair(() -> 1.5d, () -> 2.5d) */ @ParameterizedTest @MethodSource("testEqualsHashCode") - void testEqualsHashCode(KeyType keyType, boolean constantDirectory, boolean differenceInParent) { - TestValuePair values = TYPE_TEST_VALUES.get(keyType); + void testEqualsHashCode(@Nonnull KeyType keyType, boolean constantDirectory, boolean differenceInParent) { + @Nonnull TestValuePair values = TYPE_TEST_VALUES.get(keyType); // Test case 1: Same logical and resolved values (existing test) ResolvedKeySpacePath path1 = createResolvedPath(keyType, values.getValue1(), values.getValue1(), createRootParent(), constantDirectory, differenceInParent); @@ -145,13 +147,15 @@ void testRemainderNotComparedInEquals(boolean constantDirectory) { assertEquals(path1, path2, "Current implementation: remainder not compared in equals()"); } - private ResolvedKeySpacePath createResolvedPath(KeyType keyType, Object value, - ResolvedKeySpacePath parent, boolean constantDirectory) { + @Nonnull + private ResolvedKeySpacePath createResolvedPath(@Nonnull KeyType keyType, @Nullable Object value, + @Nonnull ResolvedKeySpacePath parent, boolean constantDirectory) { return createResolvedPath(keyType, value, value, parent, constantDirectory, false); } - private ResolvedKeySpacePath createResolvedPath(KeyType keyType, - Object logicalValue, Object resolvedValue, + @Nonnull + private ResolvedKeySpacePath createResolvedPath(@Nonnull KeyType keyType, + @Nullable Object logicalValue, @Nullable Object resolvedValue, @Nonnull ResolvedKeySpacePath parent, boolean constantDirectory, final boolean addConstantChild) { KeySpacePath innerPath = createKeySpacePath(parent, keyType, logicalValue, constantDirectory); @@ -170,7 +174,8 @@ private ResolvedKeySpacePath createResolvedPath(KeyType keyType, /** * Helper to create a KeySpacePath for testing. */ - private KeySpacePath createKeySpacePath(@Nonnull ResolvedKeySpacePath parent, KeyType keyType, Object value, + @Nonnull + private KeySpacePath createKeySpacePath(@Nonnull ResolvedKeySpacePath parent, @Nonnull KeyType keyType, @Nullable Object value, boolean constantDirectory) { // Create child directory based on constantDirectory parameter KeySpaceDirectory childDir; @@ -203,18 +208,22 @@ private static ResolvedKeySpacePath createRootParent() { * we want to catch if it doesn't consider those equal. */ private static class TestValuePair { + @Nonnull private final Supplier value1Supplier; + @Nonnull private final Supplier value2Supplier; - TestValuePair(Supplier value1Supplier, Supplier value2Supplier) { + TestValuePair(@Nonnull Supplier value1Supplier, @Nonnull Supplier value2Supplier) { this.value1Supplier = value1Supplier; this.value2Supplier = value2Supplier; } + @Nullable Object getValue1() { return value1Supplier.get(); } + @Nullable Object getValue2() { return value2Supplier.get(); } From a070102a60631df17ccef0f3e0d4b04c1048d244 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Mon, 15 Sep 2025 12:03:46 -0400 Subject: [PATCH 06/14] Remainder should be included in equals/hashCode + some cleanup --- .../keyspace/ResolvedKeySpacePath.java | 5 +- .../foundationdb/keyspace/PathValueTest.java | 8 +-- .../keyspace/ResolvedKeySpacePathTest.java | 53 ++++++++++--------- 3 files changed, 34 insertions(+), 32 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java index d73e50d824..a996fee24b 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java @@ -231,12 +231,13 @@ public boolean equals(Object other) { ResolvedKeySpacePath otherPath = (ResolvedKeySpacePath) other; return Objects.equals(this.getResolvedPathValue(), otherPath.getResolvedPathValue()) && Objects.equals(this.getParent(), otherPath.getParent()) && - this.inner.equals(otherPath.inner); + 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); } @Override diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java index c06644a927..575328c70a 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java @@ -30,7 +30,7 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; /** - * Tests for {@link PathValue} equals() and hashCode() methods. + * Tests for {@link PathValue}. */ class PathValueTest { @@ -59,14 +59,14 @@ static Stream unequalPathValuePairs() { 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("different resolved values", "test1", new byte[]{1, 2, 3}, "test2", new byte[]{1, 2, 3}) + 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) { + Object resolvedValue2, byte[] metadata2) { PathValue value1 = new PathValue(resolvedValue1, metadata1); PathValue value2 = new PathValue(resolvedValue2, metadata2); @@ -77,7 +77,7 @@ void testEqualsAndHashCodeForEqualValues(String description, Object resolvedValu @ParameterizedTest(name = "{0}") @MethodSource("unequalPathValuePairs") void testNotEqualsForUnequalValues(String description, Object resolvedValue1, byte[] metadata1, - Object resolvedValue2, byte[] metadata2) { + Object resolvedValue2, byte[] metadata2) { PathValue value1 = new PathValue(resolvedValue1, metadata1); PathValue value2 = new PathValue(resolvedValue2, metadata2); diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java index 6555cb4081..be3c1e33fb 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java @@ -38,31 +38,20 @@ import java.util.Map; import java.util.UUID; import java.util.function.Supplier; +import java.util.stream.IntStream; import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; /** - * Tests for {@link ResolvedKeySpacePath} equals() and hashCode() methods. + * Tests for {@link ResolvedKeySpacePath}. */ @Tag(Tags.RequiresFDB) class ResolvedKeySpacePathTest { @RegisterExtension final FDBDatabaseExtension dbExtension = new FDBDatabaseExtension(); - /** - * Provides test parameters for KeyType and constantDirectory combinations. - */ - @Nonnull - static Stream testEqualsHashCode() { - return ParameterizedTestUtils.cartesianProduct( - Arrays.stream(KeyType.values()), - ParameterizedTestUtils.booleans("constantDirectory"), - ParameterizedTestUtils.booleans("differenceInParent") - ); - } - /** * Test value pairs for each KeyType. */ @@ -77,6 +66,15 @@ KeyType.FLOAT, new TestValuePair(() -> 1.5f, () -> 2.5f), KeyType.DOUBLE, new TestValuePair(() -> 1.5d, () -> 2.5d) ); + @Nonnull + static Stream testEqualsHashCode() { + return ParameterizedTestUtils.cartesianProduct( + Arrays.stream(KeyType.values()), + ParameterizedTestUtils.booleans("constantDirectory"), + ParameterizedTestUtils.booleans("differenceInParent") + ); + } + /** * Test equals and hashCode contracts for depth 1 directories. */ @@ -136,15 +134,24 @@ void testEqualsHashCodeWithDifferentMetadata(boolean constantDirectory) { */ @ParameterizedTest @BooleanSource("constantDirectory") - void testRemainderNotComparedInEquals(boolean constantDirectory) { + void testRemainderComparedInEquals(boolean constantDirectory) { KeySpacePath innerPath = createKeySpacePath(createRootParent(), KeyType.STRING, "resolved", constantDirectory); PathValue value = new PathValue("resolved", null); ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder1")); ResolvedKeySpacePath path2 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder2")); - - // Current implementation: remainder is not compared in equals() - assertEquals(path1, path2, "Current implementation: remainder not compared in equals()"); + ResolvedKeySpacePath path3 = new ResolvedKeySpacePath(null, innerPath, value, Tuple.from("remainder1")); + + assertNotEquals(path1, path2, "Paths with different remainders should not be equal"); + assertEquals(path1, path3, "Paths with the same remainder should be equal"); + assertEquals(path1.hashCode(), path3.hashCode(), "Paths with the same remainder should have same hashCode"); + ResolvedKeySpacePath nullRemainder = new ResolvedKeySpacePath(null, innerPath, value, null); + assertNotEquals(path1, nullRemainder, "Path without a remainder should not be the equal to one with a remainder"); + assertNotEquals(nullRemainder, null, "Make sure null is properly handled in equals"); + IntStream.range(0, 10_000).mapToObj(i -> new ResolvedKeySpacePath(null, innerPath, value, Tuple.from(i))) + .filter(path -> path.hashCode() != path1.hashCode()) + .findAny() + .orElseThrow(() -> new AssertionError("Paths with different remainders should sometimes have different hash codes")); } @Nonnull @@ -162,18 +169,14 @@ private ResolvedKeySpacePath createResolvedPath(@Nonnull KeyType keyType, PathValue pathValue = new PathValue(resolvedValue, null); final ResolvedKeySpacePath resolvedKeySpacePath = new ResolvedKeySpacePath(parent, innerPath, pathValue, null); if (addConstantChild) { - final ResolvedKeySpacePath resolvedPath = createResolvedPath(KeyType.STRING, "Constant", "Constant", resolvedKeySpacePath, true, false); - System.out.println(resolvedPath); + final ResolvedKeySpacePath resolvedPath = createResolvedPath(KeyType.STRING, + "Constant", "Constant", resolvedKeySpacePath, true, false); return resolvedPath; } else { - System.out.println(resolvedKeySpacePath); return resolvedKeySpacePath; } } - /** - * Helper to create a KeySpacePath for testing. - */ @Nonnull private KeySpacePath createKeySpacePath(@Nonnull ResolvedKeySpacePath parent, @Nonnull KeyType keyType, @Nullable Object value, boolean constantDirectory) { @@ -195,10 +198,8 @@ private KeySpacePath createKeySpacePath(@Nonnull ResolvedKeySpacePath parent, @N @Nonnull private static ResolvedKeySpacePath createRootParent() { - KeySpacePath parent; final KeySpaceDirectory parentDir = new KeySpaceDirectory("root", KeyType.STRING, "root"); - KeySpace keySpace = new KeySpace(parentDir); - parent = keySpace.path("root"); + KeySpacePath parent = new KeySpace(parentDir).path("root"); return new ResolvedKeySpacePath(null, parent, new PathValue("root", null), null); } From a64373e935967f9f0c8a7d92de1aaf1ae0ab0503 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Mon, 15 Sep 2025 17:30:36 -0400 Subject: [PATCH 07/14] Add parent to ResolvedKeySpacePath.hashCode() --- .../provider/foundationdb/keyspace/ResolvedKeySpacePath.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java index a996fee24b..0afd60db10 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePath.java @@ -237,7 +237,7 @@ public boolean equals(Object other) { @Override public int hashCode() { - return Objects.hash(inner, getResolvedPathValue(), remainder); + return Objects.hash(inner, getResolvedPathValue(), remainder, getParent()); } @Override From 0720af502572ea8cb826aa03b959a14142bddada Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Mon, 22 Sep 2025 16:21:45 -0400 Subject: [PATCH 08/14] Introduce a new KeySpaceDirectory.equalsIgnoringHierarchy This will be useful when comparing paths, because they can validate the parent, and don't care about the directiory's other children. --- .../keyspace/KeySpaceDirectory.java | 34 +++++ .../keyspace/KeySpaceDirectoryTest.java | 137 +++++++++++++++++- 2 files changed, 169 insertions(+), 2 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java index a6686dc8e0..9e08357d4c 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java @@ -719,6 +719,14 @@ protected static boolean areEqual(@Nullable Object o1, @Nullable Object o2) { } } + // Handle ANY_VALUE specially - it should only be equal to itself + boolean isAnyValue = (o1 == ANY_VALUE || o2 == ANY_VALUE); + if (isAnyValue) { + @SuppressWarnings("PMD.CompareObjectsWithEquals") + boolean result = (o1 == o2); + return result; + } + KeyType o1Type = KeyType.typeOf(o1); if (o1Type != KeyType.typeOf(o2)) { return false; @@ -745,6 +753,11 @@ protected static int valueHashCode(@Nullable Object value) { 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); @@ -918,6 +931,27 @@ public static KeyType typeOf(@Nullable Object value) { } } + /** + * Compares two KeySpaceDirectory objects for equality based on their intrinsic properties, + * ignoring their position in the directory hierarchy (parent, subdirectories) and wrapper functions. + * + * @param other the other KeySpaceDirectory to compare with + * @return true if the directories have the same name, keyType, and value; false otherwise + */ + @SuppressWarnings("PMD.CompareObjectsWithEquals") + public boolean equalsIgnoringHierarchy(@Nullable Object other) { + if (this == other) { + return true; + } + if (other == null || getClass() != other.getClass()) { + return false; + } + KeySpaceDirectory that = (KeySpaceDirectory) other; + return Objects.equals(name, that.name) && + Objects.equals(keyType, that.keyType) && + areEqual(value, that.value); + } + private static class AnyValue { private AnyValue() { } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index 0263ca9bf4..da9ed708cc 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -48,6 +48,9 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.FieldSource; +import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -68,6 +71,7 @@ import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.IntStream; +import java.util.stream.Stream; import static com.apple.foundationdb.record.TestHelpers.assertThrows; import static com.apple.foundationdb.record.TestHelpers.eventually; @@ -110,11 +114,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 valueOfEveryType = new ImmutableList.Builder() + private static final List valueOfEveryType = new ImmutableList.Builder() .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; @@ -1218,6 +1227,130 @@ public void testListAcrossTransactions() { assertEquals(directoryEntries.size(), idx); } + private static Stream testEqualsIgnoringHierarchyWithVariousTypes() { + // Basic type equality tests + KeySpaceDirectory stringDir1 = new KeySpaceDirectory("dir", KeyType.STRING, "hello"); + KeySpaceDirectory stringDir2 = new KeySpaceDirectory("dir", KeyType.STRING, "hello"); + KeySpaceDirectory stringDir3 = new KeySpaceDirectory("dir", KeyType.STRING, "goodbye"); + KeySpaceDirectory stringDir4 = new KeySpaceDirectory("different_name", KeyType.STRING, "hello"); + + KeySpaceDirectory longDir1 = new KeySpaceDirectory("dir", KeyType.LONG, 42L); + KeySpaceDirectory longDir2 = new KeySpaceDirectory("dir", KeyType.LONG, 42L); + KeySpaceDirectory longDir3 = new KeySpaceDirectory("dir", KeyType.LONG, 100L); + KeySpaceDirectory longDirAsString = new KeySpaceDirectory("dir", KeyType.STRING, "42"); + + // bytes is, important here because testEqualsIgnoringHierarchyWithAllKeyTypes could pass if calling + // .equals on the two values, but this one would not. + KeySpaceDirectory bytesDir1 = new KeySpaceDirectory("dir", KeyType.BYTES, new byte[] { 0x01, 0x02 }); + KeySpaceDirectory bytesDir2 = new KeySpaceDirectory("dir", KeyType.BYTES, new byte[] { 0x01, 0x02 }); + KeySpaceDirectory bytesDir3 = new KeySpaceDirectory("dir", KeyType.BYTES, new byte[] { 0x05, 0x06 }); + + // Wrapper tests + Function wrapper1 = TestWrapper1::new; + Function wrapper2 = TestWrapper2::new; + KeySpaceDirectory dirWithWrapper1 = new KeySpaceDirectory("test", KeyType.STRING, "value", wrapper1); + KeySpaceDirectory dirWithWrapper2 = new KeySpaceDirectory("test", KeyType.STRING, "value", wrapper2); + KeySpaceDirectory dirWithoutWrapper = new KeySpaceDirectory("test", KeyType.STRING, "value", null); + + // ANY_VALUE tests + KeySpaceDirectory anyValueDir1 = new KeySpaceDirectory("test", KeyType.STRING); + KeySpaceDirectory anyValueDir2 = new KeySpaceDirectory("test", KeyType.STRING); + KeySpaceDirectory specificValueDir = new KeySpaceDirectory("test", KeyType.STRING, "specificValue"); + + // Null and different class tests + KeySpaceDirectory nullTestDir = new KeySpaceDirectory("test", KeyType.STRING, "value"); + + return Stream.of( + // Basic equality/inequality + Arguments.of(stringDir1, stringDir2, true), + Arguments.of(stringDir1, stringDir3, false), + Arguments.of(stringDir4, stringDir1, false), + Arguments.of(longDir1, longDir2, true), + Arguments.of(longDir1, longDir3, false), + Arguments.of(longDir1, longDirAsString, false), + Arguments.of(bytesDir1, bytesDir2, true), + Arguments.of(bytesDir1, bytesDir3, false), + Arguments.of(stringDir1, longDir1, false), + + // Wrapper tests - different wrappers should be equal + Arguments.of(dirWithWrapper1, dirWithWrapper2, true), + Arguments.of(dirWithWrapper1, dirWithoutWrapper, true), + Arguments.of(dirWithWrapper2, dirWithoutWrapper, true), + + // ANY_VALUE tests + Arguments.of(anyValueDir1, anyValueDir2, true), + Arguments.of(anyValueDir1, specificValueDir, false) + ); + } + + @ParameterizedTest + @MethodSource + void testEqualsIgnoringHierarchyWithVariousTypes(KeySpaceDirectory dir1, KeySpaceDirectory dir2, boolean shouldBeEqual) { + if (shouldBeEqual) { + assertTrue(dir1.equalsIgnoringHierarchy(dir2), "Directories should be equal for " + dir1.getKeyType()); + assertTrue(dir2.equalsIgnoringHierarchy(dir1), "Directories should be equal for " + dir2.getKeyType() + " (reversed)"); + } else { + assertFalse(dir1.equalsIgnoringHierarchy(dir2), "Directories should not be equal"); + assertFalse(dir2.equalsIgnoringHierarchy(dir1), "Directories should not be equal (reversed)"); + } + assertTrue(dir1.equalsIgnoringHierarchy(dir1), "Directories should always equal themselves"); + assertTrue(dir2.equalsIgnoringHierarchy(dir2), "Directories should always equal themselves"); + } + + @Test + void testEqualsIgnoringHierarchyWithNull() { + KeySpaceDirectory dir = new KeySpaceDirectory("test", KeyType.STRING, "value"); + assertFalse(dir.equalsIgnoringHierarchy(null), "Directory should not equal null"); + } + + @Test + void testEqualsIgnoringHierarchyWithSubdirectories() { + KeySpaceDirectory parent1 = new KeySpaceDirectory("parent", KeyType.STRING, "test"); + KeySpaceDirectory parent2 = new KeySpaceDirectory("parent", KeyType.STRING, "test"); + + KeySpaceDirectory child1 = new KeySpaceDirectory("child", KeyType.LONG); + KeySpaceDirectory child2 = new KeySpaceDirectory("child", KeyType.LONG); + + parent1.addSubdirectory(child1); + parent2.addSubdirectory(child2); + + assertTrue(parent1.equalsIgnoringHierarchy(parent2), "Parents with equivalent subdirectories should be equal when ignoring hierarchy"); + assertTrue(child1.equalsIgnoringHierarchy(child2), "Children should be equal when ignoring hierarchy"); + + KeySpaceDirectory parent3 = new KeySpaceDirectory("parent", KeyType.STRING, "test"); + KeySpaceDirectory child3 = new KeySpaceDirectory("child", KeyType.STRING); + parent3.addSubdirectory(child3); + + assertTrue(parent1.equalsIgnoringHierarchy(parent3), "Parents should be equal when ignoring subdirectories"); + } + + @Test + void testEqualsIgnoringHierarchyWithParentRelationship() { + KeySpaceDirectory root1 = new KeySpaceDirectory("root", KeyType.STRING, "test"); + KeySpaceDirectory root2 = new KeySpaceDirectory("root", KeyType.STRING, "test"); + + KeySpaceDirectory child1 = new KeySpaceDirectory("child", KeyType.LONG); + KeySpaceDirectory child2 = new KeySpaceDirectory("child", KeyType.LONG); + + root1.addSubdirectory(child1); + root2.addSubdirectory(child2); + + assertTrue(child1.equalsIgnoringHierarchy(child2), "Children with different parents should be equal when ignoring hierarchy"); + } + + @ParameterizedTest + @FieldSource("valueOfEveryType") + void testEqualsIgnoringHierarchyWithAllKeyTypes(KeyTypeValue keyTypeValue) { + KeySpaceDirectory dir1 = new KeySpaceDirectory("dir", keyTypeValue.keyType, keyTypeValue.value); + KeySpaceDirectory dir2 = new KeySpaceDirectory("dir", keyTypeValue.keyType, keyTypeValue.value); + + assertTrue(dir1.equalsIgnoringHierarchy(dir2), "Directories should be equal"); + if (keyTypeValue.keyType != KeyType.NULL) { + assertFalse(dir1.equalsIgnoringHierarchy(new KeySpaceDirectory("dir", keyTypeValue.keyType, keyTypeValue.value2)), + "Directories should not be equal"); + } + } + private static class TestWrapper1 extends KeySpacePathWrapper { public TestWrapper1(KeySpacePath inner) { super(inner); From f2691b4fd33926d04456704699fd3904b4738405 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Mon, 22 Sep 2025 17:06:13 -0400 Subject: [PATCH 09/14] Utilize equalsIgnoringHierarchy and add a couple tests --- .../foundationdb/keyspace/KeySpacePathImpl.java | 10 +++------- .../provider/foundationdb/keyspace/PathValueTest.java | 1 + .../keyspace/ResolvedKeySpacePathTest.java | 5 ++++- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java index 0f396ac06b..4d45311c77 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java @@ -276,13 +276,9 @@ 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()); + // We don't care whether the two objects exist in the same hierarchy, we validate the relevant bits by comparing + // parents. + boolean directoriesEqual = this.getDirectory().equalsIgnoringHierarchy(that.getDirectory()); // the values might be byte[] return directoriesEqual && diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java index 575328c70a..49b90aa0c9 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java @@ -59,6 +59,7 @@ static Stream unequalPathValuePairs() { 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}) ); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java index be3c1e33fb..e3dde94acb 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java @@ -43,6 +43,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNull; /** * Tests for {@link ResolvedKeySpacePath}. @@ -94,7 +95,7 @@ void testEqualsHashCode(@Nonnull KeyType keyType, boolean constantDirectory, boo assertEquals(path1.hashCode(), path2.hashCode(), "Equal objects must have equal hash codes"); // Test inequality when values differ (except NULL type which only has null values) - if (keyType != KeyType.NULL && values.getValue2() != null) { + if (keyType != KeyType.NULL) { ResolvedKeySpacePath path3 = createResolvedPath(keyType, values.getValue2(), createRootParent(), constantDirectory); assertNotEquals(path1, path3, "Paths with different values should not be equal"); @@ -102,6 +103,8 @@ void testEqualsHashCode(@Nonnull KeyType keyType, boolean constantDirectory, boo path1, "Paths with different resolved values should not be equal"); assertNotEquals(createResolvedPath(keyType, values.getValue2(), values.getValue1(), createRootParent(), constantDirectory, differenceInParent), path1, "Paths with different logical values should not be equal"); + } else { + assertNull(values.getValue2()); } // Test basic contracts From 89174b817710ce93d8b7ff89c6e8215b8dcb8601 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 23 Sep 2025 12:40:53 -0400 Subject: [PATCH 10/14] Cover some test gaps --- .../foundationdb/keyspace/KeySpaceDirectoryTest.java | 6 ++---- .../provider/foundationdb/keyspace/PathValueTest.java | 9 +++++++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index da9ed708cc..c807110f9a 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -1257,9 +1257,6 @@ private static Stream testEqualsIgnoringHierarchyWithVariousTypes() { KeySpaceDirectory anyValueDir2 = new KeySpaceDirectory("test", KeyType.STRING); KeySpaceDirectory specificValueDir = new KeySpaceDirectory("test", KeyType.STRING, "specificValue"); - // Null and different class tests - KeySpaceDirectory nullTestDir = new KeySpaceDirectory("test", KeyType.STRING, "value"); - return Stream.of( // Basic equality/inequality Arguments.of(stringDir1, stringDir2, true), @@ -1298,9 +1295,10 @@ void testEqualsIgnoringHierarchyWithVariousTypes(KeySpaceDirectory dir1, KeySpac } @Test - void testEqualsIgnoringHierarchyWithNull() { + void testEqualsIgnoringHierarchyTrivialCases() { KeySpaceDirectory dir = new KeySpaceDirectory("test", KeyType.STRING, "value"); assertFalse(dir.equalsIgnoringHierarchy(null), "Directory should not equal null"); + assertFalse(dir.equalsIgnoringHierarchy("value"), "Directory should not equal other classes"); } @Test diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java index 49b90aa0c9..e23bd1e153 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/PathValueTest.java @@ -20,6 +20,7 @@ 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; @@ -84,4 +85,12 @@ void testNotEqualsForUnequalValues(String description, Object resolvedValue1, by 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"); + } } From 98d4b03bb7b84cf7502e8566f068d5dcedb0e3ff Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 23 Sep 2025 15:46:37 -0400 Subject: [PATCH 11/14] Some more cleanup --- .../foundationdb/keyspace/KeySpaceDirectory.java | 7 +++---- .../keyspace/KeySpaceDirectoryTest.java | 13 ++++--------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java index 9e08357d4c..9e4279d284 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java @@ -710,6 +710,7 @@ public Object getValue() { return value; } + @SuppressWarnings("PMD.CompareObjectsWithEquals") // we use ref protected static boolean areEqual(@Nullable Object o1, @Nullable Object o2) { if (o1 == null) { return o2 == null; @@ -719,12 +720,10 @@ protected static boolean areEqual(@Nullable Object o1, @Nullable Object o2) { } } - // Handle ANY_VALUE specially - it should only be equal to itself + // Handle ANY_VALUE specially - typeOf does not support ANY_VALUE boolean isAnyValue = (o1 == ANY_VALUE || o2 == ANY_VALUE); if (isAnyValue) { - @SuppressWarnings("PMD.CompareObjectsWithEquals") - boolean result = (o1 == o2); - return result; + return Objects.equals(o1, o2); } KeyType o1Type = KeyType.typeOf(o1); diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index c807110f9a..552001778a 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -1227,7 +1227,7 @@ public void testListAcrossTransactions() { assertEquals(directoryEntries.size(), idx); } - private static Stream testEqualsIgnoringHierarchyWithVariousTypes() { + static Stream testEqualsIgnoringHierarchyWithVariousTypes() { // Basic type equality tests KeySpaceDirectory stringDir1 = new KeySpaceDirectory("dir", KeyType.STRING, "hello"); KeySpaceDirectory stringDir2 = new KeySpaceDirectory("dir", KeyType.STRING, "hello"); @@ -1281,7 +1281,7 @@ private static Stream testEqualsIgnoringHierarchyWithVariousTypes() { } @ParameterizedTest - @MethodSource + @MethodSource("testEqualsIgnoringHierarchyWithVariousTypes") void testEqualsIgnoringHierarchyWithVariousTypes(KeySpaceDirectory dir1, KeySpaceDirectory dir2, boolean shouldBeEqual) { if (shouldBeEqual) { assertTrue(dir1.equalsIgnoringHierarchy(dir2), "Directories should be equal for " + dir1.getKeyType()); @@ -1292,13 +1292,8 @@ void testEqualsIgnoringHierarchyWithVariousTypes(KeySpaceDirectory dir1, KeySpac } assertTrue(dir1.equalsIgnoringHierarchy(dir1), "Directories should always equal themselves"); assertTrue(dir2.equalsIgnoringHierarchy(dir2), "Directories should always equal themselves"); - } - - @Test - void testEqualsIgnoringHierarchyTrivialCases() { - KeySpaceDirectory dir = new KeySpaceDirectory("test", KeyType.STRING, "value"); - assertFalse(dir.equalsIgnoringHierarchy(null), "Directory should not equal null"); - assertFalse(dir.equalsIgnoringHierarchy("value"), "Directory should not equal other classes"); + assertFalse(dir1.equalsIgnoringHierarchy(null), "Directory should not equal null"); + assertFalse(dir1.equalsIgnoringHierarchy(dir1.value), "Directory should not equal other classes"); } @Test From 6db80e8059c930e4a4594e01bf02f8b7fbc18395 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Wed, 15 Oct 2025 12:05:06 -0400 Subject: [PATCH 12/14] Instead of equalsIgnoringHierarchy just use reference equality --- .../keyspace/KeySpaceDirectory.java | 21 -- .../keyspace/KeySpacePathImpl.java | 9 +- .../keyspace/KeySpaceDirectoryTest.java | 186 ++++++------------ 3 files changed, 63 insertions(+), 153 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java index 9e4279d284..0123678a3c 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectory.java @@ -930,27 +930,6 @@ public static KeyType typeOf(@Nullable Object value) { } } - /** - * Compares two KeySpaceDirectory objects for equality based on their intrinsic properties, - * ignoring their position in the directory hierarchy (parent, subdirectories) and wrapper functions. - * - * @param other the other KeySpaceDirectory to compare with - * @return true if the directories have the same name, keyType, and value; false otherwise - */ - @SuppressWarnings("PMD.CompareObjectsWithEquals") - public boolean equalsIgnoringHierarchy(@Nullable Object other) { - if (this == other) { - return true; - } - if (other == null || getClass() != other.getClass()) { - return false; - } - KeySpaceDirectory that = (KeySpaceDirectory) other; - return Objects.equals(name, that.name) && - Objects.equals(keyType, that.keyType) && - areEqual(value, that.value); - } - private static class AnyValue { private AnyValue() { } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java index 4d45311c77..73f8b86151 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpacePathImpl.java @@ -276,9 +276,9 @@ public boolean equals(Object obj) { } KeySpacePath that = (KeySpacePath) obj; - // We don't care whether the two objects exist in the same hierarchy, we validate the relevant bits by comparing - // parents. - boolean directoriesEqual = this.getDirectory().equalsIgnoringHierarchy(that.getDirectory()); + // 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 && @@ -289,8 +289,7 @@ public boolean equals(Object obj) { @Override public int hashCode() { return Objects.hash( - getDirectory().getKeyType(), - getDirectory().getName(), + getDirectory(), KeySpaceDirectory.valueHashCode(getValue()), parent); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index 552001778a..b3ea0ed54e 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -48,9 +48,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; -import org.junit.jupiter.params.provider.FieldSource; -import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -71,7 +68,6 @@ import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.IntStream; -import java.util.stream.Stream; import static com.apple.foundationdb.record.TestHelpers.assertThrows; import static com.apple.foundationdb.record.TestHelpers.eventually; @@ -1227,135 +1223,12 @@ public void testListAcrossTransactions() { assertEquals(directoryEntries.size(), idx); } - static Stream testEqualsIgnoringHierarchyWithVariousTypes() { - // Basic type equality tests - KeySpaceDirectory stringDir1 = new KeySpaceDirectory("dir", KeyType.STRING, "hello"); - KeySpaceDirectory stringDir2 = new KeySpaceDirectory("dir", KeyType.STRING, "hello"); - KeySpaceDirectory stringDir3 = new KeySpaceDirectory("dir", KeyType.STRING, "goodbye"); - KeySpaceDirectory stringDir4 = new KeySpaceDirectory("different_name", KeyType.STRING, "hello"); - - KeySpaceDirectory longDir1 = new KeySpaceDirectory("dir", KeyType.LONG, 42L); - KeySpaceDirectory longDir2 = new KeySpaceDirectory("dir", KeyType.LONG, 42L); - KeySpaceDirectory longDir3 = new KeySpaceDirectory("dir", KeyType.LONG, 100L); - KeySpaceDirectory longDirAsString = new KeySpaceDirectory("dir", KeyType.STRING, "42"); - - // bytes is, important here because testEqualsIgnoringHierarchyWithAllKeyTypes could pass if calling - // .equals on the two values, but this one would not. - KeySpaceDirectory bytesDir1 = new KeySpaceDirectory("dir", KeyType.BYTES, new byte[] { 0x01, 0x02 }); - KeySpaceDirectory bytesDir2 = new KeySpaceDirectory("dir", KeyType.BYTES, new byte[] { 0x01, 0x02 }); - KeySpaceDirectory bytesDir3 = new KeySpaceDirectory("dir", KeyType.BYTES, new byte[] { 0x05, 0x06 }); - - // Wrapper tests - Function wrapper1 = TestWrapper1::new; - Function wrapper2 = TestWrapper2::new; - KeySpaceDirectory dirWithWrapper1 = new KeySpaceDirectory("test", KeyType.STRING, "value", wrapper1); - KeySpaceDirectory dirWithWrapper2 = new KeySpaceDirectory("test", KeyType.STRING, "value", wrapper2); - KeySpaceDirectory dirWithoutWrapper = new KeySpaceDirectory("test", KeyType.STRING, "value", null); - - // ANY_VALUE tests - KeySpaceDirectory anyValueDir1 = new KeySpaceDirectory("test", KeyType.STRING); - KeySpaceDirectory anyValueDir2 = new KeySpaceDirectory("test", KeyType.STRING); - KeySpaceDirectory specificValueDir = new KeySpaceDirectory("test", KeyType.STRING, "specificValue"); - - return Stream.of( - // Basic equality/inequality - Arguments.of(stringDir1, stringDir2, true), - Arguments.of(stringDir1, stringDir3, false), - Arguments.of(stringDir4, stringDir1, false), - Arguments.of(longDir1, longDir2, true), - Arguments.of(longDir1, longDir3, false), - Arguments.of(longDir1, longDirAsString, false), - Arguments.of(bytesDir1, bytesDir2, true), - Arguments.of(bytesDir1, bytesDir3, false), - Arguments.of(stringDir1, longDir1, false), - - // Wrapper tests - different wrappers should be equal - Arguments.of(dirWithWrapper1, dirWithWrapper2, true), - Arguments.of(dirWithWrapper1, dirWithoutWrapper, true), - Arguments.of(dirWithWrapper2, dirWithoutWrapper, true), - - // ANY_VALUE tests - Arguments.of(anyValueDir1, anyValueDir2, true), - Arguments.of(anyValueDir1, specificValueDir, false) - ); - } - - @ParameterizedTest - @MethodSource("testEqualsIgnoringHierarchyWithVariousTypes") - void testEqualsIgnoringHierarchyWithVariousTypes(KeySpaceDirectory dir1, KeySpaceDirectory dir2, boolean shouldBeEqual) { - if (shouldBeEqual) { - assertTrue(dir1.equalsIgnoringHierarchy(dir2), "Directories should be equal for " + dir1.getKeyType()); - assertTrue(dir2.equalsIgnoringHierarchy(dir1), "Directories should be equal for " + dir2.getKeyType() + " (reversed)"); - } else { - assertFalse(dir1.equalsIgnoringHierarchy(dir2), "Directories should not be equal"); - assertFalse(dir2.equalsIgnoringHierarchy(dir1), "Directories should not be equal (reversed)"); - } - assertTrue(dir1.equalsIgnoringHierarchy(dir1), "Directories should always equal themselves"); - assertTrue(dir2.equalsIgnoringHierarchy(dir2), "Directories should always equal themselves"); - assertFalse(dir1.equalsIgnoringHierarchy(null), "Directory should not equal null"); - assertFalse(dir1.equalsIgnoringHierarchy(dir1.value), "Directory should not equal other classes"); - } - - @Test - void testEqualsIgnoringHierarchyWithSubdirectories() { - KeySpaceDirectory parent1 = new KeySpaceDirectory("parent", KeyType.STRING, "test"); - KeySpaceDirectory parent2 = new KeySpaceDirectory("parent", KeyType.STRING, "test"); - - KeySpaceDirectory child1 = new KeySpaceDirectory("child", KeyType.LONG); - KeySpaceDirectory child2 = new KeySpaceDirectory("child", KeyType.LONG); - - parent1.addSubdirectory(child1); - parent2.addSubdirectory(child2); - - assertTrue(parent1.equalsIgnoringHierarchy(parent2), "Parents with equivalent subdirectories should be equal when ignoring hierarchy"); - assertTrue(child1.equalsIgnoringHierarchy(child2), "Children should be equal when ignoring hierarchy"); - - KeySpaceDirectory parent3 = new KeySpaceDirectory("parent", KeyType.STRING, "test"); - KeySpaceDirectory child3 = new KeySpaceDirectory("child", KeyType.STRING); - parent3.addSubdirectory(child3); - - assertTrue(parent1.equalsIgnoringHierarchy(parent3), "Parents should be equal when ignoring subdirectories"); - } - - @Test - void testEqualsIgnoringHierarchyWithParentRelationship() { - KeySpaceDirectory root1 = new KeySpaceDirectory("root", KeyType.STRING, "test"); - KeySpaceDirectory root2 = new KeySpaceDirectory("root", KeyType.STRING, "test"); - - KeySpaceDirectory child1 = new KeySpaceDirectory("child", KeyType.LONG); - KeySpaceDirectory child2 = new KeySpaceDirectory("child", KeyType.LONG); - - root1.addSubdirectory(child1); - root2.addSubdirectory(child2); - - assertTrue(child1.equalsIgnoringHierarchy(child2), "Children with different parents should be equal when ignoring hierarchy"); - } - - @ParameterizedTest - @FieldSource("valueOfEveryType") - void testEqualsIgnoringHierarchyWithAllKeyTypes(KeyTypeValue keyTypeValue) { - KeySpaceDirectory dir1 = new KeySpaceDirectory("dir", keyTypeValue.keyType, keyTypeValue.value); - KeySpaceDirectory dir2 = new KeySpaceDirectory("dir", keyTypeValue.keyType, keyTypeValue.value); - - assertTrue(dir1.equalsIgnoringHierarchy(dir2), "Directories should be equal"); - if (keyTypeValue.keyType != KeyType.NULL) { - assertFalse(dir1.equalsIgnoringHierarchy(new KeySpaceDirectory("dir", keyTypeValue.keyType, keyTypeValue.value2)), - "Directories should not be equal"); - } - } - private static class TestWrapper1 extends KeySpacePathWrapper { public TestWrapper1(KeySpacePath inner) { super(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 @@ -1619,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 + public 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 + public 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 resolveBatch(FDBRecordContext context, String... names) { List> futures = new ArrayList<>(); for (String name : names) { From e4fbffdbb9b8e659d43a497ccbbbd7670dfbd743 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Wed, 15 Oct 2025 12:10:49 -0400 Subject: [PATCH 13/14] Test methods should be package-protected --- .../provider/foundationdb/keyspace/KeySpaceDirectoryTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java index b3ea0ed54e..675cad4cbe 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/KeySpaceDirectoryTest.java @@ -1500,7 +1500,7 @@ public void testPathCompareByValue() { * we'll want to look at other solutions, such as ignoring the hierarchy, or something more tricky. */ @Test - public void testKeySpaceDirectoryEqualsUsesReferenceEquality() { + void testKeySpaceDirectoryEqualsUsesReferenceEquality() { // Create two directories with identical properties KeySpaceDirectory dir1 = new KeySpaceDirectory("test", KeyType.STRING, "value"); KeySpaceDirectory dir2 = new KeySpaceDirectory("test", KeyType.STRING, "value"); @@ -1521,7 +1521,7 @@ public void testKeySpaceDirectoryEqualsUsesReferenceEquality() { } @Test - public void testKeySpaceDirectoryHashCodeFollowsReferenceSemantics() { + void testKeySpaceDirectoryHashCodeFollowsReferenceSemantics() { // Create two directories with identical properties KeySpaceDirectory dir1 = new KeySpaceDirectory("test", KeyType.STRING, "value"); KeySpaceDirectory dir2 = new KeySpaceDirectory("test", KeyType.STRING, "value"); From 03b2d0c844ea496e136745e121d1b7c8b60b5b1e Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Wed, 15 Oct 2025 15:03:43 -0400 Subject: [PATCH 14/14] Update ResolvedKeySpacePathTest to only create one directory structure --- .../keyspace/ResolvedKeySpacePathTest.java | 116 ++++++++++++------ 1 file changed, 80 insertions(+), 36 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java index e3dde94acb..be82a58eb6 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/keyspace/ResolvedKeySpacePathTest.java @@ -83,30 +83,97 @@ static Stream testEqualsHashCode() { @MethodSource("testEqualsHashCode") void testEqualsHashCode(@Nonnull KeyType keyType, boolean constantDirectory, boolean differenceInParent) { @Nonnull TestValuePair values = TYPE_TEST_VALUES.get(keyType); - // Test case 1: Same logical and resolved values (existing test) - ResolvedKeySpacePath path1 = createResolvedPath(keyType, values.getValue1(), values.getValue1(), - createRootParent(), constantDirectory, differenceInParent); - ResolvedKeySpacePath path2 = createResolvedPath(keyType, values.getValue1(), values.getValue1(), - createRootParent(), constantDirectory, differenceInParent); + + // Create a single KeySpace with the appropriate directory structure + KeySpaceDirectory rootDir = new KeySpaceDirectory("root", KeyType.STRING, "root"); + KeySpaceDirectory childDir = constantDirectory + ? new KeySpaceDirectory("test", keyType, values.getValue1()) + : new KeySpaceDirectory("test", keyType); + rootDir.addSubdirectory(childDir); + + // Optionally add a constant child for the differenceInParent test + if (differenceInParent) { + KeySpaceDirectory constantChild = new KeySpaceDirectory("constant", KeyType.STRING, "Constant"); + childDir.addSubdirectory(constantChild); + } + + KeySpace keySpace = new KeySpace(rootDir); + + // Create paths from the same KeySpace + KeySpacePath rootPath1 = keySpace.path("root"); + KeySpacePath rootPath2 = keySpace.path("root"); + + KeySpacePath childPath1; + KeySpacePath childPath2; + if (constantDirectory) { + childPath1 = rootPath1.add("test"); + childPath2 = rootPath2.add("test"); + } else { + childPath1 = rootPath1.add("test", values.getValue1()); + childPath2 = rootPath2.add("test", values.getValue1()); + } + + // Create ResolvedKeySpacePath instances + ResolvedKeySpacePath resolvedRoot1 = new ResolvedKeySpacePath(null, rootPath1, new PathValue("root", null), null); + ResolvedKeySpacePath resolvedRoot2 = new ResolvedKeySpacePath(null, rootPath2, new PathValue("root", null), null); + + ResolvedKeySpacePath path1 = new ResolvedKeySpacePath(resolvedRoot1, childPath1, new PathValue(values.getValue1(), null), null); + ResolvedKeySpacePath path2 = new ResolvedKeySpacePath(resolvedRoot2, childPath2, new PathValue(values.getValue1(), null), null); + + if (differenceInParent) { + KeySpacePath constantChildPath1 = childPath1.add("constant"); + KeySpacePath constantChildPath2 = childPath2.add("constant"); + path1 = new ResolvedKeySpacePath(path1, constantChildPath1, new PathValue("Constant", null), null); + path2 = new ResolvedKeySpacePath(path2, constantChildPath2, new PathValue("Constant", null), null); + } // Test equality contracts assertEquals(path1, path2, "Identical paths should be equal"); assertEquals(path2, path1, "Symmetry: path2.equals(path1)"); assertEquals(path1.hashCode(), path2.hashCode(), "Equal objects must have equal hash codes"); - + // Test inequality when values differ (except NULL type which only has null values) if (keyType != KeyType.NULL) { - ResolvedKeySpacePath path3 = createResolvedPath(keyType, values.getValue2(), createRootParent(), constantDirectory); - assertNotEquals(path1, path3, "Paths with different values should not be equal"); + if (constantDirectory) { + // For constant directories, we need a different directory (and thus different KeySpace) to test different values + // this doesn't really need to be parameterized by value type, since they will always be non-equal due + // to the directories being different + KeySpaceDirectory rootDir3 = new KeySpaceDirectory("root", KeyType.STRING, "root"); + KeySpaceDirectory childDir3 = new KeySpaceDirectory("test", keyType, values.getValue2()); + rootDir3.addSubdirectory(childDir3); + KeySpace keySpace3 = new KeySpace(rootDir3); - assertNotEquals(createResolvedPath(keyType, values.getValue1(), values.getValue2(), createRootParent(), constantDirectory, differenceInParent), - path1, "Paths with different resolved values should not be equal"); - assertNotEquals(createResolvedPath(keyType, values.getValue2(), values.getValue1(), createRootParent(), constantDirectory, differenceInParent), - path1, "Paths with different logical values should not be equal"); + KeySpacePath rootPath3 = keySpace3.path("root"); + KeySpacePath childPath3 = rootPath3.add("test"); + + ResolvedKeySpacePath resolvedRoot3 = new ResolvedKeySpacePath(null, rootPath3, new PathValue("root", null), null); + ResolvedKeySpacePath path3 = new ResolvedKeySpacePath(resolvedRoot3, childPath3, new PathValue(values.getValue2(), null), null); + + assertNotEquals(path1, path3, "Paths with different constant values should not be equal"); + } else { + // For non-constant directories, we can use the same directory with different values + KeySpacePath childPath3 = rootPath1.add("test", values.getValue2()); + ResolvedKeySpacePath path3 = new ResolvedKeySpacePath(resolvedRoot1, childPath3, new PathValue(values.getValue2(), null), null); + assertNotEquals(path1, path3, "Paths with different values should not be equal"); + } + + // Test different resolved value (same logical, different resolved) + KeySpacePath childPath4 = constantDirectory + ? rootPath1.add("test") + : rootPath1.add("test", values.getValue1()); + ResolvedKeySpacePath path4 = new ResolvedKeySpacePath(resolvedRoot1, childPath4, new PathValue(values.getValue2(), null), null); + assertNotEquals(path4, path1, "Paths with different resolved values should not be equal"); + + // Test different logical value (different logical, same resolved) + if (!constantDirectory) { + KeySpacePath childPath5 = rootPath1.add("test", values.getValue2()); + ResolvedKeySpacePath path5 = new ResolvedKeySpacePath(resolvedRoot1, childPath5, new PathValue(values.getValue1(), null), null); + assertNotEquals(path5, path1, "Paths with different logical values should not be equal"); + } } else { assertNull(values.getValue2()); } - + // Test basic contracts assertEquals(path1, path1, "Reflexivity"); assertNotEquals(path1, null, "Null comparison"); @@ -157,29 +224,6 @@ void testRemainderComparedInEquals(boolean constantDirectory) { .orElseThrow(() -> new AssertionError("Paths with different remainders should sometimes have different hash codes")); } - @Nonnull - private ResolvedKeySpacePath createResolvedPath(@Nonnull KeyType keyType, @Nullable Object value, - @Nonnull ResolvedKeySpacePath parent, boolean constantDirectory) { - return createResolvedPath(keyType, value, value, parent, constantDirectory, false); - } - - @Nonnull - private ResolvedKeySpacePath createResolvedPath(@Nonnull KeyType keyType, - @Nullable Object logicalValue, @Nullable Object resolvedValue, - @Nonnull ResolvedKeySpacePath parent, - boolean constantDirectory, final boolean addConstantChild) { - KeySpacePath innerPath = createKeySpacePath(parent, keyType, logicalValue, constantDirectory); - PathValue pathValue = new PathValue(resolvedValue, null); - final ResolvedKeySpacePath resolvedKeySpacePath = new ResolvedKeySpacePath(parent, innerPath, pathValue, null); - if (addConstantChild) { - final ResolvedKeySpacePath resolvedPath = createResolvedPath(KeyType.STRING, - "Constant", "Constant", resolvedKeySpacePath, true, false); - return resolvedPath; - } else { - return resolvedKeySpacePath; - } - } - @Nonnull private KeySpacePath createKeySpacePath(@Nonnull ResolvedKeySpacePath parent, @Nonnull KeyType keyType, @Nullable Object value, boolean constantDirectory) {