Merge pull request #1447 from katzyn/aggregate

Refactor OVER() processing code and fix some issues

h2/src/main/org/h2/command/dml/Query.java

@@ -276,8 +276,9 @@ public abstract class Query extends Prepared {
      * Update all aggregate function values.
      *
      * @param s the session
+     * @param window true for window processing stage, false for group stage
      */
-    public abstract void updateAggregate(Session s);
+    public abstract void updateAggregate(Session s, boolean window);
 
     /**
      * Call the before triggers on all tables.

h2/src/main/org/h2/command/dml/Select.java

@@ -8,7 +8,6 @@ package org.h2.command.dml;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.BitSet;
-import java.util.HashMap;
 import java.util.HashSet;
 import org.h2.api.ErrorCode;
 import org.h2.api.Trigger;
@@ -105,7 +104,8 @@ public class Select extends Query {
     SelectGroups groupData;
 
     private int havingIndex;
-    private boolean isGroupQuery, isGroupSortedQuery;
+    boolean isGroupQuery;
+    private boolean isGroupSortedQuery;
     private boolean isWindowQuery;
     private boolean isForUpdate, isForUpdateMvcc;
     private double cost;
@@ -177,8 +177,8 @@ public class Select extends Query {
         return group;
     }
 
-    public SelectGroups getGroupDataIfCurrent(boolean forAggregate) {
-        return groupData != null && (forAggregate || !isWindowQuery) && groupData.isCurrentGroup() ? groupData : null;
+    public SelectGroups getGroupDataIfCurrent(boolean window) {
+        return groupData != null && (window || groupData.isCurrentGroup()) ? groupData : null;
     }
 
     @Override
@@ -357,10 +357,9 @@ public class Select extends Query {
 
     private void queryWindow(int columnCount, LocalResult result, long offset, boolean quickOffset) {
         if (groupData == null) {
-            groupData = new SelectGroups(session, expressions, groupIndex);
+            groupData = SelectGroups.getInstance(session, expressions, isGroupQuery, groupIndex);
         }
         groupData.reset();
-        HashMap<ValueArray, ArrayList<Row>> rows = new HashMap<>();
         try {
             int rowNumber = 0;
             setCurrentRowNumber(0);
@@ -369,14 +368,8 @@ public class Select extends Query {
                 setCurrentRowNumber(rowNumber + 1);
                 if (isConditionMet()) {
                     rowNumber++;
-                    ValueArray key = groupData.nextSource();
-                    ArrayList<Row> groupRows = rows.get(key);
-                    if (groupRows == null) {
-                        groupRows = Utils.newSmallArrayList();
-                        rows.put(key, groupRows);
-                    }
-                    groupRows.add(topTableFilter.get());
-                    updateAgg(columnCount);
+                    groupData.nextSource();
+                    updateAgg(columnCount, true);
                     if (sampleSize > 0 && rowNumber >= sampleSize) {
                         break;
                     }
@@ -384,11 +377,8 @@ public class Select extends Query {
             }
             groupData.done();
             for (ValueArray currentGroupsKey; (currentGroupsKey = groupData.next()) != null;) {
-                for (Row originalRow : rows.get(currentGroupsKey)) {
-                    topTableFilter.set(originalRow);
                 offset = processGroupedRow(columnCount, result, offset, quickOffset, currentGroupsKey);
             }
-            }
         } finally {
             groupData.reset();
         }
@@ -396,7 +386,7 @@ public class Select extends Query {
 
     private void queryGroup(int columnCount, LocalResult result, long offset, boolean quickOffset) {
         if (groupData == null) {
-            groupData = new SelectGroups(session, expressions, groupIndex);
+            groupData = SelectGroups.getInstance(session, expressions, isGroupQuery, groupIndex);
         }
         groupData.reset();
         try {
@@ -408,7 +398,7 @@ public class Select extends Query {
                 if (isConditionMet()) {
                     rowNumber++;
                     groupData.nextSource();
-                    updateAgg(columnCount);
+                    updateAgg(columnCount, false);
                     if (sampleSize > 0 && rowNumber >= sampleSize) {
                         break;
                     }
@@ -423,11 +413,11 @@ public class Select extends Query {
         }
     }
 
-    private void updateAgg(int columnCount) {
+    private void updateAgg(int columnCount, boolean window) {
         for (int i = 0; i < columnCount; i++) {
             if (groupByExpression == null || !groupByExpression[i]) {
                 Expression expr = expressions.get(i);
-                expr.updateAggregate(session);
+                expr.updateAggregate(session, window);
             }
         }
     }
@@ -1492,15 +1482,15 @@ public class Select extends Query {
     }
 
     @Override
-    public void updateAggregate(Session s) {
+    public void updateAggregate(Session s, boolean window) {
         for (Expression e : expressions) {
-            e.updateAggregate(s);
+            e.updateAggregate(s, window);
         }
         if (condition != null) {
-            condition.updateAggregate(s);
+            condition.updateAggregate(s, window);
         }
         if (having != null) {
-            having.updateAggregate(s);
+            having.updateAggregate(s, window);
         }
     }
 
@@ -1654,7 +1644,7 @@ public class Select extends Query {
         LazyResultGroupSorted(Expression[] expressions, int columnCount) {
             super(expressions, columnCount);
             if (groupData == null) {
-                groupData = new SelectGroups(getSession(), Select.this.expressions, groupIndex);
+                groupData = SelectGroups.getInstance(getSession(), Select.this.expressions, isGroupQuery, groupIndex);
             } else {
                 // TODO is this branch possible?
                 groupData.resetLazy();
@@ -1696,7 +1686,7 @@ public class Select extends Query {
                     for (int i = 0; i < columnCount; i++) {
                         if (groupByExpression == null || !groupByExpression[i]) {
                             Expression expr = expressions.get(i);
-                            expr.updateAggregate(getSession());
+                            expr.updateAggregate(getSession(), false);
                         }
                     }
                     if (row != null) {

h2/src/main/org/h2/command/dml/SelectUnion.java

@@ -449,9 +449,9 @@ public class SelectUnion extends Query {
     }
 
     @Override
-    public void updateAggregate(Session s) {
-        left.updateAggregate(s);
-        right.updateAggregate(s);
+    public void updateAggregate(Session s, boolean window) {
+        left.updateAggregate(s, window);
+        right.updateAggregate(s, window);
     }
 
     @Override

h2/src/main/org/h2/expression/Alias.java

@@ -83,8 +83,8 @@ public class Alias extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        expr.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        expr.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/BinaryOperation.java

@@ -436,9 +436,9 @@ public class BinaryOperation extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
-        right.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
+        right.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/CompareLike.java

@@ -495,11 +495,11 @@ public class CompareLike extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
-        right.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
+        right.updateAggregate(session, window);
         if (escape != null) {
-            escape.updateAggregate(session);
+            escape.updateAggregate(session, window);
         }
     }
 

h2/src/main/org/h2/expression/Comparison.java

@@ -475,10 +475,10 @@ public class Comparison extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
         if (right != null) {
-            right.updateAggregate(session);
+            right.updateAggregate(session, window);
         }
     }
 

h2/src/main/org/h2/expression/ConditionAndOr.java

@@ -268,9 +268,9 @@ public class ConditionAndOr extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
-        right.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
+        right.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/ConditionExists.java

@@ -46,7 +46,7 @@ public class ConditionExists extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         // TODO exists: is it allowed that the subquery contains aggregates?
         // probably not
         // select id from test group by id having exists (select * from test2

h2/src/main/org/h2/expression/ConditionIn.java

@@ -163,10 +163,10 @@ public class ConditionIn extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
         for (Expression e : valueList) {
-            e.updateAggregate(session);
+            e.updateAggregate(session, window);
         }
     }
 

h2/src/main/org/h2/expression/ConditionInConstantSet.java

@@ -125,8 +125,8 @@ public class ConditionInConstantSet extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/ConditionInParameter.java

@@ -145,8 +145,8 @@ public class ConditionInParameter extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/ConditionInSelect.java

@@ -146,9 +146,9 @@ public class ConditionInSelect extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
-        query.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
+        query.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/ConditionNot.java

@@ -70,8 +70,8 @@ public class ConditionNot extends Condition {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        condition.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        condition.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/Expression.java

@@ -105,8 +105,9 @@ public abstract class Expression {
      * be used to make sure the internal state is only updated once.
      *
      * @param session the session
+     * @param window true for window processing stage, false for group stage
      */
-    public abstract void updateAggregate(Session session);
+    public abstract void updateAggregate(Session session, boolean window);
 
     /**
      * Check if this expression and all sub-expressions can fulfill a criteria.

h2/src/main/org/h2/expression/ExpressionColumn.java

@@ -153,7 +153,7 @@ public class ExpressionColumn extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         Value now = columnResolver.getValue(column);
         Select select = columnResolver.getSelect();
         if (select == null) {
@@ -164,9 +164,9 @@ public class ExpressionColumn extends Expression {
             // this is a different level (the enclosing query)
             return;
         }
-        Value v = (Value) groupData.getCurrentGroupExprData(this);
+        Value v = (Value) groupData.getCurrentGroupExprData(this, false);
         if (v == null) {
-            groupData.setCurrentGroupExprData(this, now);
+            groupData.setCurrentGroupExprData(this, now, false);
         } else {
             if (!database.areEqual(now, v)) {
                 throw DbException.get(ErrorCode.MUST_GROUP_BY_COLUMN_1, getSQL());
@@ -180,7 +180,7 @@ public class ExpressionColumn extends Expression {
         if (select != null) {
             SelectGroups groupData = select.getGroupDataIfCurrent(false);
             if (groupData != null) {
-                Value v = (Value) groupData.getCurrentGroupExprData(this);
+                Value v = (Value) groupData.getCurrentGroupExprData(this, false);
                 if (v != null) {
                     return v;
                 }

h2/src/main/org/h2/expression/ExpressionList.java

@@ -98,9 +98,9 @@ public class ExpressionList extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         for (Expression e : list) {
-            e.updateAggregate(session);
+            e.updateAggregate(session, window);
         }
     }
 

h2/src/main/org/h2/expression/Function.java

@@ -2633,10 +2633,10 @@ public class Function extends Expression implements FunctionCall {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         for (Expression e : args) {
             if (e != null) {
-                e.updateAggregate(session);
+                e.updateAggregate(session, window);
             }
         }
     }

h2/src/main/org/h2/expression/IntervalOperation.java

@@ -291,9 +291,9 @@ public class IntervalOperation extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        left.updateAggregate(session);
-        right.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        left.updateAggregate(session, window);
+        right.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/JavaFunction.java

@@ -107,10 +107,10 @@ public class JavaFunction extends Expression implements FunctionCall {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         for (Expression e : args) {
             if (e != null) {
-                e.updateAggregate(session);
+                e.updateAggregate(session, window);
             }
         }
     }

h2/src/main/org/h2/expression/Parameter.java

@@ -141,7 +141,7 @@ public class Parameter extends Expression implements ParameterInterface {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         // nothing to do
     }
 

h2/src/main/org/h2/expression/Rownum.java

@@ -73,7 +73,7 @@ public class Rownum extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         // nothing to do
     }
 

h2/src/main/org/h2/expression/SequenceValue.java

@@ -72,7 +72,7 @@ public class SequenceValue extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         // nothing to do
     }
 

h2/src/main/org/h2/expression/Subquery.java

@@ -94,8 +94,8 @@ public class Subquery extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        query.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        query.updateAggregate(session, window);
     }
 
     private Expression getExpression() {

h2/src/main/org/h2/expression/UnaryOperation.java

@@ -82,8 +82,8 @@ public class UnaryOperation extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        arg.updateAggregate(session);
+    public void updateAggregate(Session session, boolean window) {
+        arg.updateAggregate(session, window);
     }
 
     @Override

h2/src/main/org/h2/expression/ValueExpression.java

@@ -143,7 +143,7 @@ public class ValueExpression extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         // nothing to do
     }
 

h2/src/main/org/h2/expression/Variable.java

@@ -101,7 +101,7 @@ public class Variable extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         // nothing to do
     }
 

h2/src/main/org/h2/expression/Wildcard.java

@@ -91,7 +91,7 @@ public class Wildcard extends Expression {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
         DbException.throwInternalError(toString());
     }
 

h2/src/main/org/h2/expression/aggregate/AbstractAggregate.java

@@ -7,6 +7,7 @@ package org.h2.expression.aggregate;
 
 import org.h2.expression.Expression;
 import org.h2.table.ColumnResolver;
+import org.h2.table.TableFilter;
 
 /**
  * A base class for aggregates.
@@ -47,4 +48,24 @@ public abstract class AbstractAggregate extends Expression {
         }
     }
 
+    @Override
+    public void setEvaluatable(TableFilter tableFilter, boolean b) {
+        if (filterCondition != null) {
+            filterCondition.setEvaluatable(tableFilter, b);
+        }
+        if (over != null) {
+            over.setEvaluatable(tableFilter, b);
+        }
+    }
+
+    protected StringBuilder appendTailConditions(StringBuilder builder) {
+        if (filterCondition != null) {
+            builder.append(" FILTER (WHERE ").append(filterCondition.getSQL()).append(')');
+        }
+        if (over != null) {
+            builder.append(' ').append(over.getSQL());
+        }
+        return builder;
+    }
+
 }

h2/src/main/org/h2/expression/aggregate/Aggregate.java

@@ -170,10 +170,14 @@ public class Aggregate extends AbstractAggregate {
     /**
      * Create a new aggregate object.
      *
-     * @param type the aggregate type
-     * @param on the aggregated expression
-     * @param select the select statement
-     * @param distinct if distinct is used
+     * @param type
+     *            the aggregate type
+     * @param on
+     *            the aggregated expression
+     * @param select
+     *            the select statement
+     * @param distinct
+     *            if distinct is used
      */
     public Aggregate(AggregateType type, Expression on, Select select, boolean distinct) {
         this.type = type;
@@ -229,8 +233,10 @@ public class Aggregate extends AbstractAggregate {
      * Get the aggregate type for this name, or -1 if no aggregate has been
      * found.
      *
-     * @param name the aggregate function name
-     * @return null if no aggregate function has been found, or the aggregate type
+     * @param name
+     *            the aggregate function name
+     * @return null if no aggregate function has been found, or the aggregate
+     *         type
      */
     public static AggregateType getAggregateType(String name) {
         return AGGREGATES.get(name);
@@ -239,7 +245,8 @@ public class Aggregate extends AbstractAggregate {
     /**
      * Set the order for ARRAY_AGG() or GROUP_CONCAT() aggregate.
      *
-     * @param orderByList the order by list
+     * @param orderByList
+     *            the order by list
      */
     public void setOrderByList(ArrayList<SelectOrderBy> orderByList) {
         this.orderByList = orderByList;
@@ -248,7 +255,8 @@ public class Aggregate extends AbstractAggregate {
     /**
      * Set the separator for the GROUP_CONCAT() aggregate.
      *
-     * @param separator the separator expression
+     * @param separator
+     *            the separator expression
      */
     public void setGroupConcatSeparator(Expression separator) {
         this.groupConcatSeparator = separator;
@@ -281,12 +289,15 @@ public class Aggregate extends AbstractAggregate {
     }
 
     @Override
-    public void updateAggregate(Session session) {
+    public void updateAggregate(Session session, boolean window) {
+        if (window != (over != null)) {
+            return;
+        }
         // TODO aggregates: check nested MIN(MAX(ID)) and so on
         // if (on != null) {
         // on.updateAggregate();
         // }
-        SelectGroups groupData = select.getGroupDataIfCurrent(true);
+        SelectGroups groupData = select.getGroupDataIfCurrent(window);
         if (groupData == null) {
             // this is a different level (the enclosing query)
             return;
@@ -299,6 +310,9 @@ public class Aggregate extends AbstractAggregate {
         }
         lastGroupRowId = groupRowId;
 
+        if (over != null) {
+            over.updateAggregate(session, true);
+        }
         if (filterCondition != null) {
             if (!filterCondition.getBooleanValue(session)) {
                 return;
@@ -366,7 +380,7 @@ public class Aggregate extends AbstractAggregate {
                 DbException.throwInternalError("type=" + type);
             }
         }
-        SelectGroups groupData = select.getGroupDataIfCurrent(true);
+        SelectGroups groupData = select.getGroupDataIfCurrent(over != null);
         if (groupData == null) {
             throw DbException.get(ErrorCode.INVALID_USE_OF_AGGREGATE_FUNCTION_1, getSQL());
         }
@@ -381,8 +395,7 @@ public class Aggregate extends AbstractAggregate {
                 sortWithOrderBy(array);
             }
             StatementBuilder buff = new StatementBuilder();
-            String sep = groupConcatSeparator == null ?
-                    "," : groupConcatSeparator.getValue(session).getString();
+            String sep = groupConcatSeparator == null ? "," : groupConcatSeparator.getValue(session).getString();
             for (Value val : array) {
                 String s;
                 if (val.getType() == Value.ARRAY) {
@@ -431,10 +444,11 @@ public class Aggregate extends AbstractAggregate {
         ValueArray key;
         if (over != null && (key = over.getCurrentKey(session)) != null) {
             @SuppressWarnings("unchecked")
-            ValueHashMap<AggregateData> map = (ValueHashMap<AggregateData>) groupData.getCurrentGroupExprData(this);
+            ValueHashMap<AggregateData> map = (ValueHashMap<AggregateData>) groupData.getCurrentGroupExprData(this,
+                    true);
             if (map == null) {
                 map = new ValueHashMap<>();
-                groupData.setCurrentGroupExprData(this, map);
+                groupData.setCurrentGroupExprData(this, map, true);
             }
             data = map.get(key);
             if (data == null) {
@@ -442,10 +456,10 @@ public class Aggregate extends AbstractAggregate {
                 map.put(key, data);
             }
         } else {
-            data = (AggregateData) groupData.getCurrentGroupExprData(this);
+            data = (AggregateData) groupData.getCurrentGroupExprData(this, over != null);
             if (data == null) {
                 data = AggregateData.create(type);
-                groupData.setCurrentGroupExprData(this, data);
+                groupData.setCurrentGroupExprData(this, data, over != null);
             }
         }
         return data;
@@ -588,9 +602,7 @@ public class Aggregate extends AbstractAggregate {
         if (groupConcatSeparator != null) {
             groupConcatSeparator.setEvaluatable(tableFilter, b);
         }
-        if (filterCondition != null) {
-            filterCondition.setEvaluatable(tableFilter, b);
-        }
+        super.setEvaluatable(tableFilter, b);
     }
 
     @Override
@@ -626,13 +638,7 @@ public class Aggregate extends AbstractAggregate {
             buff.append(" SEPARATOR ").append(groupConcatSeparator.getSQL());
         }
         buff.append(')');
-        if (filterCondition != null) {
-            buff.append(" FILTER (WHERE ").append(filterCondition.getSQL()).append(')');
-        }
-        if (over != null) {
-            buff.append(' ').append(over.getSQL());
-        }
-        return buff.toString();
+        return appendTailConditions(buff.builder()).toString();
     }
 
     private String getSQLArrayAggregate() {
@@ -650,13 +656,7 @@ public class Aggregate extends AbstractAggregate {
             }
         }
         buff.append(')');
-        if (filterCondition != null) {
-            buff.append(" FILTER (WHERE ").append(filterCondition.getSQL()).append(')');
-        }
-        if (over != null) {
-            buff.append(' ').append(over.getSQL());
-        }
-        return buff.toString();
+        return appendTailConditions(buff.builder()).toString();
     }
 
     @Override
@@ -666,7 +666,7 @@ public class Aggregate extends AbstractAggregate {
         case GROUP_CONCAT:
             return getSQLGroupConcat();
         case COUNT_ALL:
-            return "COUNT(*)";
+            return appendTailConditions(new StringBuilder().append("COUNT(*)")).toString();
         case COUNT:
             text = "COUNT";
             break;
@@ -726,18 +726,13 @@ public class Aggregate extends AbstractAggregate {
         default:
             throw DbException.throwInternalError("type=" + type);
         }
+        StringBuilder builder = new StringBuilder().append(text);
         if (distinct) {
-            text += "(DISTINCT " + on.getSQL() + ')';
+            builder.append("(DISTINCT ").append(on.getSQL()).append(')');
         } else {
-            text += StringUtils.enclose(on.getSQL());
-        }
-        if (filterCondition != null) {
-            text += " FILTER (WHERE " + filterCondition.getSQL() + ')';
-        }
-        if (over != null) {
-            text += ' ' + over.getSQL();
+            builder.append(StringUtils.enclose(on.getSQL()));
         }
-        return text;
+        return appendTailConditions(builder).toString();
     }
 
     private Index getMinMaxColumnIndex() {
@@ -785,8 +780,7 @@ public class Aggregate extends AbstractAggregate {
         if (on != null && !on.isEverything(visitor)) {
             return false;
         }
-        if (groupConcatSeparator != null &&
-                !groupConcatSeparator.isEverything(visitor)) {
+        if (groupConcatSeparator != null && !groupConcatSeparator.isEverything(visitor)) {
             return false;
         }
         if (orderByList != null) {

h2/src/main/org/h2/expression/aggregate/JavaAggregate.java

@@ -83,13 +83,7 @@ public class JavaAggregate extends AbstractAggregate {
             buff.append(e.getSQL());
         }
         buff.append(')');
-        if (filterCondition != null) {
-            buff.append(" FILTER (WHERE ").append(filterCondition.getSQL()).append(')');
-        }
-        if (over != null) {
-            buff.append(' ').append(over.getSQL());
-        }
-        return buff.toString();
+        return appendTailConditions(buff.builder()).toString();
     }
 
     @Override
@@ -155,9 +149,7 @@ public class JavaAggregate extends AbstractAggregate {
         for (Expression e : args) {
             e.setEvaluatable(tableFilter, b);
         }
-        if (filterCondition != null) {
-            filterCondition.setEvaluatable(tableFilter, b);
-        }
+        super.setEvaluatable(tableFilter, b);
     }
 
     private Aggregate getInstance() throws SQLException {
@@ -168,7 +160,7 @@ public class JavaAggregate extends AbstractAggregate {
 
     @Override
     public Value getValue(Session session) {
-        SelectGroups groupData = select.getGroupDataIfCurrent(true);
+        SelectGroups groupData = select.getGroupDataIfCurrent(over != null);
         if (groupData == null) {
             throw DbException.get(ErrorCode.INVALID_USE_OF_AGGREGATE_FUNCTION_1, getSQL());
         }
@@ -208,8 +200,11 @@ public class JavaAggregate extends AbstractAggregate {
     }
 
     @Override
-    public void updateAggregate(Session session) {
-        SelectGroups groupData = select.getGroupDataIfCurrent(true);
+    public void updateAggregate(Session session, boolean window) {
+        if (window != (over != null)) {
+            return;
+        }
+        SelectGroups groupData = select.getGroupDataIfCurrent(window);
         if (groupData == null) {
             // this is a different level (the enclosing query)
             return;
@@ -222,6 +217,9 @@ public class JavaAggregate extends AbstractAggregate {
         }
         lastGroupRowId = groupRowId;
 
+        if (over != null) {
+            over.updateAggregate(session, true);
+        }
         if (filterCondition != null) {
             if (!filterCondition.getBooleanValue(session)) {
                 return;
@@ -261,13 +259,13 @@ public class JavaAggregate extends AbstractAggregate {
         ValueArray key;
         if (over != null && (key = over.getCurrentKey(session)) != null) {
             @SuppressWarnings("unchecked")
-            ValueHashMap<Aggregate> map = (ValueHashMap<Aggregate>) groupData.getCurrentGroupExprData(this);
+            ValueHashMap<Aggregate> map = (ValueHashMap<Aggregate>) groupData.getCurrentGroupExprData(this, true);
             if (map == null) {
                 if (ifExists) {
                     return null;
                 }
                 map = new ValueHashMap<>();
-                groupData.setCurrentGroupExprData(this, map);
+                groupData.setCurrentGroupExprData(this, map, true);
             }
             data = map.get(key);
             if (data == null) {
@@ -278,13 +276,13 @@ public class JavaAggregate extends AbstractAggregate {
                 map.put(key, data);
             }
         } else {
-            data = (Aggregate) groupData.getCurrentGroupExprData(this);
+            data = (Aggregate) groupData.getCurrentGroupExprData(this, over != null);
             if (data == null) {
                 if (ifExists) {
                     return null;
                 }
                 data = getInstance();
-                groupData.setCurrentGroupExprData(this, data);
+                groupData.setCurrentGroupExprData(this, data, over != null);
             }
         }
         return data;
@@ -295,14 +293,14 @@ public class JavaAggregate extends AbstractAggregate {
         ValueArray key;
         if (over != null && (key = over.getCurrentKey(session)) != null) {
             @SuppressWarnings("unchecked")
-            ValueHashMap<AggregateDataCollecting> map =
-                    (ValueHashMap<AggregateDataCollecting>) groupData.getCurrentGroupExprData(this);
+            ValueHashMap<AggregateDataCollecting> map = (ValueHashMap<AggregateDataCollecting>) groupData
+                    .getCurrentGroupExprData(this, true);
             if (map == null) {
                 if (ifExists) {
                     return null;
                 }
                 map = new ValueHashMap<>();
-                groupData.setCurrentGroupExprData(this, map);
+                groupData.setCurrentGroupExprData(this, map, true);
             }
             data = map.get(key);
             if (data == null) {
@@ -313,13 +311,13 @@ public class JavaAggregate extends AbstractAggregate {
                 map.put(key, data);
             }
         } else {
-            data = (AggregateDataCollecting) groupData.getCurrentGroupExprData(this);
+            data = (AggregateDataCollecting) groupData.getCurrentGroupExprData(this, over != null);
             if (data == null) {
                 if (ifExists) {
                     return null;
                 }
                 data = new AggregateDataCollecting();
-                groupData.setCurrentGroupExprData(this, data);
+                groupData.setCurrentGroupExprData(this, data, over != null);
             }
         }
         return data;

h2/src/main/org/h2/expression/aggregate/Window.java

@@ -10,6 +10,7 @@ import java.util.ArrayList;
 import org.h2.engine.Session;
 import org.h2.expression.Expression;
 import org.h2.table.ColumnResolver;
+import org.h2.table.TableFilter;
 import org.h2.util.StringUtils;
 import org.h2.value.Value;
 import org.h2.value.ValueArray;
@@ -38,6 +39,7 @@ public final class Window {
      *            the column resolver
      * @param level
      *            the subquery nesting level
+     * @see Expression#mapColumns(ColumnResolver, int)
      */
     public void mapColumns(ColumnResolver resolver, int level) {
         if (partitionBy != null) {
@@ -47,6 +49,24 @@ public final class Window {
         }
     }
 
+    /**
+     * Tell the expression columns whether the table filter can return values
+     * now. This is used when optimizing the query.
+     *
+     * @param tableFilter
+     *            the table filter
+     * @param value
+     *            true if the table filter can return value
+     * @see Expression#setEvaluatable(TableFilter, boolean)
+     */
+    public void setEvaluatable(TableFilter tableFilter, boolean value) {
+        if (partitionBy != null) {
+            for (Expression e : partitionBy) {
+                e.setEvaluatable(tableFilter, value);
+            }
+        }
+    }
+
     /**
      * Returns the key for the current group.
      *
@@ -72,6 +92,7 @@ public final class Window {
      * Returns SQL representation.
      *
      * @return SQL representation.
+     * @see Expression#getSQL()
      */
     public String getSQL() {
         if (partitionBy == null) {
@@ -87,6 +108,22 @@ public final class Window {
         return builder.append(')').toString();
     }
 
+    /**
+     * Update an aggregate value.
+     *
+     * @param session
+     *            the session
+     * @param window true for window processing stage, false for group stage
+     * @see Expression#updateAggregate(Session, boolean)
+     */
+    public void updateAggregate(Session session, boolean window) {
+        if (partitionBy != null) {
+            for (Expression expr : partitionBy) {
+                expr.updateAggregate(session, window);
+            }
+        }
+    }
+
     @Override
     public String toString() {
         return getSQL();

h2/src/test/org/h2/test/scripts/functions/aggregate/array-agg.sql

@@ -107,6 +107,17 @@ SELECT ARRAY_AGG(ID) OVER (PARTITION BY NAME), NAME FROM TEST;
 > (4, 5, 6)                              c
 > rows: 6
 
+SELECT ARRAY_AGG(ID) FILTER (WHERE ID < 3 OR ID > 4) OVER (PARTITION BY NAME), NAME FROM TEST ORDER BY NAME;
+> ARRAY_AGG(ID) FILTER (WHERE ((ID < 3) OR (ID > 4))) OVER (PARTITION BY NAME) NAME
+> ---------------------------------------------------------------------------- ----
+> (1, 2)                                                                       a
+> (1, 2)                                                                       a
+> null                                                                         b
+> (5, 6)                                                                       c
+> (5, 6)                                                                       c
+> (5, 6)                                                                       c
+> rows (ordered): 6
+
 SELECT ARRAY_AGG(SUM(ID)) OVER () FROM TEST;
 > exception FEATURE_NOT_SUPPORTED_1
 

h2/src/test/org/h2/test/scripts/functions/aggregate/count.sql

@@ -8,8 +8,8 @@
 create table test(v int);
 > ok
 
-insert into test values (1), (2), (3), (4), (5), (6), (7), (8), (9), (10), (11), (12);
-> update count: 12
+insert into test values (1), (2), (3), (4), (5), (6), (7), (8), (9), (10), (11), (12), (null);
+> update count: 13
 
 select count(v), count(v) filter (where v >= 4) from test where v <= 10;
 > COUNT(V) COUNT(V) FILTER (WHERE (V >= 4))
@@ -17,6 +17,19 @@ select count(v), count(v) filter (where v >= 4) from test where v <= 10;
 > 10       7
 > rows: 1
 
+select count(*), count(*) filter (where v >= 4) from test;
+> COUNT(*) COUNT(*) FILTER (WHERE (V >= 4))
+> -------- --------------------------------
+> 13       9
+> rows: 1
+
+
+select count(*), count(*) filter (where v >= 4) from test where v <= 10;
+> COUNT(*) COUNT(*) FILTER (WHERE (V >= 4))
+> -------- --------------------------------
+> 10       7
+> rows: 1
+
 create index test_idx on test(v);
 > ok