From 7435863443944acf0e0e06c22286ad545f6645a1 Mon Sep 17 00:00:00 2001 From: Robert Brunel Date: Wed, 20 May 2026 16:07:54 +0100 Subject: [PATCH] Make `RelationalExpression#equalsWithoutChildren()` compare the result values where necessary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change adds `semanticEqualsForResults(…)` comparisons to any `RelationalExpression#equalsWithoutChildren()` implementations that were missing it. Without this comparison, the Cascades memoizer will unify plans that differ (_only_) in their result value or type, which is incorrect. See Issue #4184 for details. This change does not adjust the `hashCodeWithoutChildren()` methods likewise, as this is not strictly necessary. Leaving these methods untouched avoids needless plan changes (since the hashes are used as a tie-breaker in the cost model). The potentially incorrect memoization prior to this change can trigger a sanity-check in `Reference.insertUnchecked()`, if enabled. Note though that in production the sanity-checks are disabled, so a violation would have gone undetected there. Testing: * Existing test coverage. * Add missing `equals…()` coverage for `RecordQueryDeletePlan`. Fixes #4184. --- .../plan/plans/RecordQueryDeletePlan.java | 5 +- ...RecordQueryFetchFromPartialRecordPlan.java | 4 +- .../plan/plans/RecordQueryFilterPlan.java | 3 + .../plan/plans/RecordQueryInJoinPlan.java | 3 + .../plan/plans/RecordQueryInUnionPlan.java | 3 + .../RecordQueryPredicatesFilterPlan.java | 3 + .../RecordQueryRecursiveLevelUnionPlan.java | 3 + .../RecordQueryUnorderedDistinctPlan.java | 3 + ...dQueryUnorderedPrimaryKeyDistinctPlan.java | 5 +- .../plan/plans/RecordQueryDeletePlanTest.java | 72 +++++++++++++++++++ 10 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDeletePlanTest.java diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDeletePlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDeletePlan.java index 5c7be76902..67fc6c422b 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDeletePlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDeletePlan.java @@ -165,7 +165,10 @@ public boolean equalsWithoutChildren(@Nonnull final RelationalExpression other, if (this == other) { return true; } - return getClass() == other.getClass(); + if (getClass() != other.getClass()) { + return false; + } + return semanticEqualsForResults(other, equivalences); } @Override diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryFetchFromPartialRecordPlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryFetchFromPartialRecordPlan.java index 8c698c7f2b..0484fcbc93 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryFetchFromPartialRecordPlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryFetchFromPartialRecordPlan.java @@ -229,7 +229,9 @@ public boolean equalsWithoutChildren(@Nonnull final RelationalExpression otherEx if (getClass() != otherExpression.getClass()) { return false; } - + if (!semanticEqualsForResults(otherExpression, equivalences)) { + return false; + } final var otherFetchPlan = (RecordQueryFetchFromPartialRecordPlan)otherExpression; return fetchIndexRecords == otherFetchPlan.fetchIndexRecords; } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryFilterPlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryFilterPlan.java index 3623e2561f..bebcf7fc53 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryFilterPlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryFilterPlan.java @@ -153,6 +153,9 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi if (getClass() != otherExpression.getClass()) { return false; } + if (!semanticEqualsForResults(otherExpression, equivalencesMap)) { + return false; + } final RecordQueryFilterPlan otherPlan = (RecordQueryFilterPlan)otherExpression; return conjunctedFilter.equals(otherPlan.getConjunctedFilter()); } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryInJoinPlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryInJoinPlan.java index 18233365a5..219686b428 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryInJoinPlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryInJoinPlan.java @@ -209,6 +209,9 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi if (getClass() != otherExpression.getClass()) { return false; } + if (!semanticEqualsForResults(otherExpression, equivalencesMap)) { + return false; + } final RecordQueryInJoinPlan other = (RecordQueryInJoinPlan)otherExpression; return inSource.equals(other.inSource); } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryInUnionPlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryInUnionPlan.java index c7562ab4b6..85eedba180 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryInUnionPlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryInUnionPlan.java @@ -283,6 +283,9 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi if (getClass() != otherExpression.getClass()) { return false; } + if (!semanticEqualsForResults(otherExpression, equivalencesMap)) { + return false; + } final RecordQueryInUnionPlan other = (RecordQueryInUnionPlan)otherExpression; return inSources.equals(other.inSources) && comparisonKeyFunction.equals(other.comparisonKeyFunction); } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryPredicatesFilterPlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryPredicatesFilterPlan.java index c698350684..395e30c8e3 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryPredicatesFilterPlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryPredicatesFilterPlan.java @@ -180,6 +180,9 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi return false; } final var otherPlan = (RecordQueryPredicatesFilterPlan)otherExpression; + if (!semanticEqualsForResults(otherPlan, equivalencesMap)) { + return false; + } final var otherPredicates = otherPlan.getPredicates(); if (predicates.size() != otherPredicates.size()) { return false; diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryRecursiveLevelUnionPlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryRecursiveLevelUnionPlan.java index ab7622b104..94704cf3b1 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryRecursiveLevelUnionPlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryRecursiveLevelUnionPlan.java @@ -284,6 +284,9 @@ public boolean equalsWithoutChildren(@Nonnull final RelationalExpression otherEx if (!(otherExpression instanceof RecordQueryRecursiveLevelUnionPlan)) { return false; } + if (!semanticEqualsForResults(otherExpression, equivalences)) { + return false; + } final var otherRecursiveUnionQueryPlan = (RecordQueryRecursiveLevelUnionPlan)otherExpression; diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryUnorderedDistinctPlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryUnorderedDistinctPlan.java index 80858bab85..71b24baf02 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryUnorderedDistinctPlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryUnorderedDistinctPlan.java @@ -182,6 +182,9 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi if (getClass() != otherExpression.getClass()) { return false; } + if (!semanticEqualsForResults(otherExpression, equivalencesMap)) { + return false; + } return comparisonKey.equals(((RecordQueryUnorderedDistinctPlan)otherExpression).comparisonKey); } diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryUnorderedPrimaryKeyDistinctPlan.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryUnorderedPrimaryKeyDistinctPlan.java index 25504f1ac1..1f7091f941 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryUnorderedPrimaryKeyDistinctPlan.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryUnorderedPrimaryKeyDistinctPlan.java @@ -165,7 +165,10 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi if (this == otherExpression) { return true; } - return (getClass() == otherExpression.getClass()); + if (getClass() != otherExpression.getClass()) { + return false; + } + return semanticEqualsForResults(otherExpression, equivalencesMap); } @SuppressWarnings("EqualsWhichDoesntCheckParameterClass") diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDeletePlanTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDeletePlanTest.java new file mode 100644 index 0000000000..27d1eb224a --- /dev/null +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryDeletePlanTest.java @@ -0,0 +1,72 @@ +/* + * RecordQueryDeletePlanTest.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2026 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.query.plan.plans; + +import com.apple.foundationdb.record.query.plan.ScanComparisons; +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.Reference; +import com.apple.foundationdb.record.query.plan.cascades.typing.Type; +import com.google.common.collect.ImmutableSet; +import org.junit.jupiter.api.Test; + +import java.util.List; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class RecordQueryDeletePlanTest { + + private static RecordQueryDeletePlan deletePlanOver(final Type flowedType, + final CorrelationIdentifier alias) { + final var scan = new RecordQueryScanPlan(ImmutableSet.of("R"), flowedType, null, + ScanComparisons.EMPTY, false, false); + return RecordQueryDeletePlan.deletePlan(Quantifier.physical(Reference.plannedOf(scan), alias)); + } + + private static Type recordTypeWith(final Type.TypeCode fieldType) { + return Type.Record.fromFields(false, List.of( + Type.Record.Field.of(Type.primitiveType(fieldType), Optional.of("a")))); + } + + @Test + void equalsWithoutChildrenReturnsFalseWhenResultValuesDiffer() { + final var typeLong = recordTypeWith(Type.TypeCode.LONG); + final var deletePlan1 = deletePlanOver(typeLong, CorrelationIdentifier.of("q1")); + final var deletePlan2 = deletePlanOver(typeLong, CorrelationIdentifier.of("q2")); + + assertFalse(deletePlan1.equalsWithoutChildren(deletePlan2, AliasMap.emptyMap())); + assertFalse(deletePlan2.equalsWithoutChildren(deletePlan1, AliasMap.emptyMap())); + } + + @Test + void equalsWithoutChildrenReturnsTrueWhenResultValuesMatch() { + final var typeLong = recordTypeWith(Type.TypeCode.LONG); + final var alias = CorrelationIdentifier.of("q"); + final var deletePlan1 = deletePlanOver(typeLong, alias); + final var deletePlan2 = deletePlanOver(typeLong, alias); + + assertTrue(deletePlan1.equalsWithoutChildren(deletePlan2, AliasMap.emptyMap())); + assertTrue(deletePlan1.equalsWithoutChildren(deletePlan1, AliasMap.emptyMap())); + } +}