Merge pull request #1009 from katzyn/aggregate

Fix ARRAY_AGG with ORDER BY and refactor aggregates

h2/src/main/org/h2/command/Parser.java

@@ -2628,7 +2628,7 @@ public class Parser {
                     readExpression(), currentSelect, distinct);
                 if (readIf("ORDER")) {
                     read("BY");
-                    r.setGroupConcatOrder(parseSimpleOrderList());
+                    r.setOrderByList(parseSimpleOrderList());
                 }
 
                 if (readIf("SEPARATOR")) {
@@ -2642,7 +2642,7 @@ public class Parser {
                 r.setGroupConcatSeparator(readExpression());
                 if (readIf("ORDER")) {
                     read("BY");
-                    r.setGroupConcatOrder(parseSimpleOrderList());
+                    r.setOrderByList(parseSimpleOrderList());
                 }
             } else {
                 r = null;
@@ -2654,7 +2654,7 @@ public class Parser {
                 readExpression(), currentSelect, distinct);
             if (readIf("ORDER")) {
                 read("BY");
-                r.setArrayAggOrder(parseSimpleOrderList());
+                r.setOrderByList(parseSimpleOrderList());
             }
         } else {
             boolean distinct = readIf("DISTINCT");

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

@@ -6,12 +6,13 @@
 package org.h2.expression;
 
 import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Arrays;
 import java.util.Comparator;
 import java.util.HashMap;
 import org.h2.api.ErrorCode;
 import org.h2.command.dml.Select;
 import org.h2.command.dml.SelectOrderBy;
+import org.h2.engine.Database;
 import org.h2.engine.Session;
 import org.h2.index.Cursor;
 import org.h2.index.Index;
@@ -143,10 +144,8 @@ public class Aggregate extends Expression {
 
     private Expression on;
     private Expression groupConcatSeparator;
-    private ArrayList<SelectOrderBy> groupConcatOrderList;
-    private ArrayList<SelectOrderBy> arrayAggOrderList;
-    private SortOrder groupConcatSort;
-    private SortOrder arrayOrderSort;
+    private ArrayList<SelectOrderBy> orderByList;
+    private SortOrder orderBySort;
     private int dataType, scale;
     private long precision;
     private int displaySize;
@@ -220,21 +219,12 @@ public class Aggregate extends Expression {
     }
 
     /**
-     * Set the order for GROUP_CONCAT() aggregate.
+     * Set the order for ARRAY_AGG() or GROUP_CONCAT() aggregate.
      *
-     * @param orderBy the order by list
+     * @param orderByList the order by list
      */
-    public void setGroupConcatOrder(ArrayList<SelectOrderBy> orderBy) {
-        this.groupConcatOrderList = orderBy;
-    }
-
-    /**
-     * Set the order for ARRAY_AGG() aggregate.
-     *
-     * @param orderBy the order by list
-     */
-    public void setArrayAggOrder(ArrayList<SelectOrderBy> orderBy) {
-        this.arrayAggOrderList = orderBy;
+    public void setOrderByList(ArrayList<SelectOrderBy> orderByList) {
+        this.orderByList = orderByList;
     }
 
     /**
@@ -255,12 +245,12 @@ public class Aggregate extends Expression {
         this.filterCondition = filterCondition;
     }
 
-    private SortOrder initOrder(ArrayList<SelectOrderBy> orderList, Session session) {
-        int size = orderList.size();
+    private SortOrder initOrder(Session session) {
+        int size = orderByList.size();
         int[] index = new int[size];
         int[] sortType = new int[size];
         for (int i = 0; i < size; i++) {
-            SelectOrderBy o = orderList.get(i);
+            SelectOrderBy o = orderByList.get(i);
             index[i] = i + 1;
             int order = o.descending ? SortOrder.DESCENDING : SortOrder.ASCENDING;
             sortType[i] = order;
@@ -268,6 +258,26 @@ public class Aggregate extends Expression {
         return new SortOrder(session.getDatabase(), index, sortType, null);
     }
 
+    private void sortWithOrderBy(Value[] array) {
+        final SortOrder sortOrder = orderBySort;
+        if (sortOrder != null) {
+            Arrays.sort(array, new Comparator<Value>() {
+                @Override
+                public int compare(Value v1, Value v2) {
+                    return sortOrder.compare(((ValueArray) v1).getList(), ((ValueArray) v2).getList());
+                }
+            });
+        } else {
+            final Database database = select.getSession().getDatabase();
+            Arrays.sort(array, new Comparator<Value> () {
+                @Override
+                public int compare(Value v1, Value v2) {
+                    return database.compare(v1, v2);
+                }
+            });
+        }
+    }
+
     @Override
     public void updateAggregate(Session session) {
         // TODO aggregates: check nested MIN(MAX(ID)) and so on
@@ -296,12 +306,12 @@ public class Aggregate extends Expression {
         if (type == AggregateType.GROUP_CONCAT) {
             if (v != ValueNull.INSTANCE) {
                 v = v.convertTo(Value.STRING);
-                if (groupConcatOrderList != null) {
-                    int size = groupConcatOrderList.size();
+                if (orderByList != null) {
+                    int size = orderByList.size();
                     Value[] array = new Value[1 + size];
                     array[0] = v;
                     for (int i = 0; i < size; i++) {
-                        SelectOrderBy o = groupConcatOrderList.get(i);
+                        SelectOrderBy o = orderByList.get(i);
                         array[i + 1] = o.expression.getValue(session);
                     }
                     v = ValueArray.get(array);
@@ -310,12 +320,12 @@ public class Aggregate extends Expression {
         }
         if (type == AggregateType.ARRAY_AGG) {
             if (v != ValueNull.INSTANCE) {
-                if (arrayAggOrderList != null) {
-                    int size = arrayAggOrderList.size();
+                if (orderByList != null) {
+                    int size = orderByList.size();
                     Value[] array = new Value[1 + size];
                     array[0] = v;
                     for (int i = 0; i < size; i++) {
-                        SelectOrderBy o = arrayAggOrderList.get(i);
+                        SelectOrderBy o = orderByList.get(i);
                         array[i + 1] = o.expression.getValue(session);
                     }
                     v = ValueArray.get(array);
@@ -371,27 +381,18 @@ public class Aggregate extends Expression {
         if (data == null) {
             data = AggregateData.create(type);
         }
-        Value v = data.getValue(session.getDatabase(), dataType, distinct);
         if (type == AggregateType.GROUP_CONCAT) {
-            ArrayList<Value> list = ((AggregateDataArrayCollecting) data).getList();
-            if (list == null || list.isEmpty()) {
+            Value[] array = ((AggregateDataCollecting) data).getArray();
+            if (array == null) {
                 return ValueNull.INSTANCE;
             }
-            if (groupConcatOrderList != null) {
-                final SortOrder sortOrder = groupConcatSort;
-                Collections.sort(list, new Comparator<Value>() {
-                    @Override
-                    public int compare(Value v1, Value v2) {
-                        Value[] a1 = ((ValueArray) v1).getList();
-                        Value[] a2 = ((ValueArray) v2).getList();
-                        return sortOrder.compare(a1, a2);
-                    }
-                });
+            if (orderByList != null || distinct) {
+                sortWithOrderBy(array);
             }
             StatementBuilder buff = new StatementBuilder();
             String sep = groupConcatSeparator == null ?
                     "," : groupConcatSeparator.getValue(session).getString();
-            for (Value val : list) {
+            for (Value val : array) {
                 String s;
                 if (val.getType() == Value.ARRAY) {
                     s = ((ValueArray) val).getList()[0].getString();
@@ -406,26 +407,23 @@ public class Aggregate extends Expression {
                 }
                 buff.append(s);
             }
-            v = ValueString.get(buff.toString());
+            return ValueString.get(buff.toString());
         } else if (type == AggregateType.ARRAY_AGG) {
-            ArrayList<Value> list = ((AggregateDataArrayCollecting) data).getList();
-            if (list == null || list.isEmpty()) {
+            Value[] array = ((AggregateDataCollecting) data).getArray();
+            if (array == null) {
                 return ValueNull.INSTANCE;
             }
-            if (arrayAggOrderList != null) {
-                final SortOrder sortOrder = arrayOrderSort;
-                Collections.sort(list, new Comparator<Value>() {
-                    @Override
-                    public int compare(Value v1, Value v2) {
-                        Value[] a1 = ((ValueArray) v1).getList();
-                        Value[] a2 = ((ValueArray) v2).getList();
-                        return sortOrder.compare(a1, a2);
-                    }
-                });
+            if (orderByList != null || distinct) {
+                sortWithOrderBy(array);
+            }
+            if (orderByList != null) {
+                for (int i = 0; i < array.length; i++) {
+                    array[i] = ((ValueArray) array[i]).getList()[0];
+                }
             }
-            v = ValueArray.get(list.toArray(new Value[list.size()]));
+            return ValueArray.get(array);
         }
-        return v;
+        return data.getValue(session.getDatabase(), dataType, distinct);
     }
 
     @Override
@@ -438,13 +436,8 @@ public class Aggregate extends Expression {
         if (on != null) {
             on.mapColumns(resolver, level);
         }
-        if (groupConcatOrderList != null) {
-            for (SelectOrderBy o : groupConcatOrderList) {
-                o.expression.mapColumns(resolver, level);
-            }
-        }
-        if (arrayAggOrderList != null) {
-            for (SelectOrderBy o : arrayAggOrderList) {
+        if (orderByList != null) {
+            for (SelectOrderBy o : orderByList) {
                 o.expression.mapColumns(resolver, level);
             }
         }
@@ -465,17 +458,11 @@ public class Aggregate extends Expression {
             precision = on.getPrecision();
             displaySize = on.getDisplaySize();
         }
-        if (groupConcatOrderList != null) {
-            for (SelectOrderBy o : groupConcatOrderList) {
+        if (orderByList != null) {
+            for (SelectOrderBy o : orderByList) {
                 o.expression = o.expression.optimize(session);
             }
-            groupConcatSort = initOrder(groupConcatOrderList, session);
-        }
-        if (arrayAggOrderList != null) {
-            for (SelectOrderBy o : arrayAggOrderList) {
-                o.expression = o.expression.optimize(session);
-            }
-            arrayOrderSort = initOrder(arrayAggOrderList, session);
+            orderBySort = initOrder(session);
         }
         if (groupConcatSeparator != null) {
             groupConcatSeparator = groupConcatSeparator.optimize(session);
@@ -564,13 +551,8 @@ public class Aggregate extends Expression {
         if (on != null) {
             on.setEvaluatable(tableFilter, b);
         }
-        if (groupConcatOrderList != null) {
-            for (SelectOrderBy o : groupConcatOrderList) {
-                o.expression.setEvaluatable(tableFilter, b);
-            }
-        }
-        if (arrayAggOrderList != null) {
-            for (SelectOrderBy o : arrayAggOrderList) {
+        if (orderByList != null) {
+            for (SelectOrderBy o : orderByList) {
                 o.expression.setEvaluatable(tableFilter, b);
             }
         }
@@ -603,9 +585,9 @@ public class Aggregate extends Expression {
             buff.append("DISTINCT ");
         }
         buff.append(on.getSQL());
-        if (groupConcatOrderList != null) {
+        if (orderByList != null) {
             buff.append(" ORDER BY ");
-            for (SelectOrderBy o : groupConcatOrderList) {
+            for (SelectOrderBy o : orderByList) {
                 buff.appendExceptFirst(", ");
                 buff.append(o.expression.getSQL());
                 if (o.descending) {
@@ -629,9 +611,9 @@ public class Aggregate extends Expression {
             buff.append("DISTINCT ");
         }
         buff.append(on.getSQL());
-        if (arrayAggOrderList != null) {
+        if (orderByList != null) {
             buff.append(" ORDER BY ");
-            for (SelectOrderBy o : arrayAggOrderList) {
+            for (SelectOrderBy o : orderByList) {
                 buff.appendExceptFirst(", ");
                 buff.append(o.expression.getSQL());
                 if (o.descending) {
@@ -765,15 +747,8 @@ public class Aggregate extends Expression {
                 !groupConcatSeparator.isEverything(visitor)) {
             return false;
         }
-        if (groupConcatOrderList != null) {
-            for (SelectOrderBy o : groupConcatOrderList) {
-                if (!o.expression.isEverything(visitor)) {
-                    return false;
-                }
-            }
-        }
-        if (arrayAggOrderList != null) {
-            for (SelectOrderBy o : arrayAggOrderList) {
+        if (orderByList != null) {
+            for (SelectOrderBy o : orderByList) {
                 if (!o.expression.isEverything(visitor)) {
                     return false;
                 }

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

@@ -21,21 +21,21 @@ abstract class AggregateData {
      * @return the aggregate data object of the specified type
      */
     static AggregateData create(AggregateType aggregateType) {
-        if (aggregateType == AggregateType.SELECTIVITY) {
+        switch (aggregateType) {
+        case SELECTIVITY:
             return new AggregateDataSelectivity();
-        } else if (aggregateType == AggregateType.GROUP_CONCAT) {
-            return new AggregateDataArrayCollecting();
-        } else if (aggregateType == AggregateType.ARRAY_AGG) {
-            return new AggregateDataArrayCollecting();
-        } else if (aggregateType == AggregateType.COUNT_ALL) {
+        case GROUP_CONCAT:
+        case ARRAY_AGG:
+            return new AggregateDataCollecting();
+        case COUNT_ALL:
             return new AggregateDataCountAll();
-        } else if (aggregateType == AggregateType.COUNT) {
+        case COUNT:
             return new AggregateDataCount();
-        } else if (aggregateType == AggregateType.HISTOGRAM) {
+        case HISTOGRAM:
             return new AggregateDataHistogram();
-        } else if (aggregateType == AggregateType.MEDIAN) {
+        case MEDIAN:
             return new AggregateDataMedian();
-        } else {
+        default:
             return new AggregateDataDefault(aggregateType);
         }
     }

h2/src/main/org/h2/expression/AggregateDataArrayCollecting.java → h2/src/main/org/h2/expression/AggregateDataCollecting.java

@@ -6,55 +6,54 @@
 package org.h2.expression;
 
 import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+
 import org.h2.engine.Database;
-import org.h2.util.New;
-import org.h2.util.ValueHashMap;
 import org.h2.value.Value;
 import org.h2.value.ValueNull;
 
 /**
- * Data stored while calculating a GROUP_CONCAT/ARRAY_AGG aggregate.
+ * Data stored while calculating an aggregate that needs collecting of all
+ * values.
+ *
+ * <p>
+ * NULL values are not collected. {@link #getValue(Database, int, boolean)}
+ * method returns {@code null}. Use {@link #getArray()} for instances of this
+ * class instead. Notice that subclasses like {@link AggregateDataMedian} may
+ * override {@link #getValue(Database, int, boolean)} to return useful result.
+ * </p>
  */
-class AggregateDataArrayCollecting extends AggregateData {
-    private ArrayList<Value> list;
-    private ValueHashMap<AggregateDataArrayCollecting> distinctValues;
+class AggregateDataCollecting extends AggregateData {
+    Collection<Value> values;
 
     @Override
     void add(Database database, int dataType, boolean distinct, Value v) {
         if (v == ValueNull.INSTANCE) {
             return;
         }
-        if (distinct) {
-            if (distinctValues == null) {
-                distinctValues = ValueHashMap.newInstance();
-            }
-            distinctValues.put(v, this);
-            return;
-        }
-        if (list == null) {
-            list = New.arrayList();
+        Collection<Value> c = values;
+        if (c == null) {
+            values = c = distinct ? new HashSet<Value>() : new ArrayList<Value>();
         }
-        list.add(v);
+        c.add(v);
     }
 
     @Override
     Value getValue(Database database, int dataType, boolean distinct) {
-        if (distinct) {
-            distinct(database, dataType);
-        }
         return null;
     }
 
-    ArrayList<Value> getList() {
-        return list;
-    }
-
-    private void distinct(Database database, int dataType) {
-        if (distinctValues == null) {
-            return;
-        }
-        for (Value v : distinctValues.keys()) {
-            add(database, dataType, false, v);
+    /**
+     * Returns array with values or {@code null}.
+     *
+     * @return array with values or {@code null}
+     */
+    Value[] getArray() {
+        Collection<Value> values = this.values;
+        if (values == null) {
+            return null;
         }
+        return values.toArray(new Value[0]);
     }
 }

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

@@ -8,9 +8,7 @@ package org.h2.expression;
 import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Comparator;
-import java.util.HashSet;
 
 import org.h2.engine.Database;
 import org.h2.engine.Session;
@@ -39,9 +37,7 @@ import org.h2.value.ValueTimestampTimeZone;
 /**
  * Data stored while calculating a MEDIAN aggregate.
  */
-class AggregateDataMedian extends AggregateData {
-    private Collection<Value> values;
-
+class AggregateDataMedian extends AggregateDataCollecting {
     private static boolean isNullsLast(Index index) {
         IndexColumn ic = index.getIndexColumns()[0];
         int sortType = ic.sortType;
@@ -168,29 +164,12 @@ class AggregateDataMedian extends AggregateData {
         return v;
     }
 
-    @Override
-    void add(Database database, int dataType, boolean distinct, Value v) {
-        if (v == ValueNull.INSTANCE) {
-            return;
-        }
-        Collection<Value> c = values;
-        if (c == null) {
-            values = c = distinct ? new HashSet<Value>() : new ArrayList<Value>();
-        }
-        c.add(v);
-    }
-
     @Override
     Value getValue(Database database, int dataType, boolean distinct) {
-        Collection<Value> c = values;
-        // Non-null collection cannot be empty here
-        if (c == null) {
+        Value[] a = getArray();
+        if (a == null) {
             return ValueNull.INSTANCE;
         }
-        if (distinct && c instanceof ArrayList) {
-            c = new HashSet<>(c);
-        }
-        Value[] a = c.toArray(new Value[0]);
         final CompareMode mode = database.getCompareMode();
         Arrays.sort(a, new Comparator<Value>() {
             @Override

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

@@ -14,10 +14,9 @@ insert into test values ('1'), ('2'), ('3'), ('4'), ('5'), ('6'), ('7'), ('8'),
 select array_agg(v order by v asc),
     array_agg(v order by v desc) filter (where v >= '4')
     from test where v >= '2';
-> ARRAY_AGG(V ORDER BY V)                                          ARRAY_AGG(V ORDER BY V DESC) FILTER (WHERE (V >= '4'))
-> ---------------------------------------------------------------- ------------------------------------------------------
-------------------------------
-> (2, 3, 4, 5, 6, 7, 8, 9)       (9, 8, 7, 6, 5, 4)
+> ARRAY_AGG(V ORDER BY V)  ARRAY_AGG(V ORDER BY V DESC) FILTER (WHERE (V >= '4'))
+> ------------------------ ------------------------------------------------------
+> (2, 3, 4, 5, 6, 7, 8, 9) (9, 8, 7, 6, 5, 4)
 > rows (ordered): 1
 
 create index test_idx on test(v);
@@ -25,21 +24,45 @@ create index test_idx on test(v);
 select ARRAY_AGG(v order by v asc),
     ARRAY_AGG(v order by v desc) filter (where v >= '4')
     from test where v >= '2';
-> ARRAY_AGG(V ORDER BY V)                                          ARRAY_AGG(V ORDER BY V DESC) FILTER (WHERE (V >= '4'))
-> ---------------------------------------------------------------- ------------------------------------------------------
-------------------------------
-> (2, 3, 4, 5, 6, 7, 8, 9)       (9, 8, 7, 6, 5, 4)
+> ARRAY_AGG(V ORDER BY V)  ARRAY_AGG(V ORDER BY V DESC) FILTER (WHERE (V >= '4'))
+> ------------------------ ------------------------------------------------------
+> (2, 3, 4, 5, 6, 7, 8, 9) (9, 8, 7, 6, 5, 4)
 > rows (ordered): 1
 
 select ARRAY_AGG(v order by v asc),
     ARRAY_AGG(v order by v desc) filter (where v >= '4')
     from test;
-> ARRAY_AGG(V ORDER BY V)                                                  ARRAY_AGG(V ORDER BY V DESC) FILTER (WHERE (V >= '4'))
-> ------------------------------------------------------------------------ ------------------------------------------------------
-------------------------------
-> (1, 2, 3, 4, 5, 6, 7, 8, 9)    (9, 8, 7, 6, 5, 4)
+> ARRAY_AGG(V ORDER BY V)     ARRAY_AGG(V ORDER BY V DESC) FILTER (WHERE (V >= '4'))
+> --------------------------- ------------------------------------------------------
+> (1, 2, 3, 4, 5, 6, 7, 8, 9) (9, 8, 7, 6, 5, 4)
 > rows (ordered): 1
 
+drop table test;
+> ok
+
+create table test (id int auto_increment primary key, v int);
+> ok
+
+insert into test(v) values (7), (2), (8), (3), (7), (3), (9), (-1);
+> update count: 8
+
+select array_agg(v) from test;
+> ARRAY_AGG(V)
+> -------------------------
+> (7, 2, 8, 3, 7, 3, 9, -1)
+> rows: 1
+
+select array_agg(distinct v) from test;
+> ARRAY_AGG(DISTINCT V)
+> ---------------------
+> (-1, 2, 3, 7, 8, 9)
+> rows: 1
+
+select array_agg(distinct v order by v desc) from test;
+> ARRAY_AGG(DISTINCT V ORDER BY V DESC)
+> -------------------------------------
+> (9, 8, 7, 3, 2, -1)
+> rows (ordered): 1
 
 drop table test;
 > ok

h2/src/test/org/h2/test/scripts/functions/aggregate/group-concat.sql

@@ -37,6 +37,32 @@ select group_concat(v order by v asc separator '-'),
 > 1-2-3-4-5-6-7-8-9                        9-8-7-6-5-4
 > rows (ordered): 1
 
+drop table test;
+> ok
+
+create table test (id int auto_increment primary key, v int);
+> ok
+
+insert into test(v) values (7), (2), (8), (3), (7), (3), (9), (-1);
+> update count: 8
+
+select group_concat(v) from test;
+> GROUP_CONCAT(V)
+> ----------------
+> 7,2,8,3,7,3,9,-1
+> rows: 1
+
+select group_concat(distinct v) from test;
+> GROUP_CONCAT(DISTINCT V)
+> ------------------------
+> -1,2,3,7,8,9
+> rows: 1
+
+select group_concat(distinct v order by v desc) from test;
+> GROUP_CONCAT(DISTINCT V ORDER BY V DESC)
+> ----------------------------------------
+> 9,8,7,3,2,-1
+> rows (ordered): 1
 
 drop table test;
 > ok