[CALCITE-5883] ROWS window aggregates ignore frames when there is no …

…ORDER BY clause

Before this change, `SUM(x) OVER (ROWS 1 PRECEDING)` was
treated the same as `SUM(x) OVER (ROWS BETWEEN UNBOUNDED
PRECEDING AND UNBOUNDED FOLLOWING)`. This was wrong -
removing frames is correct for RANGE but not for ROWS.

This change also fixes a special case,
[CALCITE-6538] OVER (ROWS CURRENT ROW) should return a
window with one row, not all rows

In class RexWindowBound, add `isUnboundedPreceding()` and
`isUnboundedFollowing()` convenience methods.

Fix `RexWindow digest`, so that `OVER ()` is equivalent to
`OVER (RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)`
but not equivalent to `OVER (ROWS CURRENT ROW)`.

Add Quidem tests for OVER, checked against Postgres.

A few cosmetic changes, such as removing `assert x != null`
when x is a parameter not declared `@Nullable`, adding static
imports for `Objects.requireNonNull`, and
changing `size() = 0` to `isEmpty()`.

Close #3925

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java

@@ -401,7 +401,7 @@ private static void sampleOfTheGeneratedWindowedAggregate() {
       }
 
       Expression lowerBoundCanChange =
-          group.lowerBound.isUnbounded() && group.lowerBound.isPreceding()
+          group.lowerBound.isUnboundedPreceding()
           ? Expressions.constant(false)
           : Expressions.notEqual(startX, prevStart);
 

core/src/main/java/org/apache/calcite/rel/core/Window.java

@@ -55,7 +55,9 @@
 import java.util.AbstractList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
+
+import static java.util.Objects.hash;
+import static java.util.Objects.requireNonNull;
 
 /**
  * A relational expression representing a set of window aggregates.
@@ -91,8 +93,7 @@ protected Window(RelOptCluster cluster, RelTraitSet traitSet, List<RelHint> hint
       RelNode input, List<RexLiteral> constants, RelDataType rowType, List<Group> groups) {
     super(cluster, traitSet, input);
     this.constants = ImmutableList.copyOf(constants);
-    assert rowType != null;
-    this.rowType = rowType;
+    this.rowType = requireNonNull(rowType, "rowType");
     this.groups = ImmutableList.copyOf(groups);
     this.hints = ImmutableList.copyOf(hints);
   }
@@ -267,12 +268,12 @@ public Group(
         RexWindowExclusion exclude,
         RelCollation orderKeys,
         List<RexWinAggCall> aggCalls) {
-      this.keys = Objects.requireNonNull(keys, "keys");
+      this.keys = requireNonNull(keys, "keys");
       this.isRows = isRows;
-      this.lowerBound = Objects.requireNonNull(lowerBound, "lowerBound");
-      this.upperBound = Objects.requireNonNull(upperBound, "upperBound");
+      this.lowerBound = requireNonNull(lowerBound, "lowerBound");
+      this.upperBound = requireNonNull(upperBound, "upperBound");
       this.exclude = exclude;
-      this.orderKeys = Objects.requireNonNull(orderKeys, "orderKeys");
+      this.orderKeys = requireNonNull(orderKeys, "orderKeys");
       this.aggCalls = ImmutableList.copyOf(aggCalls);
       this.digest = computeString();
     }
@@ -297,17 +298,14 @@ private String computeString(@UnderInitialization Group this) {
         buf.append(orderKeys);
       }
       if (orderKeys.getFieldCollations().isEmpty()
-          && lowerBound.isUnbounded()
-          && lowerBound.isPreceding()
-          && upperBound.isUnbounded()
-          && upperBound.isFollowing()) {
+          && lowerBound.isUnboundedPreceding()
+          && upperBound.isUnboundedFollowing()) {
         // skip bracket if no ORDER BY, and if bracket is the default,
         // "RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING",
         // which is equivalent to
         // "ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING"
       } else if (!orderKeys.getFieldCollations().isEmpty()
-          && lowerBound.isUnbounded()
-          && lowerBound.isPreceding()
+          && lowerBound.isUnboundedPreceding()
           && upperBound.isCurrentRow()
           && !isRows) {
         // skip bracket if there is ORDER BY, and if bracket is the default,
@@ -467,7 +465,7 @@ public RexWinAggCall(
 
     @Override public int hashCode() {
       if (hash == 0) {
-        hash = Objects.hash(super.hashCode(), ordinal, distinct, ignoreNulls);
+        hash = hash(super.hashCode(), ordinal, distinct, ignoreNulls);
       }
       return hash;
     }

core/src/main/java/org/apache/calcite/rex/RexBuilder.java

@@ -512,8 +512,12 @@ public RexWindow makeWindow(
       RexWindowBound upperBound,
       boolean rows,
       RexWindowExclusion exclude) {
-    if (lowerBound.isUnbounded() && lowerBound.isPreceding()
-        && upperBound.isUnbounded() && upperBound.isFollowing()) {
+    if (orderKeys.isEmpty() && !rows) {
+      lowerBound = RexWindowBounds.UNBOUNDED_PRECEDING;
+      upperBound = RexWindowBounds.UNBOUNDED_FOLLOWING;
+    }
+    if (lowerBound.isUnboundedPreceding()
+        && upperBound.isUnboundedFollowing()) {
       // RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
       //   is equivalent to
       // ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
@@ -936,7 +940,7 @@ public RexNode makeAbstractCast(RelDataType type, RexNode exp, boolean safe, Rex
    * Makes a reinterpret cast.
    *
    * @param type          type returned by the cast
-   * @param exp           expression to be casted
+   * @param exp           expression to be cast
    * @param checkOverflow whether an overflow check is required
    * @return a RexCall with two operands and a special return type
    */
