[CALCITE-7675] GROUP BY ALL and ORDER BY ALL should infer keys only from expressions that depend on the input

core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java

@@ -5379,13 +5379,19 @@ protected void rewriteOrderByAll(SqlSelect select) {
       final SqlNode expr = SqlUtil.stripAs(selectItem);
       if (expr instanceof SqlIdentifier && ((SqlIdentifier) expr).isStar()) {
         for (SqlNode column : expandStarForAllRewrite(select, expr)) {
-          keys.add(applyOrderByAllDirection(column, desc, nulls, pos));
+          if (dependsOnInput(column)) {
+            keys.add(applyOrderByAllDirection(column, desc, nulls, pos));
+          }
         }
         continue;
       }
-      keys.add(applyOrderByAllDirection(expr, desc, nulls, pos));
+      if (dependsOnInput(expr)) {
+        keys.add(applyOrderByAllDirection(expr, desc, nulls, pos));
+      }
     }
-    select.setOrderBy(new SqlNodeList(keys, pos));
+    // An empty but non-null ORDER BY list would make the select unparse with
+    // redundant parentheses, so remove the clause entirely.
+    select.setOrderBy(keys.isEmpty() ? null : new SqlNodeList(keys, pos));
   }
 
   /** Wraps a single ORDER BY ALL key with the optional descending direction
@@ -5565,8 +5571,71 @@ protected void validateGroupClause(SqlSelect select) {
     }
   }
 
+  /** Returns whether an expression depends on the input row, and is therefore
+   * worth using as an implicit {@code GROUP BY ALL} grouping key or
+   * {@code ORDER BY ALL} sort key.
+   *
+   * <p>An expression that references no column, and all of whose operators are
+   * deterministic, has the same value in every row; grouping by it does not
+   * divide the input into more than one group, and sorting by it does not
+   * reorder rows. Such expressions -- for example {@code 42}, {@code 1 + 1},
+   * {@code upper('x')}, {@code current_date} and {@code ?} -- are therefore
+   * not made keys. This follows BigQuery, which infers keys only from
+   * expressions that reference a name in the FROM clause.
+   *
+   * <p>A call to a non-deterministic operator such as {@code RAND()} does vary
+   * from row to row, and is retained. This is the same rule that
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-7697">[CALCITE-7697]</a>
+   * applies to window keys, one phase later. An unresolved function is
+   * retained, because its determinism is not yet known.
+   *
+   * <p>A sub-query is always retained, and is not inspected. Its identifiers
+   * include table names, which are not expressions in this sense, and
+   * resolving them here would be premature. Retaining is conservative, and is
+   * required for a correlated sub-query.
+   *
+   * <p>The rule inherits the limits of
+   * {@link SqlOperator#isDeterministic()}: a user-defined function is
+   * deterministic unless it says otherwise, so a call to one with no column
+   * argument is excluded. */
+  private boolean dependsOnInput(SqlNode node) {
+    try {
+      final SqlVisitor<Void> visitor =
+          new SqlBasicVisitor<Void>() {
+            @Override public Void visit(SqlIdentifier identifier) {
+              // The parser leaves a niladic function such as CURRENT_DATE as an
+              // identifier, and it becomes a call only later, during expansion;
+              // such an identifier does not reference the input.
+              if (!identifier.isStar() && makeNullaryCall(identifier) != null) {
+                return null;
+              }
+              throw new Util.FoundOne(identifier);
+            }
+
+            @Override public Void visit(SqlCall call) {
+              if (call.getKind().belongsTo(SqlKind.QUERY)) {
+                throw new Util.FoundOne(call);
+              }
+              final SqlOperator operator = call.getOperator();
+              if (!operator.isDeterministic()
+                  || operator instanceof SqlUnresolvedFunction) {
+                throw new Util.FoundOne(call);
+              }
+              return super.visit(call);
+            }
+          };
+      node.accept(visitor);
+      return false;
+    } catch (Util.FoundOne e) {
+      Util.swallow(e, null);
+      return true;
+    }
+  }
+
   /** If GROUP BY clause is the {@code GROUP BY ALL} placeholder, replaces it
-   * with every non-aggregated expression from the SELECT clause. */
+   * with every non-aggregated expression from the SELECT clause that depends on
+   * the input. If no such expression remains, the query becomes a single-group
+   * aggregation, {@code GROUP BY ()}. */
   private void rewriteGroupByAll(SqlSelect select) {
     final SqlNodeList groupList = select.getGroup();
     if (groupList == null
@@ -5582,13 +5651,14 @@ private void rewriteGroupByAll(SqlSelect select) {
       final SqlNode expr = SqlUtil.stripAs(selectItem);
       if (expr instanceof SqlIdentifier && ((SqlIdentifier) expr).isStar()) {
         for (SqlNode column : expandStarForAllRewrite(select, expr)) {
-          if (aggOrOverFinder.findAgg(column) == null) {
+          if (aggOrOverFinder.findAgg(column) == null
+              && dependsOnInput(column)) {
             keys.add(column);
           }
         }
         continue;
       }
-      if (aggOrOverFinder.findAgg(expr) == null) {
+      if (aggOrOverFinder.findAgg(expr) == null && dependsOnInput(expr)) {
         keys.add(expr);
       }
     }

core/src/test/java/org/apache/calcite/test/JdbcTest.java

@@ -1297,15 +1297,23 @@ private void checkResultSetMetaData(Connection connection, String sql)
             + "c0=1998\n");
   }
 
-  /** Test case for [CALCITE-7594] GROUP BY ALL: grouping only by a constant
-   * over empty input returns 0 rows. */
+  /** Test case for [CALCITE-7675] GROUP BY ALL over a select list whose only
+   * non-aggregate item is a constant.
+   *
+   * <p>The constant is not a grouping key, so no grouping key remains and the
+   * query is a single-group aggregation. It therefore returns one row even
+   * though the input is empty. Before [CALCITE-7675] the constant was a
+   * grouping key, empty input yielded no groups, and the query returned no
+   * rows. BigQuery documents the behavior asserted here: "If the set of
+   * inferred grouping keys is empty after exclusions are applied, all input
+   * rows are considered a single group for aggregation." */
   @Test void testGroupByAllOverEmptyInput() {
     CalciteAssert.hr()
         .query("select 'x', count(*)\n"
                 + "from \"hr\".\"emps\"\n"
                 + "where false\n"
                 + "group by all")
-        .returnsCount(0);
+        .returns("EXPR$0=x; EXPR$1=0\n");
   }
 
   /** Test case for

core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java

@@ -7390,6 +7390,55 @@ public boolean isBangEqualAllowed() {
         .ok();
   }
 
+  /** Tests that {@code ORDER BY ALL} makes a sort key only of a SELECT item
+   * that depends on the input row. See
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-7675">[CALCITE-7675]</a>. */
+  @Test void testOrderByAllExcludesExpressionsNotDependingOnInput() {
+    // A constant cannot reorder rows, so it is not a sort key. Before
+    // [CALCITE-7675] the synthesized "ORDER BY 42" was read as a sort ordinal
+    // and failed with "Ordinal out of range", in the default conformance.
+    sql("select sal, 42 from emp order by all")
+        .rewritesTo("SELECT `SAL`, 42\n"
+            + "FROM `EMP`\n"
+            + "ORDER BY `SAL`");
+
+    // The dangerous case: an in-range value silently resolved to a different
+    // SELECT item, so this query sorted on DEPTNO twice rather than on the
+    // constant and then DEPTNO.
+    sql("select 2, deptno from emp order by all")
+        .rewritesTo("SELECT 2, `DEPTNO`\n"
+            + "FROM `EMP`\n"
+            + "ORDER BY `DEPTNO`");
+
+    // Constant expressions and deterministic niladic functions are excluded
+    // too; a non-deterministic call is retained.
+    sql("select sal, 1 + 1, current_date from emp order by all")
+        .rewritesTo("SELECT `SAL`, 1 + 1, CURRENT_DATE\n"
+            + "FROM `EMP`\n"
+            + "ORDER BY `SAL`");
+    sql("select sal, rand() from emp order by all")
+        .rewritesTo("SELECT `SAL`, RAND()\n"
+            + "FROM `EMP`\n"
+            + "ORDER BY `SAL`, RAND()");
+
+    // If no sort key remains, no ORDER BY is emitted at all.
+    sql("select 42 from emp order by all")
+        .rewritesTo("SELECT 42\n"
+            + "FROM `EMP`");
+
+    // The trailing direction still applies to every remaining key.
+    sql("select sal, deptno, 42 from emp order by all desc")
+        .rewritesTo("SELECT `SAL`, `DEPTNO`, 42\n"
+            + "FROM `EMP`\n"
+            + "ORDER BY `SAL` DESC, `DEPTNO` DESC");
+
+    // A sort ordinal the user writes is still resolved, and still rejected
+    // when out of range.
+    sql("select sal, deptno from emp order by 2").ok();
+    sql("select sal, deptno from emp order by ^42^")
+        .fails("Ordinal out of range");
+  }
+
   @Test void testOrder() {
     final SqlConformance conformance = fixture().conformance();
     sql("select empno as x from emp order by empno").ok();
@@ -7860,6 +7909,125 @@ public boolean isBangEqualAllowed() {
         .ok();
   }
 
+  /** Tests that {@code GROUP BY ALL} makes a grouping key only of a SELECT item
+   * that depends on the input row. See
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-7675">[CALCITE-7675]</a>. */
+  @Test void testGroupByAllExcludesExpressionsNotDependingOnInput() {
+    // A constant is not a grouping key.
+    sql("select deptno, 42, count(*) from emp group by all")
+        .rewritesTo("SELECT `DEPTNO`, 42, COUNT(*)\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY `EMP`.`DEPTNO`");
+
+    // Nor is a constant expression, nor a deterministic niladic function.
+    sql("select deptno, 1 + 1, upper('x'), current_date, count(*)\n"
+        + "from emp group by all")
+        .rewritesTo("SELECT `DEPTNO`, 1 + 1, UPPER('x'), CURRENT_DATE, COUNT(*)\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY `EMP`.`DEPTNO`");
+
+    // If no grouping key remains the query is a single-group aggregation.
+    sql("select 42, count(*) from emp group by all")
+        .rewritesTo("SELECT 42, COUNT(*)\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY ()");
+
+    // An expression over a column is a grouping key, as always.
+    sql("select deptno + 1, count(*) from emp group by all")
+        .rewritesTo("SELECT `DEPTNO` + 1, COUNT(*)\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY `EMP`.`DEPTNO` + 1");
+    sql("select case when sal > 100 then 'high' else 'low' end, count(*)\n"
+        + "from emp group by all")
+        .rewritesTo("SELECT CASE WHEN `SAL` > 100 THEN 'high' ELSE 'low' END, COUNT(*)\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY CASE WHEN `EMP`.`SAL` > 100 THEN 'high' ELSE 'low' END");
+
+    // A non-deterministic call varies from row to row, so it is a grouping key.
+    sql("select rand(), count(*) from emp group by all")
+        .rewritesTo("SELECT RAND(), COUNT(*)\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY RAND()");
+
+    // Excluding the constant also means the expansion never emits a bare
+    // integer, which a conformance that groups by ordinal would otherwise read
+    // as a select-list position. Before [CALCITE-7675] this failed with
+    // "Ordinal out of range".
+    sql("select deptno, 42, count(*) from emp group by all")
+        .withConformance(SqlConformanceEnum.LENIENT)
+        .ok();
+
+    // An ordinal the user writes is still resolved, and still rejected when out
+    // of range.
+    sql("select deptno, count(*) from emp group by 1")
+        .withConformance(SqlConformanceEnum.LENIENT)
+        .ok();
+    sql("select deptno, count(*) from emp group by ^42^")
+        .withConformance(SqlConformanceEnum.LENIENT)
+        .fails("Ordinal out of range");
+
+    // A sub-query is retained without being inspected. Its identifiers include
+    // table names and aliases, which are not expressions; treating one as a
+    // possible niladic function would wrongly reject a query whose alias
+    // happens to name one, under a conformance that requires parentheses.
+    sql("select (select 1 from (values (1)) as pi), count(*) from emp group by all")
+        .withConformance(SqlConformanceEnum.BIG_QUERY)
+        .ok();
+    sql("select (select 1 from (values (1)) as pi), count(*) from emp group by all")
+        .rewritesTo("SELECT (SELECT 1\n"
+            + "FROM (VALUES ROW(1)) AS PI), COUNT(*)\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY (SELECT 1\n"
+            + "FROM (VALUES ROW(1)) AS PI)");
+  }
+
+  /** Tests that the expansion of {@code GROUP BY ALL} can be re-parsed with
+   * the same meaning under a conformance that reads an integer in GROUP BY as
+   * a select-list ordinal.
+   *
+   * <p>The expanded form is what is stored as a view or materialized-table
+   * definition, so it is read back by a validator that cannot know which keys
+   * the user wrote and which the expansion synthesized. Before
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-7675">[CALCITE-7675]</a>
+   * a constant SELECT item became a grouping key and the definition contained
+   * a bare integer literal, which the re-parse resolved as an ordinal: out of
+   * range it failed to validate, and in range it silently designated a
+   * different SELECT item. */
+  @Test void testGroupByAllExpansionSurvivesReparseAsOrdinal() {
+    // Sergey Nuyanzin's query from the FLIP-606 discussion. This expanded to
+    // "GROUP BY 42", which is not a valid definition: 42 exceeds the size of
+    // the select list.
+    sql("select count(*) as cnt, sum(sal) as sm, 42 as just_a_constant\n"
+        + "from emp group by all")
+        .rewritesTo("SELECT COUNT(*) AS `CNT`, SUM(`SAL`) AS `SM`, 42 AS `JUST_A_CONSTANT`\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY ()");
+
+    // The expansion above, fed back in where GROUP BY ordinals are enabled.
+    sql("select count(*) as cnt, sum(sal) as sm, 42 as just_a_constant\n"
+        + "from emp group by ()")
+        .withConformance(SqlConformanceEnum.LENIENT)
+        .ok();
+
+    // The quieter case: an in-range value. This expanded to
+    // "GROUP BY `EMP`.`DEPTNO`, 2", whose re-parse read 2 as the second
+    // SELECT item -- an aggregate, and so not a legal grouping key.
+    sql("select deptno, count(*) as c, 2 as k from emp group by all")
+        .rewritesTo("SELECT `DEPTNO`, COUNT(*) AS `C`, 2 AS `K`\n"
+            + "FROM `EMP`\n"
+            + "GROUP BY `EMP`.`DEPTNO`");
+
+    sql("select deptno, count(*) as c, 2 as k from emp\n"
+        + "group by emp.deptno")
+        .withConformance(SqlConformanceEnum.LENIENT)
+        .ok();
+
+    // A grouping key the user writes as an ordinal is still resolved as one.
+    sql("select deptno, count(*) from emp group by 1")
+        .withConformance(SqlConformanceEnum.LENIENT)
+        .ok();
+  }
+
   /** Test case for
    * <a href="https://issues.apache.org/jira/browse/CALCITE-5507">[CALCITE-5507]
    * HAVING alias failed when aggregate function in condition</a>. */

core/src/test/resources/sql/agg.iq

@@ -4749,4 +4749,43 @@ order by y;
 
 !ok
 
+# [CALCITE-7675] GROUP BY ALL should infer grouping keys only from expressions
+# that depend on the input. The constant 42 is the same in every row, so it is
+# not a grouping key and the rows are grouped by X alone. Verified against
+# BigQuery, which infers keys only from expressions that reference a name in
+# the FROM clause.
+select x, 42 as c, count(*) as n
+from (values (1, 'a'), (1, 'a'), (2, 'b')) as t(x, y)
+group by all
+order by x;
++---+----+---+
+| X | C  | N |
++---+----+---+
+| 1 | 42 | 2 |
+| 2 | 42 | 1 |
++---+----+---+
+(2 rows)
+
+!ok
+
+# [CALCITE-7675] GROUP BY ALL should infer grouping keys only from expressions
+# that depend on the input. When none does, no grouping key remains and the
+# query is a single-group aggregation, GROUP BY (). It
+# therefore returns one row even though the input is empty. BigQuery
+# specifies the same: "if the set of inferred grouping keys is empty after
+# exclusions are applied, all input rows are considered a single group for
+# aggregation".
+select 42 as c, count(*) as n
+from (values (1, 'a')) as t(x, y)
+where x < 0
+group by all;
++----+---+
+| C  | N |
++----+---+
+| 42 | 0 |
++----+---+
+(1 row)
+
+!ok
+
 # End agg.iq

core/src/test/resources/sql/sort.iq

@@ -583,4 +583,22 @@ order by all;
 
 !ok
 
+# [CALCITE-7675] GROUP BY ALL should infer grouping keys only from expressions
+# that depend on the input; the same holds for the sort keys of ORDER BY ALL.
+# The constant 2 cannot reorder rows and is not a sort key, so the rows are
+# ordered by Y alone. Before [CALCITE-7675] the synthesized "ORDER BY 2" was
+# read as a sort ordinal and silently ordered by the second SELECT item.
+select 2 as c, y from (values (2, 'b'), (1, 'a'), (1, 'c')) as t(x, y)
+order by all;
++---+---+
+| C | Y |
++---+---+
+| 2 | a |
+| 2 | b |
+| 2 | c |
++---+---+
+(3 rows)
+
+!ok
+
 # End sort.iq