From ef58730a75640859dd4b9d349fdfaae40bce0c79 Mon Sep 17 00:00:00 2001 From: Youssef Hatem Date: Thu, 25 Sep 2025 12:50:27 +0100 Subject: [PATCH] add new sanity check to `Ordering`. --- .../record/query/plan/cascades/Ordering.java | 28 +++++++- .../query/plan/cascades/OrderingTest.java | 72 +++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Ordering.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Ordering.java index e372da639a..f91006fc6d 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Ordering.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Ordering.java @@ -57,6 +57,7 @@ import java.util.function.BiPredicate; import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Stream; /** * This class captures an ordering property. @@ -1139,12 +1140,37 @@ protected static void noChooseBindingCheck(@Nonnull final SetMultimap bindingMap, + @Nonnull final PartiallyOrderedSet orderingSet) { + final var directionalValuesBuilder = ImmutableSet.builder(); + for (final var valueBindingsEntry : bindingMap.asMap().entrySet()) { + final var bindings = valueBindingsEntry.getValue(); + if (isSingularDirectionalBinding(bindings)) { + directionalValuesBuilder.add(Verify.verifyNotNull(valueBindingsEntry.getKey())); + } + } + final var directionalValues = directionalValuesBuilder.build(); + Verify.verify(directionalValues.size() <= 1 || orderingSet.getDependencyMap() + .entries().stream().flatMap(entry -> Stream.of(entry.getKey(), entry.getValue())) + .collect(ImmutableSet.toImmutableSet()) + .containsAll(directionalValues)); + } + @Nonnull public static Ordering ofOrderingSet(@Nonnull final SetMultimap bindingMap, @Nonnull final PartiallyOrderedSet orderingSet, final boolean isDistinct) { return new Ordering(bindingMap, orderingSet, isDistinct, - normalizationCheckConsumer().andThen(Ordering::singularFixedBindingCheck).andThen(Ordering::noChooseBindingCheck)); + normalizationCheckConsumer().andThen(Ordering::singularFixedBindingCheck).andThen(Ordering::noChooseBindingCheck).andThen(Ordering::noInvalidOrderingCheck)); } @Nonnull diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/OrderingTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/OrderingTest.java index 29e2f500e5..ee49d51fa5 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/OrderingTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/OrderingTest.java @@ -34,11 +34,13 @@ import com.apple.foundationdb.record.query.plan.cascades.values.Value; import com.apple.foundationdb.record.query.plan.cascades.values.ValueTestHelpers; import com.google.common.base.Verify; +import com.google.common.base.VerifyException; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.SetMultimap; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import javax.annotation.Nonnull; @@ -50,6 +52,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; class OrderingTest { @Test @@ -606,4 +609,73 @@ private static List requested(@Nonnull final Object... ob return resultRequestedOrderingParts.build(); } + + @Test + void testNoInvalidOrderingCheckDoesNotThrowOnValidCases() { + final var qov = ValueTestHelpers.qov(); + final var a = ValueTestHelpers.field(qov, "a"); + final var b = ValueTestHelpers.field(qov, "b"); + final var c = ValueTestHelpers.field(qov, "c"); + + // Test case 1: Single directional value - should pass + final var singleDirectionalBindingMap = bindingMap(a, ProvidedSortOrder.ASCENDING); + final var singleDirectionalOrderingSet = PartiallyOrderedSet.of(ImmutableSet.of(a), ImmutableSetMultimap.of()); + + // This should not throw an exception + Assertions.assertDoesNotThrow(() -> + Ordering.noInvalidOrderingCheck(singleDirectionalBindingMap, singleDirectionalOrderingSet)); + + // Test case 2: Multiple directional values with proper dependencies - should pass + final var multiDirectionalBindingMap = bindingMap( + a, ProvidedSortOrder.ASCENDING, + b, ProvidedSortOrder.DESCENDING + ); + final var multiDirectionalOrderingSet = PartiallyOrderedSet.of( + ImmutableSet.of(a, b), + ImmutableSetMultimap.of(b, a) // b depends on a + ); + + // This should not throw an exception + Assertions.assertDoesNotThrow(() -> + Ordering.noInvalidOrderingCheck(multiDirectionalBindingMap, multiDirectionalOrderingSet)); + + // Test case 3: Mix of directional and fixed values - should pass + final var mixedBindingMap = bindingMap( + a, ProvidedSortOrder.ASCENDING, + b, new Comparisons.NullComparison(Comparisons.Type.IS_NULL), + c, ProvidedSortOrder.DESCENDING + ); + final var mixedOrderingSet = PartiallyOrderedSet.of( + ImmutableSet.of(a, b, c), + ImmutableSetMultimap.of(c, a) // c depends on a + ); + + // This should not throw an exception + Assertions.assertDoesNotThrow(() -> + Ordering.noInvalidOrderingCheck(mixedBindingMap, mixedOrderingSet)); + } + + @Test + void testNoInvalidOrderingCheckThrowsCorrectly() { + final var qov = ValueTestHelpers.qov(); + final var a = ValueTestHelpers.field(qov, "a"); + final var b = ValueTestHelpers.field(qov, "b"); + final var c = ValueTestHelpers.field(qov, "c"); + + // Test case: Multiple directional values without proper interdependencies - should fail + final var invalidBindingMap = bindingMap( + a, ProvidedSortOrder.ASCENDING, + b, ProvidedSortOrder.DESCENDING, + c, new Comparisons.NullComparison(Comparisons.Type.IS_NULL) // fixed value + ); + final var invalidOrderingSet = PartiallyOrderedSet.of( + ImmutableSet.of(a, b, c), + ImmutableSetMultimap.of() // No dependencies between directional values a and b + ); + + // This should throw a VerifyException via Verify.verify() + assertThrows(VerifyException.class, () -> + Ordering.noInvalidOrderingCheck(invalidBindingMap, invalidOrderingSet) + ); + } }