responding to more comments

Author: normen662

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

@@ -753,7 +753,7 @@ default Compensation intersect(@Nonnull Compensation otherCompensation) {
                     //    matter which side we take as both create the same compensating filter. If at any point in the
                     //    future one data access has a better reapplication we need to generate plan variants with
                     //    either compensation and let the planner figure out which one wins. We just pick one side here.
-                    // 2. Either one or both compensation functions are impossible, but thee intersection is possible.
+                    // 2. Either one or both compensation functions are impossible, but the intersection is possible.
                     //    We take the compensation function from this side and amend it with the compensation function
                     //    from the other side.
                     final var newPredicateCompensationFunction = entry.getValue().amend(unmatchedAggregateMap, newMatchedAggregatesMap);

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

@@ -25,13 +25,32 @@
 import com.google.common.collect.ImmutableBiMap;
 
 import javax.annotation.Nonnull;
+import java.util.Set;
+import java.util.function.Function;
 
+/**
+ * Class to keep track of matched groupings, matched aggregates, as well as unmatched aggregates (so far).
+ */
 public class GroupByMappings {
+    /**
+     * A {@link BiMap} from original query grouping {@link Value} to translated query grouping {@link Value} which is
+     * specific to a {@link MatchCandidate}.
+     */
     @Nonnull
     private final BiMap<Value, Value> matchedGroupingsMap;
+
+    /**
+     * A {@link BiMap} from original query aggregate {@link Value} to translated query aggregate {@link Value} which is
+     * specific to a {@link MatchCandidate}.
+     */
     @Nonnull
     private final BiMap<Value, Value> matchedAggregatesMap;
 
+    /**
+     * A {@link BiMap} from a unique id to an original query aggregate that is not yet matched. The unique id used here
+     * is the same id that is used to handle unmatched aggregate functions in
+     * {@link com.apple.foundationdb.record.query.plan.cascades.values.translation.MaxMatchMap#compute(Value, Value, Set, ValueEquivalence, Function)}.
+     */
     @Nonnull
     private final BiMap<CorrelationIdentifier, Value> unmatchedAggregatesMap;
 

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

@@ -156,6 +156,14 @@ public Set<QueryPredicate> applyCompensationForPredicate(@Nonnull final Translat
 
         boolean isImpossible();
 
+        /**
+         * Recreates this predicate compensation function, and if appropriate, allows this compensation function
+         * to become possible.
+         * @param unmatchedAggregateMap unmatched aggregated map; the resulting predicate compensation function
+         *        can only become possible if the unmatched aggregates are not referenced by the predicate
+         * @param amendedMatchedAggregateMap matched aggregate map (amended)
+         * @return a new {@link PredicateCompensationFunction}
+         */
         @Nonnull
         PredicateCompensationFunction amend(@Nonnull BiMap<CorrelationIdentifier, Value> unmatchedAggregateMap,
                                             @Nonnull Map<Value, Value> amendedMatchedAggregateMap);
@@ -365,6 +373,14 @@ public Value applyCompensationForResult(@Nonnull final TranslationMap translatio
 
         boolean isImpossible();
 
+        /**
+         * Recreates this result compensation function, and if appropriate, allows this compensation function
+         * to become possible.
+         * @param unmatchedAggregateMap unmatched aggregated map; the resulting predicate compensation function
+         *        can only become possible if the unmatched aggregates are not referenced by the predicate
+         * @param amendedMatchedAggregateMap matched aggregate map (amended)
+         * @return a new {@link ResultCompensationFunction}
+         */
         @Nonnull
         ResultCompensationFunction amend(@Nonnull BiMap<CorrelationIdentifier, Value> unmatchedAggregateMap,
                                          @Nonnull Map<Value, Value> amendedMatchedAggregateMap);

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

@@ -578,12 +578,11 @@ private SubsumedGroupingsResult groupingSubsumedBy(@Nonnull final Quantifier can
             // by the prefix of the candidate side. Iterate up to the smaller query side's grouping values to
             // find out.
             //
-            for (int i = 0; i < translatedGroupingValues.size(); i++) {
-                if (unmatchedCandidateValues.contains(candidateGroupingValues.get(i))) {
+            for (final var translatedGroupingValue : translatedGroupingValuesSet) {
+                if (unmatchedCandidateValues.contains(translatedGroupingValue)) {
                     return SubsumedGroupingsResult.noSubsumption();
                 }
             }
-
             return SubsumedGroupingsResult.of(booleanWithConstraint, matchedGroupingsMap, translatedGroupingValues);
         }
 

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

@@ -106,19 +106,18 @@
  *
  * The logic that this rules delegates to and actually creates the expressions can be found in
  * {@link MatchCandidate#toEquivalentPlan(PartialMatch, PlanContext, Memoizer, boolean)}.
- * @param <R> subtype of {@link RelationalExpression}
  */
 @API(API.Status.EXPERIMENTAL)
 @SuppressWarnings({"java:S3776", "java:S4738", "OptionalUsedAsFieldOrParameterType"})
-public abstract class AbstractDataAccessRule<R extends RelationalExpression> extends CascadesRule<MatchPartition> {
+public abstract class AbstractDataAccessRule extends CascadesRule<MatchPartition> {
     private static final Logger logger = LoggerFactory.getLogger(AbstractDataAccessRule.class);
 
     private final BindingMatcher<PartialMatch> completeMatchMatcher;
-    private final BindingMatcher<R> expressionMatcher;
+    private final BindingMatcher<? extends RelationalExpression> expressionMatcher;
 
     protected AbstractDataAccessRule(@Nonnull final BindingMatcher<MatchPartition> rootMatcher,
                                      @Nonnull final BindingMatcher<PartialMatch> completeMatchMatcher,
-                                     @Nonnull final BindingMatcher<R> expressionMatcher) {
+                                     @Nonnull final BindingMatcher<? extends RelationalExpression> expressionMatcher) {
         super(rootMatcher, ImmutableSet.of(ReferencedFieldsConstraint.REFERENCED_FIELDS, RequestedOrderingConstraint.REQUESTED_ORDERING));
         this.completeMatchMatcher = completeMatchMatcher;
         this.expressionMatcher = expressionMatcher;
@@ -130,7 +129,7 @@ protected BindingMatcher<PartialMatch> getCompleteMatchMatcher() {
     }
 
     @Nonnull
-    protected BindingMatcher<R> getExpressionMatcher() {
+    protected BindingMatcher<? extends RelationalExpression> getExpressionMatcher() {
         return expressionMatcher;
     }
 
@@ -213,9 +212,8 @@ public void onMatch(@Nonnull final CascadesRuleCall call) {
                 final var matchPartition = matchPartitionEntry.getValue();
 
                 //
-                // The current match partition covers all matches that match the aliases in matchedAliases
-                // as well as all predicates in matchedPredicates. In other words we now have to compensate
-                // for all the remaining quantifiers and all remaining predicates.
+                // The current match partition covers all matches that match a certain set of aliases and predicates.
+                // We now have to compensate for all the those quantifiers and predicates.
                 //
                 final var dataAccessExpressions =
                         dataAccessForMatchPartition(call,
@@ -912,6 +910,8 @@ private static Optional<RelationalExpression> applyCompensationForSingleDataAcce
      * before using the resulting {@link Compensation} to compute the compensating expression for the entire
      * intersection.
      * @param memoizer the memoizer
+     * @param intersectionInfoMap the intersection info map being maintained by the caller (that enumerates the possible
+     *        intersections)
      * @param matchToPlanMap a map from match to single data access expression
      * @param partition a partition (i.e. a list of {@link SingleMatchedAccess}es that the caller would like to compute
      *        and intersected data access for
@@ -976,6 +976,7 @@ protected static boolean isPartitionRedundant(@Nonnull final Map<BitSet, Interse
     @Nonnull
     private static Optional<Set<CorrelationIdentifier>> unmatchedIdsMaybe(@Nonnull final Map<BitSet, IntersectionInfo> intersectionInfoMap,
                                                                           @Nonnull final List<Vectored<SingleMatchedAccess>> partition) {
+        Verify.verify(!partition.isEmpty());
         final var iterator = partition.iterator();
 
         var unmatchedIdsOptional =

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

@@ -50,7 +50,7 @@
  *     <li>referencing the {@link MatchCandidate}'s traversal w.r.t. the (partially) matched query.</li>
  *     <li>does not have a corresponding match on the query side.</li>
  * </ul>
- * For more information, see {@link RelationalExpression#adjustMatch(PartialMatch, Quantifier)}.
+ * For more information, see {@link RelationalExpression#adjustMatch(PartialMatch)}.
  * Currently the only such expression that can be absorbed is
  * {@link com.apple.foundationdb.record.query.plan.cascades.expressions.MatchableSortExpression}.
  * TODO Maybe that expression should just be a generic property-defining expression or properties should be kept on quantifiers.

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

@@ -71,7 +71,7 @@
  */
 @API(API.Status.EXPERIMENTAL)
 @SuppressWarnings("PMD.TooManyStaticImports")
-public class AggregateDataAccessRule extends AbstractDataAccessRule<RelationalExpression> {
+public class AggregateDataAccessRule extends AbstractDataAccessRule {
     private static final BindingMatcher<PartialMatch> completeMatchMatcher =
             completeMatch().and(matchingAggregateIndexMatchCandidate());
     private static final BindingMatcher<RelationalExpression> expressionMatcher = anyExpression();

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

@@ -71,7 +71,7 @@
  */
 @API(API.Status.EXPERIMENTAL)
 @SuppressWarnings("PMD.TooManyStaticImports")
-public class WithPrimaryKeyDataAccessRule extends AbstractDataAccessRule<RelationalExpression> {
+public class WithPrimaryKeyDataAccessRule extends AbstractDataAccessRule {
     private static final BindingMatcher<PartialMatch> completeMatchMatcher =
             completeMatch().and(matchingWithPrimaryKeyMatchCandidate());
     private static final BindingMatcher<RelationalExpression> expressionMatcher = anyExpression();

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

@@ -57,7 +57,6 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.function.Function;
-import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 /**