Don't memoize correlation sets in planner objects (#3365)

We should not use a memoizing supplier to cache the correlation sets of
a quantifier since the reference the quantifier ranges over is mutable
with respect to the contained expressions. While these expressions are
rarely exposing new correlations it may be that a new expression is
added that has no correlations at all. If we prune to that expression
later, the correlation set, if memoized, is wrong.

Example
```
SELECT *
FROM A, (SELECT * FROM B WHERE A = 5 AND FALSE)
```

The inner
```
...(SELECT * FROM B WHERE A = 5 AND FALSE)...
```
has `B` in its correlation set. That expression, however, can also be
simplified to:
```
...(SELECT * FROM B WHERE FALSE)...
```
(and even further). The correlation set is empty.

This PR deals with the change of removing the `MemoizingSupplier` as
well as the fallout from now exposed bugs.

Author: normen662

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Quantifier.java

@@ -94,13 +94,6 @@ public abstract class Quantifier implements Correlated<Quantifier> {
     @Nonnull
     private final CorrelationIdentifier alias;
 
-    /**
-     * As a quantifier is immutable, the correlated set can be computed lazily and then cached. This supplier
-     * represents that cached set.
-     */
-    @Nonnull
-    private final Supplier<Set<CorrelationIdentifier>> correlatedToSupplier;
-
     /**
      * As a quantifier is immutable, the columns that flow along the quantifier can be lazily computed.
      */
@@ -584,7 +577,6 @@ public static Physical physical(@Nonnull final Reference reference,
 
     protected Quantifier(@Nonnull final CorrelationIdentifier alias) {
         this.alias = alias;
-        this.correlatedToSupplier = Suppliers.memoize(() -> getRangesOver().getCorrelatedTo());
         this.flowedColumnsSupplier = Suppliers.memoize(this::computeFlowedColumns);
         this.flowedValuesSupplier = Suppliers.memoize(this::computeFlowedValues);
         // Call debugger hook for this new quantifier.
@@ -672,7 +664,7 @@ public String toString() {
     @Nonnull
     @Override
     public Set<CorrelationIdentifier> getCorrelatedTo() {
-        return correlatedToSupplier.get();
+        return getRangesOver().getCorrelatedTo();
     }
 
     /**

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Reference.java

@@ -704,10 +704,26 @@ private static boolean isMemoizedExpression(@Nonnull final RelationalExpression
             return false;
         }
 
+        //
+        // We need to check if the expressions' correlatedTo sets are identical under the consideration of the given
+        // map of alias equivalences. If they are not, the member is not memoizing the otherExpression.
+        //
+        // Specific example:
+        // SELECT *
+        // FROM A a, SELECT * FROM B WHERE a.x = b.y
+        //
+        // Let's assume the inner 'SELECT * FROM B WHERE a.x = b.y' already is member (in the memoization structure).
+        // This member is correlated to 'a'.
+        // However, if we didn't do the check for correlation sets here, we would satisfy the following otherExpression:
+        // SELECT * FROM A WHERE a.x = b.y is correlated to b
+        //
         final Set<CorrelationIdentifier> originalCorrelatedTo = member.getCorrelatedTo();
-        final Set<CorrelationIdentifier> translatedCorrelatedTo = equivalenceMap.definesOnlyIdentities()
+        final Set<CorrelationIdentifier> translatedCorrelatedTo =
+                equivalenceMap.definesOnlyIdentities()
                 ? originalCorrelatedTo
-                : originalCorrelatedTo.stream().map(id -> equivalenceMap.getTargetOrDefault(id, id)).collect(ImmutableSet.toImmutableSet());
+                : originalCorrelatedTo.stream()
+                        .map(alias -> equivalenceMap.getTargetOrDefault(alias, alias))
+                        .collect(ImmutableSet.toImmutableSet());
         if (!translatedCorrelatedTo.equals(otherExpression.getCorrelatedTo())) {
             return false;
         }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ScanWithFetchMatchCandidate.java

@@ -54,7 +54,7 @@ static Optional<Value> pushValueThroughFetch(@Nonnull final Value toBePushedValu
                                                  @Nonnull final CorrelationIdentifier sourceAlias,
                                                  @Nonnull final CorrelationIdentifier targetAlias,
                                                  @Nonnull final Iterable<? extends Value> providedValuesFromIndex) {
-        if (!isOfPushableTypes(toBePushedValue)) {
+        if (!isOfPushableTypesOrConstant(toBePushedValue, sourceAlias)) {
             return Optional.empty();
         }
 
@@ -76,12 +76,16 @@ static Optional<Value> pushValueThroughFetch(@Nonnull final Value toBePushedValu
         return translatedValueOptional.filter(translatedValue -> !translatedValue.getCorrelatedTo().contains(sourceAlias));
     }
 
-    private static boolean isOfPushableTypes(@Nonnull Value toBePushedValue) {
+    private static boolean isOfPushableTypesOrConstant(@Nonnull final Value toBePushedValue,
+                                                       @Nonnull final CorrelationIdentifier sourceAlias) {
+        if (!toBePushedValue.getCorrelatedTo().contains(sourceAlias)) {
+            return true;
+        }
         if (toBePushedValue instanceof FieldValue) {
             return true;
         } else if (toBePushedValue instanceof RecordConstructorValue) {
             return ((RecordConstructorValue)toBePushedValue).getColumns().stream()
-                    .allMatch(column -> isOfPushableTypes(column.getValue()));
+                    .allMatch(column -> isOfPushableTypesOrConstant(column.getValue(), sourceAlias));
         } else {
             // Effectively, this check is needed because of values like the VersionValue, which aren't
             // accessible without a record fetch, even if the index entry contains a VersionValue. We

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/rules/PushFilterThroughFetchRule.java

@@ -32,7 +32,6 @@
 import com.apple.foundationdb.record.query.plan.cascades.predicates.PredicateWithValue;
 import com.apple.foundationdb.record.query.plan.cascades.predicates.QueryPredicate;
 import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
-import com.apple.foundationdb.record.query.plan.cascades.values.Value;
 import com.apple.foundationdb.record.query.plan.plans.RecordQueryFetchFromPartialRecordPlan;
 import com.apple.foundationdb.record.query.plan.plans.RecordQueryPlan;
 import com.apple.foundationdb.record.query.plan.plans.RecordQueryPredicatesFilterPlan;
@@ -43,7 +42,6 @@
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import java.util.List;
-import java.util.Objects;
 import java.util.Optional;
 
 import static com.apple.foundationdb.record.query.plan.cascades.matching.structure.QuantifierMatchers.physicalQuantifier;
@@ -229,21 +227,24 @@ public void onMatch(@Nonnull final ImplementationCascadesRuleCall call) {
     }
 
     @Nullable
-    private QueryPredicate pushLeafPredicate(@Nonnull RecordQueryFetchFromPartialRecordPlan fetchPlan,
-                                             @Nonnull CorrelationIdentifier oldInnerAlias,
-                                             @Nonnull CorrelationIdentifier newInnerAlias,
+    private QueryPredicate pushLeafPredicate(@Nonnull final RecordQueryFetchFromPartialRecordPlan fetchPlan,
+                                             @Nonnull final CorrelationIdentifier oldInnerAlias,
+                                             @Nonnull final CorrelationIdentifier newInnerAlias,
                                              @Nonnull final QueryPredicate leafPredicate) {
         if (!(leafPredicate instanceof PredicateWithValue)) {
             // Only values depend on aliases -- returning this leaf is ok as it
             // appears to be pushable as is.
             return leafPredicate;
         }
-        final PredicateWithValue predicateWithValue = (PredicateWithValue)leafPredicate;
-
-        final Value value = Objects.requireNonNull(predicateWithValue.getValue());
-        final Optional<Value> pushedValueOptional = fetchPlan.pushValue(value, oldInnerAlias, newInnerAlias);
+        final var predicateWithValue = (PredicateWithValue)leafPredicate;
+        final var pushedLeafPredicateOptional =
+                predicateWithValue.translateValueAndComparisonsMaybe(
+                        value -> fetchPlan.pushValue(value, oldInnerAlias, newInnerAlias),
+                        comparison -> comparison.replaceValuesMaybe(value -> {
+                            return fetchPlan.pushValue(value, oldInnerAlias, newInnerAlias);
+                        }));
         // Something went wrong when attempting to push this value through the fetch.
         // We must return null to prevent pushing of this conjunct.
-        return pushedValueOptional.map(predicateWithValue::withValue).orElse(null);
+        return pushedLeafPredicateOptional.orElse(null);
     }
 }

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/IndexedValue.java

@@ -54,16 +54,16 @@ public IndexedValue() {
         this(Type.primitiveType(Type.TypeCode.UNKNOWN));
     }
 
+    public IndexedValue(@Nonnull final Type resultType) {
+        this.resultType = resultType;
+    }
+
     @Nonnull
     @Override
     protected Iterable<? extends Value> computeChildren() {
         return ImmutableList.of();
     }
 
-    public IndexedValue(@Nonnull final Type resultType) {
-        this.resultType = resultType;
-    }
-
     @Nonnull
     @Override
     public Type getResultType() {

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/RecordTypeValue.java

@@ -75,7 +75,7 @@ public <M extends Message> Object eval(@Nullable final FDBRecordStoreBase<M> sto
     @Nonnull
     @Override
     protected Iterable<? extends Value> computeChildren() {
-        return ImmutableList.of();
+        return ImmutableList.of(in);
     }
 
     @Nonnull

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryAggregateIndexPlan.java

@@ -47,10 +47,10 @@
 import com.apple.foundationdb.record.query.plan.cascades.FinalMemoizer;
 import com.apple.foundationdb.record.query.plan.cascades.MatchCandidate;
 import com.apple.foundationdb.record.query.plan.cascades.Quantifier;
+import com.apple.foundationdb.record.query.plan.cascades.explain.ExplainPlanVisitor;
 import com.apple.foundationdb.record.query.plan.cascades.explain.NodeInfo;
 import com.apple.foundationdb.record.query.plan.cascades.explain.PlannerGraph;
 import com.apple.foundationdb.record.query.plan.cascades.expressions.RelationalExpression;
-import com.apple.foundationdb.record.query.plan.cascades.explain.ExplainPlanVisitor;
 import com.apple.foundationdb.record.query.plan.cascades.typing.TypeRepository;
 import com.apple.foundationdb.record.query.plan.cascades.values.Value;
 import com.apple.foundationdb.record.query.plan.cascades.values.translation.TranslationMap;
@@ -83,11 +83,14 @@ public class RecordQueryAggregateIndexPlan implements RecordQueryPlanWithNoChild
     private final String recordTypeName;
     @Nonnull
     private final IndexKeyValueToPartialRecord toRecord;
+    // TODO the following value should not be part of this plan
+    //      https://github.com/FoundationDB/fdb-record-layer/issues/3367
     @Nonnull
     private final Value resultValue;
+    // TODO the following value should not be part of this plan
+    //      https://github.com/FoundationDB/fdb-record-layer/issues/3367
     @Nonnull
     private final Value groupByResultValue;
-
     @Nonnull
     private final QueryPlanConstraint constraint;
 
@@ -227,6 +230,7 @@ public boolean hasLoadBykeys() {
     @Nonnull
     @Override
     public Value getResultValue() {
+        // TODO this is bad since the derivations property or others might pick this up without understanding it
         return resultValue;
     }
 
@@ -241,8 +245,6 @@ public String toString() {
     public Set<CorrelationIdentifier> getCorrelatedTo() {
         final var result = ImmutableSet.<CorrelationIdentifier>builder();
         result.addAll(indexPlan.getCorrelatedTo());
-        result.addAll(resultValue.getCorrelatedTo());
-        result.addAll(groupByResultValue.getCorrelatedTo());
         return result.build();
     }
 
@@ -260,13 +262,11 @@ public RecordQueryAggregateIndexPlan translateCorrelations(@Nonnull final Transl
 
         final var translatedIndexPlan = indexPlan.translateCorrelations(translationMap,
                 shouldSimplifyValues, translatedQuantifiers);
-        final var newResultValue = resultValue.translateCorrelations(translationMap, shouldSimplifyValues);
-        final var newGroupByResultValue = groupByResultValue.translateCorrelations(translationMap, shouldSimplifyValues);
 
         // this is ok as there are no new quantifiers
-        if (translatedIndexPlan != indexPlan || newResultValue != resultValue || newGroupByResultValue != groupByResultValue) {
-            return new RecordQueryAggregateIndexPlan(translatedIndexPlan, recordTypeName, toRecord, newResultValue,
-                    newGroupByResultValue, constraint);
+        if (translatedIndexPlan != indexPlan) {
+            return new RecordQueryAggregateIndexPlan(translatedIndexPlan, recordTypeName, toRecord, resultValue,
+                    groupByResultValue, constraint);
         }
         return this;
     }
@@ -297,9 +297,7 @@ public boolean equalsWithoutChildren(@Nonnull RelationalExpression otherExpressi
         final RecordQueryAggregateIndexPlan other = (RecordQueryAggregateIndexPlan) otherExpression;
         return indexPlan.structuralEquals(other.indexPlan, equivalencesMap) &&
                 recordTypeName.equals(other.recordTypeName) &&
-                toRecord.equals(other.toRecord) &&
-                resultValue.semanticEquals(other.resultValue, equivalencesMap) &&
-                groupByResultValue.semanticEquals(other.groupByResultValue, equivalencesMap);
+                toRecord.equals(other.toRecord);
     }
 
     @SuppressWarnings("EqualsWhichDoesntCheckParameterClass")
@@ -315,7 +313,7 @@ public int hashCode() {
 
     @Override
     public int hashCodeWithoutChildren() {
-        return Objects.hash(indexPlan, recordTypeName, toRecord, resultValue, groupByResultValue);
+        return Objects.hash(indexPlan, recordTypeName, toRecord);
     }
 
     @Override

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/query/FDBNestedRepeatedQueryTest.java

@@ -1672,7 +1672,8 @@ void chooseCorrectAggregateIndex() {
             // Verify that the byKey plan chooses the index that keeps entries together
             final RecordQueryPlan byKeyPlan = planGraph(this::querySumIntValueByKey);
             assertThat(byKeyPlan.getUsedIndexes(), contains(SUM_VALUE_BY_KEY));
-            assertEquals(byKeyPlan, planGraph(this::querySumIntValueByKey, SUM_VALUE_BY_KEY));
+            final var actual = planGraph(this::querySumIntValueByKey, SUM_VALUE_BY_KEY);
+            assertEquals(byKeyPlan, actual);
 
             // Verify that the other byWholeRecord plan chooses the index that does not keep entries together
             final RecordQueryPlan byWholeRecordPlan = planGraph(this::querySumIntValueForRecordByKey);

fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/query/plan/cascades/values/simplification/ValueComputationTest.java

@@ -174,7 +174,7 @@ void testPullUpValue2() {
         final var new_x_b = FieldValue.ofFieldNames(upperCurrentValue, ImmutableList.of("_1", "x", "xb"));
 
         final var expectedMap = ImmutableMap.of(
-                record_type, new RecordTypeValue(QuantifiedObjectValue.of(UPPER_ALIAS, new Type.AnyRecord(true))),
+                record_type, new RecordTypeValue(FieldValue.ofFieldName(upperCurrentValue, "_1")),
                 _a_ab__plus__x_xb, (Value)new ArithmeticValue.AddFn().encapsulate(ImmutableList.of(new_a_b, new_x_b)));
         Assertions.assertEquals(expectedMap, resultsMap);
     }