Fix an aggregate over a whole-record argument planning incorrectly

fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/Expression.java

@@ -232,16 +232,18 @@
         return false;
     }
 
     /**
-     * Returns this expression rewritten in terms of the given value, which is simplified on the way. See
+     * Returns this expression rewritten in terms of the given value, simplifying both sides on the way. See
      * {@link #pullUpSimplified} for details.
      */
     @Nonnull
     public Expression pullUp(@Nonnull Value value, @Nonnull CorrelationIdentifier correlationIdentifier,
                              @Nonnull Set<CorrelationIdentifier> constantAliases) {
         final AliasMap aliasMap = AliasMap.identitiesFor(value.getCorrelatedTo());
         final Value simplifiedValue = value.simplify(EvaluationContext.empty(), aliasMap, constantAliases);
-        return withUnderlying(pullUpSimplified(getUnderlying(), simplifiedValue, aliasMap, correlationIdentifier,
+        final Value simplifiedUnderlying =
+                getUnderlying().simplify(EvaluationContext.empty(), aliasMap, constantAliases);
+        return withUnderlying(pullUpSimplified(simplifiedUnderlying, simplifiedValue, aliasMap, correlationIdentifier,
                 constantAliases));
     }
 
@@ -257,14 +259,15 @@
      * by a reference to the column that holds it, while the {@code + 1} is left alone because it has no counterpart on
      * the other side.
      *
-     * <p>The candidate has to arrive simplified, under the same {@code aliasMap} and {@code constantAliases} that are
-     * passed here. Simplification paves the way for the matching by performing certain canonicalization steps (such as
-     * collapsing a record constructor that effectively reconstructs a whole record to just that record), and the
-     * matching is structural, so a value that is canonicalized on one side but not on the other does not match at all.
+     * <p>Both given values should be in their simplified form, under the same {@code aliasMap} and
+     * {@code constantAliases} that are passed here. Simplification paves the way for the matching by performing certain
+     * canonicalization steps (such as collapsing a record constructor that effectively reconstructs a whole record to
+     * just that record). This matters because the matching is structural, so a value that is canonicalized on one side
+     * but not on the other would not necessarily match.
      *
      * <p>Neither value is modified; the result is a new value, or the given one in case nothing matched.
      *
-     * @param value the value to rewrite
+     * @param value the value to rewrite, simplified
      * @param simplifiedValue the candidate to express {@code value} in terms of, simplified
      * @param aliasMap the alias map of equalities to match under
      * @param correlationIdentifier the alias the resulting references are expressed over

fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/Expressions.java

@@ -103,9 +103,11 @@
         final AliasMap aliasMap = AliasMap.identitiesFor(value.getCorrelatedTo());
         final Value simplifiedValue = value.simplify(EvaluationContext.empty(), aliasMap, constantAliases);
         return Expressions.of(stream()
                 .map(expression -> expression.withUnderlying(
-                        Expression.pullUpSimplified(expression.getUnderlying(), simplifiedValue, aliasMap,
-                                correlationIdentifier, constantAliases)))
+                        Expression.pullUpSimplified(
+                                expression.getUnderlying().simplify(EvaluationContext.empty(), aliasMap,
+                                        constantAliases),
+                                simplifiedValue, aliasMap, correlationIdentifier, constantAliases)))
                 .collect(ImmutableList.toImmutableList()));
     }
 

fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/OrderByExpression.java

@@ -85,8 +85,10 @@
                                 ((Star) orderBy.getExpression()).getExpansion().stream().map(orderBy::withExpression) :
                                 Stream.of(orderBy))
                 .map(orderBy -> {
                     final var orderByExpression = orderBy.getExpression();
-                    final var underlying = orderByExpression.getUnderlying();
+                    // Simplify this side as well, so that it meets the candidate in the same, canonical shape.
+                    final var underlying = orderByExpression.getUnderlying()
+                            .simplify(EvaluationContext.empty(), aliasMap, constantAliases);
                     final var pulledUpUnderlying = Assert.notNullUnchecked(underlying.replace(
                             subExpression -> {
                                 final var pulledUpExpressionMap =

yaml-tests/src/test/resources/groupby-tests.yamsql

@@ -260,4 +260,45 @@ test_block:
       - result: [{!l 10, !l 5}]
       - result: []
       - result: [{!l 20, !l 8}]
+---
+# Aggregates over a record constructor. These are regression tests for Issue #4481. They exercise the interaction of
+# `collapseSimpleSelectMaybe()`, which collapses some of these record constructors, with the group-by pull-up logic.
+test_block:
+  name: aggregate-over-record
+  tests:
+    -
+      # A star-derived record constructor.
+      - query: SELECT COUNT((T1.*)) FROM T1
+      - supported_version: !current_version
+      - result: [{!l 13}]
+    -
+      # Record constructor with every column enumerated in declaration order; gets collapsed like the star-derived form.
+      - query: SELECT COUNT((T1.id, T1.col1, T1.col2)) FROM T1
+      - supported_version: !current_version
+      - result: [{!l 13}]
+    -
+      # The same columns, but out of order. Such a record constructor doesn’t get collapsed.
+      - query: SELECT COUNT((T1.col2, T1.col1, T1.id)) FROM T1
+      - result: [{!l 13}]
+    -
+      # A strict subset of the columns. Doesn’t get collapsed either.
+      - query: SELECT COUNT((T1.id, T1.col2)) FROM T1
+      - result: [{!l 13}]
+    -
+      # The star-derived form again, but with grouping. Here the pull-up has grouping columns to rewrite alongside the
+      # aggregate.
+      - query: SELECT col1, COUNT((T1.*)) FROM T1 GROUP BY col1
+      - supported_version: !current_version
+      - result: [{!l 10, !l 5}, {!l 20, !l 8}]
+    -
+      # The HAVING predicate is normalized as well, not only the projection.
+      - query: SELECT col1 FROM T1 GROUP BY col1 HAVING COUNT((T1.*)) > 5
+      - supported_version: !current_version
+      - result: [{!l 20}]
+    -
+      # An ORDER BY over an aggregate is not supported, whatever the aggregate's argument looks like. This case pins
+      # that the whole-record spelling is rejected as such, rather than failing at evaluation time like it used to.
+      - query: SELECT col1 FROM T1 GROUP BY col1 ORDER BY COUNT((T1.*))
+      - supported_version: !current_version
+      - error: UNSUPPORTED_QUERY
 ...