From 711ad027ab18d03b67175b19a1e2483e0450b032 Mon Sep 17 00:00:00 2001 From: Robert Brunel Date: Thu, 16 Apr 2026 16:50:44 +0100 Subject: [PATCH] Preserve unconstrained placeholders in `AndPredicate#and()` This change fixes the conjuncts filter in `and()`. Previously, it silently removed unconstrained `Placeholder` instances from the resulting `AndPredicate`. Such placeholders are tautologies but must be kept for the index-matching machinery to work correctly. Removing them can cause incorrect or missed index matches. Testing: * Regression test in `QueryPredicateTest.java`. This fixes #4091. --- .../cascades/predicates/AndPredicate.java | 4 ++- .../predicates/QueryPredicateTest.java | 29 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/AndPredicate.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/AndPredicate.java index 5d36d12fe4..a97cbe194f 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/AndPredicate.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/predicates/AndPredicate.java @@ -187,9 +187,11 @@ public static QueryPredicate and(@Nonnull final Collection conjuncts, final boolean isAtomic) { + // Keep only conjuncts that aren’t tautologies; except for placeholders (where tautology means "no range + // constraint"). All placeholders must be retained for the index-matching machinery to work correctly. final var filteredConjuncts = conjuncts.stream() - .filter(queryPredicate -> !queryPredicate.isTautology()) + .filter(queryPredicate -> (queryPredicate instanceof Placeholder) || !queryPredicate.isTautology()) .collect(ImmutableList.toImmutableList()); if (filteredConjuncts.isEmpty()) { diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/predicates/QueryPredicateTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/predicates/QueryPredicateTest.java index 961b3f13e7..18e9d9093c 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/predicates/QueryPredicateTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/predicates/QueryPredicateTest.java @@ -28,6 +28,7 @@ import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreBase; import com.apple.foundationdb.record.query.expressions.Comparisons; import com.apple.foundationdb.record.query.plan.cascades.AliasMap; +import com.apple.foundationdb.record.query.plan.cascades.CorrelationIdentifier; import com.apple.foundationdb.record.query.plan.cascades.Quantifier; import com.apple.foundationdb.record.query.plan.cascades.predicates.simplification.DefaultQueryPredicateRuleSet; import com.apple.foundationdb.record.query.plan.cascades.predicates.simplification.QueryPredicateWithCnfRuleSet; @@ -58,6 +59,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -391,4 +393,31 @@ void simplifyPredicateTestRemovalRedundantDeepSubPredicates() { QueryPredicateWithDnfRuleSet.ofSimplificationRules()).get(); assertEquals(expectedSimplifiedPredicate, simplifiedPredicate); } + + /** + * Tests that {@link AndPredicate#and} retains placeholders. + * + *

A {@link Placeholder} with no range constraints reports {@code isTautology() == true} because it accepts every + * value. The tautology filter in {@code and()} must not drop such placeholders, as they are necessary for correct + * index matching. + */ + @Test + void andRetainsPlaceholders() { + final var alias = CorrelationIdentifier.of("p0"); + final var value = LiteralValue.ofScalar(42); + final Placeholder placeholder = Placeholder.newInstanceWithoutRanges(value, alias); + + // Sanity-check that the placeholder indeed claims to be a tautology. + assertTrue(placeholder.isTautology()); + + // A conjunction of only an unconstrained placeholder is simplified to just that (not collapsed to TRUE). + final QueryPredicate pred1 = AndPredicate.and(List.of(placeholder)); + assertSame(placeholder, pred1); + + // Mixing a placeholder with a real predicate. The resulting `AndPredicate` must contain the placeholder. + final var pred2 = new ValuePredicate(value, new Comparisons.SimpleComparison(Comparisons.Type.EQUALS, 42)); + final QueryPredicate pred3 = AndPredicate.and(List.of(placeholder, pred2)); + assertTrue(pred3 instanceof AndPredicate); + assertTrue(((AndPredicate)pred3).getChildren().contains(placeholder)); + } }