Fix instability in the query planner (#3185)

This fixes a number of instabilities found in Cascades when planning
certain type of queries that end up generating a relatively large number
of variations that might end up storing actual duplicates in the
memoization structure (which is permissible).

Essentially, the following issues were detected and resolved in this PR:
1. **Fix instability in hash calculation**: Certain expressions had
their hash unstable, because it includes hashing an `enum` member, which
is [known to be
unstable](https://bugs.openjdk.org/browse/JDK-8050217?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel).
This issue made it very difficult to compare planner traces from
different runs, since a planner trace might include the hash codes of
some expressions, and the hash code could change across different JVMs,
causing the comparator to report false negatives.
2. **Set instantiation with deterministic traversal order**: Certain
instantiations of `Maps.newIdentityHashMap` and `Sets.newHashSet` are
replaced with `LinkedIdentityMap`, resp. `LinkedIdentitySet`.
3. **Fix instability detected in Guava `StandardMutableNetwork`**: We
use Guava [`Network`
](https://guava.dev/releases/20.0/api/docs/com/google/common/graph/Network.html)
for the planner memoization structure, internally, the network uses a
hash map to store the parallel edges between network vertices, this is
exactly why we were getting the alternatives in different order as
explained above. Due to the way the `Network` code is written, it is not
possible to pass the (type of the) map to the builder, therefore, I went
ahead and copied enough of the code to be able to modify the map
construction and make it stable, which effectively fixed the issue, but
ideally, Guava should make it possible to control how these maps are
created, making it easier to use and reason about the code.
This fixes #3191.

---------

Co-authored-by: Normen Seemann <[email protected]>

Author: hatyo

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/expressions/Comparisons.java

@@ -1679,7 +1679,7 @@ public int hashCode() {
         }
 
         public int computeHashCode() {
-            return Objects.hash(type, relatedByEquality());
+            return Objects.hash(type.name(), relatedByEquality());
         }
 
         private Set<String> relatedByEquality() {
@@ -2091,7 +2091,7 @@ public boolean equals(Object o) {
 
         @Override
         public int hashCode() {
-            return Objects.hash(type);
+            return Objects.hash(type.name());
         }
 
         @Override

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

@@ -32,13 +32,15 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 
 import javax.annotation.Nonnull;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Deque;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
@@ -241,8 +243,7 @@ public Reference memoizeExpression(@Nonnull final RelationalExpression expressio
         if (expression.getQuantifiers().isEmpty()) {
             return memoizeLeafExpression(expression);
         }
-
-        Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.BEGIN)));
+        Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.begin()));
         try {
             Preconditions.checkArgument(!(expression instanceof RecordQueryPlan));
 
@@ -278,28 +279,33 @@ public Reference memoizeExpression(@Nonnull final RelationalExpression expressio
                 commonReferencingExpressions.retainAll(referencingExpressionsIterator.next());
             }
 
-            for (final var commonReferencingExpression : commonReferencingExpressions) {
-                if (Reference.isMemoizedExpression(commonReferencingExpression, expression)) {
-                    Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.REUSED)));
-                    final var reference = expressionToReferenceMap.get(commonReferencingExpression);
-                    Verify.verifyNotNull(reference);
-                    Verify.verify(reference != this.root);
-                    return reference;
-                }
+            commonReferencingExpressions.removeIf(commonReferencingExpression -> !Reference.isMemoizedExpression(expression, commonReferencingExpression));
+
+            if (!commonReferencingExpressions.isEmpty()) {
+                Debugger.withDebugger(debugger ->
+                        debugger.onEvent(Debugger.InsertIntoMemoEvent.reusedExpWithReferences(expression,
+                                commonReferencingExpressions.stream()
+                                        .map(expressionToReferenceMap::get)
+                                        .collect(ImmutableList.toImmutableList()))));
+                final var reference = expressionToReferenceMap.get(Objects.requireNonNull(Iterables.getFirst(commonReferencingExpressions, null)));
+                Verify.verifyNotNull(reference);
+                Verify.verify(reference != this.root);
+                return reference;
             }
-            Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.NEW)));
+
+            Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.newExp(expression)));
             final var newRef = Reference.of(expression);
             traversal.addExpression(newRef, expression);
             return newRef;
         } finally {
-            Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.END)));
+            Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.end()));
         }
     }
 
     @Nonnull
     @Override
     public Reference memoizeLeafExpression(@Nonnull final RelationalExpression expression) {
-        Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.BEGIN)));
+        Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.begin()));
         try {
             Preconditions.checkArgument(!(expression instanceof RecordQueryPlan));
             Preconditions.checkArgument(expression.getQuantifiers().isEmpty());
@@ -309,17 +315,17 @@ public Reference memoizeLeafExpression(@Nonnull final RelationalExpression expre
             for (final var leafRef : leafRefs) {
                 for (final var member : leafRef.getMembers()) {
                     if (Reference.isMemoizedExpression(expression, member)) {
-                        Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.REUSED)));
+                        Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.reusedExp(expression)));
                         return leafRef;
                     }
                 }
             }
