Refactor AggregateDataCollecting and AggregateDataMedian to reduce code duplication

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

@@ -6,7 +6,7 @@
 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;
@@ -257,9 +257,9 @@ public class Aggregate extends Expression {
         return new SortOrder(session.getDatabase(), index, sortType, null);
     }
 
-    private void sortWithOrderBy(ArrayList<Value> list) {
+    private void sortWithOrderBy(Value[] array) {
         final SortOrder sortOrder = orderBySort;
-        Collections.sort(list, new Comparator<Value>() {
+        Arrays.sort(array, new Comparator<Value>() {
             @Override
             public int compare(Value v1, Value v2) {
                 return sortOrder.compare(((ValueArray) v1).getList(), ((ValueArray) v2).getList());
@@ -370,19 +370,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 (orderByList != null) {
-                sortWithOrderBy(list);
+                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();
@@ -397,18 +396,18 @@ 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 (orderByList != null) {
-                sortWithOrderBy(list);
+                sortWithOrderBy(array);
             }
-            v = ValueArray.get(list.toArray(new Value[list.size()]));
+            return ValueArray.get(array);
         }
-        return v;
+        return data.getValue(session.getDatabase(), dataType, distinct);
     }
 
     @Override

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