@@ -945,7 +949,7 @@ public RexNode makeReinterpretCast(
       RexNode exp,
       RexNode checkOverflow) {
     List<RexNode> args;
-    if ((checkOverflow != null) && checkOverflow.isAlwaysTrue()) {
+    if (checkOverflow.isAlwaysTrue()) {
       args = ImmutableList.of(exp, checkOverflow);
     } else {
       args = ImmutableList.of(exp);
@@ -1268,8 +1272,7 @@ public RexLiteral makeLiteral(String s) {
    * @return Character string literal
    */
   protected RexLiteral makePreciseStringLiteral(String s) {
-    assert s != null;
-    if (s.equals("")) {
+    if (s.isEmpty()) {
       return charEmpty;
     }
     return makeCharLiteral(new NlsString(s, null, null));
@@ -1342,7 +1345,6 @@ public RelDataType matchNullability(
    * defaults.
    */
   public RexLiteral makeCharLiteral(NlsString str) {
-    assert str != null;
     RelDataType type = SqlUtil.createNlsStringType(typeFactory, str);
     return makeLiteral(str, type, SqlTypeName.CHAR);
   }

core/src/main/java/org/apache/calcite/rex/RexShuttle.java

@@ -75,16 +75,14 @@ public RexWindow visitWindow(RexWindow window) {
         visitList(window.partitionKeys, update);
     final RexWindowBound lowerBound = window.getLowerBound().accept(this);
     final RexWindowBound upperBound = window.getUpperBound().accept(this);
-    if (lowerBound == null
-        || upperBound == null
-        || !update[0]
+    if (!update[0]
         && lowerBound == window.getLowerBound()
         && upperBound == window.getUpperBound()) {
       return window;
     }
     boolean rows = window.isRows();
-    if (lowerBound.isUnbounded() && lowerBound.isPreceding()
-        && upperBound.isUnbounded() && upperBound.isFollowing()) {
+    if (lowerBound.isUnboundedPreceding()
+        && upperBound.isUnboundedFollowing()) {
       // RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
       //   is equivalent to
       // ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING

core/src/main/java/org/apache/calcite/rex/RexWindow.java

@@ -23,10 +23,11 @@
 import org.checkerframework.checker.nullness.qual.Nullable;
 
 import java.util.List;
-import java.util.Objects;
 
 import static com.google.common.base.Preconditions.checkArgument;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * Specification of the window of rows over which a {@link RexOver} windowed
  * aggregate is evaluated.
@@ -81,15 +82,16 @@ public class RexWindow {
       RexWindowExclusion exclude) {
     this.partitionKeys = ImmutableList.copyOf(partitionKeys);
     this.orderKeys = ImmutableList.copyOf(orderKeys);
-    this.lowerBound = Objects.requireNonNull(lowerBound, "lowerBound");
-    this.upperBound = Objects.requireNonNull(upperBound, "upperBound");
+    this.lowerBound = requireNonNull(lowerBound, "lowerBound");
+    this.upperBound = requireNonNull(upperBound, "upperBound");
     this.exclude = exclude;
     this.isRows = isRows;
     this.nodeCount = computeCodeCount();
     this.digest = computeDigest();
     checkArgument(
-        !(lowerBound.isUnbounded() && lowerBound.isPreceding()
-            && upperBound.isUnbounded() && upperBound.isFollowing() && isRows),
+        !(lowerBound.isUnboundedPreceding()
+            && upperBound.isUnboundedFollowing()
+            && isRows),
         "use RANGE for unbounded, not ROWS");
   }
 
@@ -157,16 +159,18 @@ private StringBuilder appendDigest_(StringBuilder sb, boolean allowFraming) {
     // There are 3 reasons to skip the ROWS/RANGE clause.
     // 1. If this window is being used with a RANK-style function that does not
     //    allow framing, or
-    // 2. If there is no ORDER BY (in which case a frame is invalid), or
-    // 3. If the ROWS/RANGE clause is the default, "RANGE BETWEEN UNBOUNDED
-    //    PRECEDING AND CURRENT ROW"
+    // 2. If it is RANGE without ORDER BY (in which case all frames yield all
+    //    rows),
+    // 3. If it is an unbounded range
+    //    ("RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING"
+    //    or "ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING")
+    //    with no ORDER BY.
     if (!allowFraming // 1
-        || orderKeys.isEmpty() // 2
-        || (lowerBound.isUnbounded() // 3
-            && lowerBound.isPreceding()
-            && upperBound.isCurrentRow()
-            && !isRows)) {
-      // No ROWS or RANGE clause
+        || (!isRows && orderKeys.isEmpty()) // 2
+        || (orderKeys.isEmpty()
+            && lowerBound.isUnboundedPreceding() // 3
+            && upperBound.isUnboundedFollowing())) {
+      // Don't print a ROWS or RANGE clause
     } else if (upperBound.isCurrentRow()) {
       // Per MSSQL: If ROWS/RANGE is specified and <window frame preceding>
       // is used for <window frame extent> (short syntax) then this
@@ -175,18 +179,25 @@ private StringBuilder appendDigest_(StringBuilder sb, boolean allowFraming) {
       // "ROWS 5 PRECEDING" is equal to "ROWS BETWEEN 5 PRECEDING AND CURRENT
       // ROW".
       //
-      // By similar reasoning to (3) above, we print the shorter option if it is
+      // We print the shorter option if it is
       // the default. If the RexWindow is, say, "ROWS BETWEEN 5 PRECEDING AND
       // CURRENT ROW", we output "ROWS 5 PRECEDING" because it is equivalent and
       // is shorter.
-      sb.append(sb.length() > initialLength
-          ? (isRows ? " ROWS " : " RANGE ")
-          : (isRows ? "ROWS " : "RANGE "))
-          .append(lowerBound);
+      if (!isRows && lowerBound.isUnboundedPreceding()) {
+        // OVER (ORDER BY x)
+        // OVER (ORDER BY x RANGE UNBOUNDED PRECEDING)
+        // OVER (ORDER BY x RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)
+        // are equivalent, so print the first (i.e. nothing).
+      } else {
+        sb.append(sb.length() > initialLength ? " " : "")
+            .append(isRows ? "ROWS" : "RANGE")
+            .append(' ')
+            .append(lowerBound);
+      }
     } else {
-      sb.append(sb.length() > initialLength
-          ? (isRows ? " ROWS BETWEEN " : " RANGE BETWEEN ")
-          : (isRows ? "ROWS BETWEEN " : "RANGE BETWEEN "))
+      sb.append(sb.length() > initialLength ? " " : "")
+          .append(isRows ? "ROWS" : "RANGE")
+          .append(" BETWEEN ")
           .append(lowerBound)
           .append(" AND ")
           .append(upperBound);

core/src/main/java/org/apache/calcite/rex/RexWindowBound.java

@@ -48,6 +48,16 @@ public boolean isUnbounded() {
     return false;
   }
 
+  /** Returns whether the bound is {@code UNBOUNDED PRECEDING}. */
+  public final boolean isUnboundedPreceding() {
+    return isUnbounded() && isPreceding();
+  }
+
+  /** Returns whether the bound is {@code UNBOUNDED FOLLOWING}. */
+  public final boolean isUnboundedFollowing() {
+    return isUnbounded() && isFollowing();
+  }
+
   /**
    * Returns if the bound is PRECEDING.
    *

core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

@@ -1720,8 +1720,8 @@ public RelNode convertToSingleValueSubq(
       SqlNodeList selectList = select.getSelectList();
       SqlNodeList groupList = select.getGroup();
 
-      if ((selectList.size() == 1)
-          && ((groupList == null) || (groupList.size() == 0))) {
+      if (selectList.size() == 1
+          && (groupList == null || groupList.isEmpty())) {
         SqlNode selectExpr = selectList.get(0);
         if (selectExpr instanceof SqlCall) {
           SqlCall selectExprCall = (SqlCall) selectExpr;
@@ -2284,8 +2284,9 @@ private RexNode convertOver(Blackboard bb, SqlNode node) {
         // ROW_NUMBER() expects specific kind of framing.
         rows = true;
       }
-    } else if (orderList.size() == 0) {
-      // Without ORDER BY, there must be no bracketing.
+    } else if (orderList.isEmpty() && !rows) {
+      // In RANGE without ORDER BY, all rows are equivalent, so bracketing has
+      // no effect.
       sqlLowerBound = SqlWindow.createUnboundedPreceding(SqlParserPos.ZERO);
       sqlUpperBound = SqlWindow.createUnboundedFollowing(SqlParserPos.ZERO);
     } else if (sqlLowerBound == null && sqlUpperBound == null) {
@@ -2307,7 +2308,7 @@ private RexNode convertOver(Blackboard bb, SqlNode node) {
         bb.convertExpression(requireNonNull(sqlLowerBound, "sqlLowerBound"));
     final RexNode upperBound =
         bb.convertExpression(requireNonNull(sqlUpperBound, "sqlUpperBound"));
-    if (orderList.size() == 0 && !rows) {
+    if (orderList.isEmpty() && !rows) {
       // A logical range requires an ORDER BY clause. Use the implicit
       // ordering of this relation. There must be one, otherwise it would
       // have failed validation.
@@ -2745,7 +2746,7 @@ protected void convertPivot(Blackboard bb, SqlPivot pivot) {
     // 3. Gather columns used as arguments to aggregate functions.
     pivotBb.agg = aggConverter;
     final List<@Nullable String> aggAliasList = new ArrayList<>();
-    assert aggConverter.aggCalls.size() == 0;
+    assert aggConverter.aggCalls.isEmpty();
     pivot.forEachAgg((alias, call) -> {
       call.accept(aggConverter);
       aggAliasList.add(alias);
@@ -3404,7 +3405,7 @@ private Pair<RexNode, RelNode> convertOnCondition(
     bb.setRoot(ImmutableList.of(leftRel, rightRel));
     replaceSubQueries(bb, condition, RelOptUtil.Logic.UNKNOWN_AS_FALSE);
     final RelNode newRightRel =
-        bb.root == null || bb.registered.size() == 0
+        bb.root == null || bb.registered.isEmpty()
             ? rightRel
             : bb.reRegister(rightRel);
     bb.setRoot(ImmutableList.of(leftRel, newRightRel));
@@ -4638,7 +4639,7 @@ private RelNode convertMultisets(final List<SqlNode> operands,
       joinList.add(relBuilder.build());
     }
 
-    if (joinList.size() == 0) {
+    if (joinList.isEmpty()) {
       joinList.add(lastList);
     }