-            Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.NEW)));
+            Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.newExp(expression)));
             final var newRef = Reference.of(expression);
             traversal.addExpression(newRef, expression);
             return newRef;
         } finally {
-            Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.END)));
+            Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.end()));
         }
     }
 
@@ -351,7 +357,8 @@ public Reference memoizeReference(@Nonnull final Reference reference) {
     @Nonnull
     private Reference memoizeExpressionsExactly(@Nonnull final Collection<? extends RelationalExpression> expressions,
                                                 @Nonnull Function<Set<? extends RelationalExpression>, Reference> referenceCreator) {
-        Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.BEGIN)));
+        Debugger.withDebugger(debugger -> expressions.forEach(
+                expression -> debugger.onEvent(Debugger.InsertIntoMemoEvent.begin())));
         try {
             final var expressionSet = new LinkedIdentitySet<>(expressions);
 
@@ -367,19 +374,21 @@ private Reference memoizeExpressionsExactly(@Nonnull final Collection<? extends
                 final Optional<Reference> memoizedRefMaybe = findExpressionsInMemo(expressionSet);
                 if (memoizedRefMaybe.isPresent()) {
                     Debugger.withDebugger(debugger ->
-                            expressionSet.forEach(plan -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.REUSED))));
+                            expressionSet.forEach(
+                                    plan -> debugger.onEvent(Debugger.InsertIntoMemoEvent.reusedExpWithReferences(plan, ImmutableList.of(memoizedRefMaybe.get())))));
                     return memoizedRefMaybe.get();
                 }
             }
 
             final var newRef = referenceCreator.apply(expressionSet);
             for (final var plan : expressionSet) {
-                Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.NEW)));
+                Debugger.withDebugger(debugger -> expressions.forEach(
+                        expression -> debugger.onEvent(Debugger.InsertIntoMemoEvent.newExp(expression))));
                 traversal.addExpression(newRef, plan);
             }
             return newRef;
         } finally {
-            Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.END)));
+            Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.end()));
         }
     }
 

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

@@ -128,6 +128,11 @@ public void replaceAll(final BiFunction<? super K, ? super V, ? extends V> funct
         map.replaceAll((wrappedK, v) -> function.apply(wrappedK.get(), v));
     }
 
+    @Override
+    public V remove(final Object key) {
+        return map.remove(identity.wrap(key));
+    }
+
     @Override
     public boolean equals(final Object o) {
         if (!(o instanceof LinkedIdentityMap)) {

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

@@ -185,18 +185,23 @@ public boolean insert(@Nonnull final RelationalExpression newValue) {
      *         otherwise.
      */
     private boolean insert(@Nonnull final RelationalExpression newValue, @Nullable final Map<PlanProperty<?>, ?> precomputedPropertiesMap) {
-        Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.BEGIN)));
+        Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.begin()));
         try {
             final boolean containsInMemo = containsInMemo(newValue);
-            Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(containsInMemo ? Debugger.Location.REUSED : Debugger.Location.NEW)));
-
+            Debugger.withDebugger(debugger -> {
+                if (containsInMemo) {
+                    debugger.onEvent(Debugger.InsertIntoMemoEvent.reusedExpWithReferences(newValue, ImmutableList.of(this)));
+                } else {
+                    debugger.onEvent(Debugger.InsertIntoMemoEvent.newExp(newValue));
+                }
+            });
             if (!containsInMemo) {
                 insertUnchecked(newValue, precomputedPropertiesMap);
                 return true;
             }
             return false;
         } finally {
-            Debugger.withDebugger(debugger -> debugger.onEvent(new Debugger.InsertIntoMemoEvent(Debugger.Location.END)));
+            Debugger.withDebugger(debugger -> debugger.onEvent(Debugger.InsertIntoMemoEvent.end()));
         }
     }
 

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

@@ -25,13 +25,14 @@
 import com.apple.foundationdb.record.query.plan.cascades.expressions.RelationalExpression;
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Maps;
 import com.google.common.collect.Multimaps;
 import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
+import com.google.common.graph.ElementOrder;
 import com.google.common.graph.EndpointPair;
 import com.google.common.graph.MutableNetwork;
 import com.google.common.graph.NetworkBuilder;
+import com.google.common.graph.StableStandardMutableNetwork;
 
 import javax.annotation.Nonnull;
 import java.util.Set;
@@ -172,15 +173,19 @@ public void removeExpression(@Nonnull final Reference reference, @Nonnull final
      * @return a new traversal object
      */
     public static Traversal withRoot(final Reference rootRef) {
+        // This diverges from the standard construction of Guava network; it delegates the construction to a slightly
+        // modified version that guarantees a stable iteration order over the edges, which is required to guarantee a
+        // deterministic planning behavior.
         final MutableNetwork<Reference, ReferencePath> network =
-                NetworkBuilder.directed()
+                new StableStandardMutableNetwork<>(NetworkBuilder.directed()
                         .allowsParallelEdges(true)
                         .allowsSelfLoops(true)
-                        .build();
+                        .edgeOrder(ElementOrder.insertion())
+                        .nodeOrder(ElementOrder.insertion()));
 
         final SetMultimap<RelationalExpression, Reference> containedInMap =
-                Multimaps.newSetMultimap(Maps.newIdentityHashMap(), Sets::newHashSet);
-        final Set<Reference> leafRefs = Sets.newHashSet();
+                Multimaps.newSetMultimap(new LinkedIdentityMap<>(), LinkedIdentitySet::new);
+        final Set<Reference> leafRefs = new LinkedIdentitySet<>();
         collectNetwork(network, containedInMap, leafRefs, rootRef);
 
         return new Traversal(rootRef, network, containedInMap, leafRefs);

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

@@ -46,12 +46,15 @@
 import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PTransformRuleCallEvent;
 import com.apple.foundationdb.record.query.plan.cascades.debug.eventprotos.PTranslateCorrelationsEvent;
 import com.apple.foundationdb.record.query.plan.cascades.expressions.RelationalExpression;
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.protobuf.Message;
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
+import java.util.Collection;
 import java.util.Deque;
+import java.util.List;
 import java.util.Optional;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -919,11 +922,26 @@ public PEvent.Builder toEventBuilder() {
      * structures of the planner.
      */
     class InsertIntoMemoEvent implements Event {
+        @Nullable
+        private final RelationalExpression expression;
+
         @Nonnull
         private final Location location;
 
-        public InsertIntoMemoEvent(@Nonnull final Location location) {
+        @Nonnull
+        private final List<Reference> reusedExpressionReferences;
+
+        private InsertIntoMemoEvent(@Nonnull final Location location,
+                                    @Nullable final RelationalExpression expression,
+                                    @Nonnull final Collection<Reference> reusedExpressionReferences) {
+            if (expression != null) {
+                Debugger.registerExpression(expression);
+            }
+            this.expression = expression;
             this.location = location;
+            this.reusedExpressionReferences = ImmutableList.copyOf(reusedExpressionReferences);
+            // Call debugger hook to potentially register this new reference.
+            this.reusedExpressionReferences.forEach(Debugger::registerReference);
         }
 
         @Override
@@ -938,6 +956,16 @@ public Shorthand getShorthand() {
             return Shorthand.INSERT_INTO_MEMO;
         }
 
+        @Nullable
+        public RelationalExpression getExpression() {
+            return expression;
+        }
+
+        @Nonnull
+        public Collection<Reference> getReusedExpressionReferences() {
+            return reusedExpressionReferences;
+        }
+
         @Nonnull
         @Override
         public Location getLocation() {
@@ -947,9 +975,15 @@ public Location getLocation() {
         @Nonnull
         @Override
         public PInsertIntoMemoEvent toProto() {
-            return PInsertIntoMemoEvent.newBuilder()
+            final var builder = PInsertIntoMemoEvent.newBuilder()
                     .setLocation(getLocation().name())
-                    .build();
+                    .addAllReusedExpressionReferences(getReusedExpressionReferences().stream()
+                            .map(Event::toReferenceProto)
+                            .collect(ImmutableList.toImmutableList()));
+            if (expression != null) {
+                builder.setExpression(Event.toExpressionProto(expression));
+            }
+            return builder.build();
         }
 
         @Nonnull
@@ -958,6 +992,32 @@ public PEvent.Builder toEventBuilder() {
             return PEvent.newBuilder()
                     .setInsertIntoMemoEvent(toProto());
         }
+
+        @Nonnull
+        public static InsertIntoMemoEvent begin() {
+            return new InsertIntoMemoEvent(Location.BEGIN, null, ImmutableList.of());
+        }
+
+        @Nonnull
+        public static InsertIntoMemoEvent end() {
+            return new InsertIntoMemoEvent(Location.END, null, ImmutableList.of());
+        }
+
+        @Nonnull
+        public static InsertIntoMemoEvent newExp(@Nonnull final RelationalExpression expression) {
+            return new InsertIntoMemoEvent(Location.NEW, expression, ImmutableList.of());
+        }
+
+        @Nonnull
+        public static InsertIntoMemoEvent reusedExp(@Nonnull final RelationalExpression expression) {
+            return new InsertIntoMemoEvent(Location.REUSED, expression, ImmutableList.of());
+        }
+
+        @Nonnull
+        public static InsertIntoMemoEvent reusedExpWithReferences(@Nonnull final RelationalExpression expression,
+                                                                  @Nonnull final List<Reference> references) {
+            return new InsertIntoMemoEvent(Location.REUSED, expression, references);
+        }
     }
 
     /**