Merge pull request #1705 from katzyn/aggregate

Fix GROUP_CONCAT with variable separator

h2/src/docsrc/help/help.csv

@@ -3545,6 +3545,7 @@ GROUP_CONCAT ( [ DISTINCT|ALL ] string
 [FILTER (WHERE expression)] [OVER windowNameOrSpecification]
 ","
 Concatenates strings with a separator.
+Separator must be the same for all rows in the same group.
 The default separator is a ',' (without space).
 This method returns a string.
 If no rows are selected, the result is NULL.

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

@@ -3041,43 +3041,50 @@ public class Parser {
         switch (aggregateType) {
         case COUNT:
             if (readIf(ASTERISK)) {
-                r = new Aggregate(AggregateType.COUNT_ALL, null, currentSelect, false);
+                r = new Aggregate(AggregateType.COUNT_ALL, new Expression[0], currentSelect, false);
             } else {
                 boolean distinct = readDistinctAgg();
                 Expression on = readExpression();
                 if (on instanceof Wildcard && !distinct) {
                     // PostgreSQL compatibility: count(t.*)
-                    r = new Aggregate(AggregateType.COUNT_ALL, null, currentSelect, false);
+                    r = new Aggregate(AggregateType.COUNT_ALL, new Expression[0], currentSelect, false);
                 } else {
-                    r = new Aggregate(AggregateType.COUNT, on, currentSelect, distinct);
+                    r = new Aggregate(AggregateType.COUNT, new Expression[] { on }, currentSelect, distinct);
                 }
             }
             break;
         case GROUP_CONCAT: {
             boolean distinct = readDistinctAgg();
-            r = new Aggregate(AggregateType.GROUP_CONCAT, readExpression(), currentSelect, distinct);
+            Expression arg = readExpression(), separator = null;
+            ArrayList<SelectOrderBy> orderByList = null;
             if (equalsToken("STRING_AGG", aggregateName)) {
                 // PostgreSQL compatibility: string_agg(expression, delimiter)
                 read(COMMA);
-                r.setGroupConcatSeparator(readExpression());
+                separator = readExpression();
                 if (readIf(ORDER)) {
                     read("BY");
-                    r.setOrderByList(parseSimpleOrderList());
+                    orderByList = parseSimpleOrderList();
                 }
             } else {
                 if (readIf(ORDER)) {
                     read("BY");
-                    r.setOrderByList(parseSimpleOrderList());
+                    orderByList = parseSimpleOrderList();
                 }
                 if (readIf("SEPARATOR")) {
-                    r.setGroupConcatSeparator(readExpression());
+                    separator = readExpression();
                 }
             }
+            r = new Aggregate(AggregateType.GROUP_CONCAT,
+                    separator == null ? new Expression[] { arg } : new Expression[] { arg, separator },
+                            currentSelect, distinct);
+            if (orderByList != null) {
+                r.setOrderByList(orderByList);
+            }
             break;
         }
         case ARRAY_AGG: {
             boolean distinct = readDistinctAgg();
-            r = new Aggregate(AggregateType.ARRAY_AGG, readExpression(), currentSelect, distinct);
+            r = new Aggregate(AggregateType.ARRAY_AGG, new Expression[] { readExpression() }, currentSelect, distinct);
             if (readIf(ORDER)) {
                 read("BY");
                 r.setOrderByList(parseSimpleOrderList());
@@ -3088,15 +3095,15 @@ public class Parser {
         case PERCENTILE_DISC: {
             Expression num = readExpression();
             read(CLOSE_PAREN);
-            r = readWithinGroup(aggregateType, num);
+            r = readWithinGroup(aggregateType, new Expression[] { num });
             break;
         }
         case MODE: {
             if (readIf(CLOSE_PAREN)) {
-                r = readWithinGroup(AggregateType.MODE, null);
+                r = readWithinGroup(AggregateType.MODE, new Expression[0]);
             } else {
                 Expression expr = readExpression();
-                r = new Aggregate(aggregateType, null, currentSelect, false);
+                r = new Aggregate(aggregateType, new Expression[0], currentSelect, false);
                 if (readIf(ORDER)) {
                     read("BY");
                     Expression expr2 = readExpression();
@@ -3114,7 +3121,7 @@ public class Parser {
         }
         default:
             boolean distinct = readDistinctAgg();
-            r = new Aggregate(aggregateType, readExpression(), currentSelect, distinct);
+            r = new Aggregate(aggregateType, new Expression[] { readExpression() }, currentSelect, distinct);
             break;
         }
         read(CLOSE_PAREN);
@@ -3122,7 +3129,7 @@ public class Parser {
         return r;
     }
 
-    private Aggregate readWithinGroup(AggregateType aggregateType, Expression argument) {
+    private Aggregate readWithinGroup(AggregateType aggregateType, Expression[] args) {
         Aggregate r;
         read("WITHIN");
         read(GROUP);
@@ -3130,7 +3137,7 @@ public class Parser {
         read(ORDER);
         read("BY");
         Expression expr = readExpression();
-        r = new Aggregate(aggregateType, argument, currentSelect, false);
+        r = new Aggregate(aggregateType, args, currentSelect, false);
         readAggregateOrder(r, expr, true);
         return r;
     }

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

@@ -22,6 +22,7 @@ import org.h2.expression.analysis.WindowFrameExclusion;
 import org.h2.expression.analysis.WindowFrameUnits;
 import org.h2.table.ColumnResolver;
 import org.h2.table.TableFilter;
+import org.h2.value.TypeInfo;
 import org.h2.value.Value;
 
 /**
@@ -34,13 +35,24 @@ public abstract class AbstractAggregate extends DataAnalysisOperation {
      */
     protected final boolean distinct;
 
+    /**
+     * The arguments.
+     */
+    protected final Expression[] args;
+
     /**
      * FILTER condition for aggregate
      */
     protected Expression filterCondition;
 
-    AbstractAggregate(Select select, boolean distinct) {
+    /**
+     * The type of the result.
+     */
+    protected TypeInfo type;
+
+    AbstractAggregate(Select select, Expression[] args, boolean distinct) {
         super(select);
+        this.args = args;
         this.distinct = distinct;
     }
 
@@ -59,8 +71,16 @@ public abstract class AbstractAggregate extends DataAnalysisOperation {
         this.filterCondition = filterCondition;
     }
 
+    @Override
+    public TypeInfo getType() {
+        return type;
+    }
+
     @Override
     public void mapColumnsAnalysis(ColumnResolver resolver, int level, int innerState) {
+        for (Expression arg : args) {
+            arg.mapColumns(resolver, level, innerState);
+        }
         if (filterCondition != null) {
             filterCondition.mapColumns(resolver, level, innerState);
         }
@@ -69,6 +89,9 @@ public abstract class AbstractAggregate extends DataAnalysisOperation {
 
     @Override
     public Expression optimize(Session session) {
+        for (int i = 0; i < args.length; i++) {
+            args[i] = args[i].optimize(session);
+        }
         if (filterCondition != null) {
             filterCondition = filterCondition.optimize(session);
         }
@@ -77,6 +100,9 @@ public abstract class AbstractAggregate extends DataAnalysisOperation {
 
     @Override
     public void setEvaluatable(TableFilter tableFilter, boolean b) {
+        for (Expression arg : args) {
+            arg.setEvaluatable(tableFilter, b);
+        }
         if (filterCondition != null) {
             filterCondition.setEvaluatable(tableFilter, b);
         }

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

@@ -53,31 +53,27 @@ public class Aggregate extends AbstractAggregate {
 
     private final AggregateType aggregateType;
 
-    private Expression on;
-    private Expression groupConcatSeparator;
     private ArrayList<SelectOrderBy> orderByList;
     private SortOrder orderBySort;
-    private TypeInfo type;
 
     /**
      * Create a new aggregate object.
      *
      * @param aggregateType
      *            the aggregate type
-     * @param on
-     *            the aggregated expression
+     * @param args
+     *            the aggregated expressions
      * @param select
      *            the select statement
      * @param distinct
      *            if distinct is used
      */
-    public Aggregate(AggregateType aggregateType, Expression on, Select select, boolean distinct) {
-        super(select, distinct);
+    public Aggregate(AggregateType aggregateType, Expression[] args, Select select, boolean distinct) {
+        super(select, args, distinct);
         if (distinct && aggregateType == AggregateType.COUNT_ALL) {
             throw DbException.throwInternalError();
         }
         this.aggregateType = aggregateType;
-        this.on = on;
     }
 
     static {
@@ -149,16 +145,6 @@ public class Aggregate extends AbstractAggregate {
         this.orderByList = orderByList;
     }
 
-    /**
-     * Set the separator for the GROUP_CONCAT() aggregate.
-     *
-     * @param separator
-     *            the separator expression
-     */
-    public void setGroupConcatSeparator(Expression separator) {
-        this.groupConcatSeparator = separator;
-    }
-
     /**
      * Returns the type of this aggregate.
      *
@@ -185,7 +171,7 @@ public class Aggregate extends AbstractAggregate {
     @Override
     protected void updateAggregate(Session session, Object aggregateData) {
         AggregateData data = (AggregateData) aggregateData;
-        Value v = on == null ? null : on.getValue(session);
+        Value v = args.length == 0 ? null : args[0].getValue(session);
         updateData(session, data, v, null);
     }
 
@@ -195,6 +181,10 @@ public class Aggregate extends AbstractAggregate {
             if (v != ValueNull.INSTANCE) {
                 v = updateCollecting(session, v.convertTo(Value.STRING), remembered);
             }
+            if (args.length >= 2) {
+                ((AggregateDataCollecting) data).setSharedArgument(
+                        remembered != null ? remembered[1] : args[1].getValue(session));
+            }
             break;
         case ARRAY_AGG:
             if (v != ValueNull.INSTANCE) {
@@ -218,8 +208,8 @@ public class Aggregate extends AbstractAggregate {
     @Override
     protected void updateGroupAggregates(Session session, int stage) {
         super.updateGroupAggregates(session, stage);
-        if (on != null) {
-            on.updateAggregate(session, stage);
+        for (Expression arg : args) {
+            arg.updateAggregate(session, stage);
         }
         if (orderByList != null) {
             for (SelectOrderBy orderBy : orderByList) {
@@ -248,7 +238,7 @@ public class Aggregate extends AbstractAggregate {
 
     @Override
     protected int getNumExpressions() {
-        int n = on != null ? 1 : 0;
+        int n = args.length;
         if (orderByList != null) {
             n += orderByList.size();
         }
@@ -261,8 +251,8 @@ public class Aggregate extends AbstractAggregate {
     @Override
     protected void rememberExpressions(Session session, Value[] array) {
         int offset = 0;
-        if (on != null) {
-            array[offset++] = on.getValue(session);
+        for (Expression arg : args) {
+            array[offset++] = arg.getValue(session);
         }
         if (orderByList != null) {
             for (SelectOrderBy o : orderByList) {
@@ -278,7 +268,7 @@ public class Aggregate extends AbstractAggregate {
     protected void updateFromExpressions(Session session, Object aggregateData, Value[] array) {
         if (filterCondition == null || array[getNumExpressions() - 1].getBoolean()) {
             AggregateData data = (AggregateData) aggregateData;
-            Value v = on == null ? null : array[0];
+            Value v = args.length == 0 ? null : array[0];
             updateData(session, data, v, array);
         }
     }
@@ -319,7 +309,7 @@ public class Aggregate extends AbstractAggregate {
         }
         case PERCENTILE_CONT:
         case PERCENTILE_DISC: {
-            Value v = on.getValue(session);
+            Value v = args[0].getValue(session);
             if (v == ValueNull.INSTANCE) {
                 return ValueNull.INSTANCE;
             }
@@ -333,9 +323,9 @@ public class Aggregate extends AbstractAggregate {
             }
         }
         case MEDIAN:
-            return Percentile.getFromIndex(session, on, type.getValueType(), orderByList, Percentile.HALF, true);
+            return Percentile.getFromIndex(session, args[0], type.getValueType(), orderByList, Percentile.HALF, true);
         case ENVELOPE:
-            return ((MVSpatialIndex) AggregateDataEnvelope.getGeometryColumnIndex(on)).getBounds(session);
+            return ((MVSpatialIndex) AggregateDataEnvelope.getGeometryColumnIndex(args[0])).getBounds(session);
         default:
             throw DbException.throwInternalError("type=" + aggregateType);
         }
@@ -429,7 +419,8 @@ public class Aggregate extends AbstractAggregate {
     }
 
     private Value getGroupConcat(Session session, AggregateData data) {
-        Value[] array = ((AggregateDataCollecting) data).getArray();
+        AggregateDataCollecting collectingData = (AggregateDataCollecting) data;
+        Value[] array = collectingData.getArray();
         if (array == null) {
             return ValueNull.INSTANCE;
         }
@@ -437,7 +428,7 @@ public class Aggregate extends AbstractAggregate {
             sortWithOrderBy(array);
         }
         StatementBuilder buff = new StatementBuilder();
-        String sep = groupConcatSeparator == null ? "," : groupConcatSeparator.getValue(session).getString();
+        String sep = args.length < 2 ? "," : collectingData.getSharedArgument().getString();
         for (Value val : array) {
             String s;
             if (val.getValueType() == Value.ARRAY) {
@@ -521,33 +512,21 @@ public class Aggregate extends AbstractAggregate {
         return v.convertTo(type.getValueType());
     }
 
-    @Override
-    public TypeInfo getType() {
-        return type;
-    }
-
     @Override
     public void mapColumnsAnalysis(ColumnResolver resolver, int level, int innerState) {
-        if (on != null) {
-            on.mapColumns(resolver, level, innerState);
-        }
         if (orderByList != null) {
             for (SelectOrderBy o : orderByList) {
                 o.expression.mapColumns(resolver, level, innerState);
             }
         }
-        if (groupConcatSeparator != null) {
-            groupConcatSeparator.mapColumns(resolver, level, innerState);
-        }
         super.mapColumnsAnalysis(resolver, level, innerState);
     }
 
     @Override
     public Expression optimize(Session session) {
         super.optimize(session);
-        if (on != null) {
-            on = on.optimize(session);
-            type = on.getType();
+        if (args.length == 1) {
+            type = args[0].getType();
         }
         if (orderByList != null) {
             for (SelectOrderBy o : orderByList) {
@@ -555,9 +534,6 @@ public class Aggregate extends AbstractAggregate {
             }
             orderBySort = createOrder(session, orderByList, 1);
         }
-        if (groupConcatSeparator != null) {
-            groupConcatSeparator = groupConcatSeparator.optimize(session);
-        }
         switch (aggregateType) {
         case GROUP_CONCAT:
             type = TypeInfo.TYPE_STRING_DEFAULT;
@@ -642,17 +618,11 @@ public class Aggregate extends AbstractAggregate {
 
     @Override
     public void setEvaluatable(TableFilter tableFilter, boolean b) {
-        if (on != null) {
-            on.setEvaluatable(tableFilter, b);
-        }
         if (orderByList != null) {
             for (SelectOrderBy o : orderByList) {
                 o.expression.setEvaluatable(tableFilter, b);
             }
         }
-        if (groupConcatSeparator != null) {
-            groupConcatSeparator.setEvaluatable(tableFilter, b);
-        }
         super.setEvaluatable(tableFilter, b);
     }
 
@@ -661,11 +631,11 @@ public class Aggregate extends AbstractAggregate {
         if (distinct) {
             builder.append("DISTINCT ");
         }
-        on.getSQL(builder);
+        args[0].getSQL(builder);
         Window.appendOrderBy(builder, orderByList);
-        if (groupConcatSeparator != null) {
+        if (args.length >= 2) {
             builder.append(" SEPARATOR ");
-            groupConcatSeparator.getSQL(builder);
+            args[1].getSQL(builder);
         }
         builder.append(')');
         return appendTailConditions(builder);
@@ -676,7 +646,7 @@ public class Aggregate extends AbstractAggregate {
         if (distinct) {
             builder.append("DISTINCT ");
         }
-        on.getSQL(builder);
+        args[0].getSQL(builder);
         Window.appendOrderBy(builder, orderByList);
         builder.append(')');
         return appendTailConditions(builder);
@@ -758,14 +728,14 @@ public class Aggregate extends AbstractAggregate {
         builder.append(text);
         if (distinct) {
             builder.append("(DISTINCT ");
-            on.getSQL(builder).append(')');
+            args[0].getSQL(builder).append(')');
         } else {
             builder.append('(');
-            if (on != null) {
-                if (on instanceof Subquery) {
-                    on.getSQL(builder);
+            for (Expression arg : args) {
+                if (arg instanceof Subquery) {
+                    arg.getSQL(builder);
                 } else {
-                    on.getUnenclosedSQL(builder);
+                    arg.getUnenclosedSQL(builder);
                 }
             }
             builder.append(')');
@@ -779,8 +749,9 @@ public class Aggregate extends AbstractAggregate {
     }
 
     private Index getMinMaxColumnIndex() {
-        if (on instanceof ExpressionColumn) {
-            ExpressionColumn col = (ExpressionColumn) on;
+        Expression arg = args[0];
+        if (arg instanceof ExpressionColumn) {
+            ExpressionColumn col = (ExpressionColumn) arg;
             Column column = col.getColumn();
             TableFilter filter = col.getTableFilter();
             if (filter != null) {
@@ -802,7 +773,7 @@ public class Aggregate extends AbstractAggregate {
         if (visitor.getType() == ExpressionVisitor.OPTIMIZABLE_AGGREGATE) {
             switch (aggregateType) {
             case COUNT:
-                if (!distinct && on.getNullable() == Column.NOT_NULLABLE) {
+                if (!distinct && args[0].getNullable() == Column.NOT_NULLABLE) {
                     return visitor.getTable().canGetRowCount();
                 }
                 return false;
@@ -814,23 +785,22 @@ public class Aggregate extends AbstractAggregate {
                 return index != null;
             case PERCENTILE_CONT:
             case PERCENTILE_DISC:
-                return on.isConstant() && Percentile.getColumnIndex(orderByList.get(0).expression) != null;
+                return args[0].isConstant() && Percentile.getColumnIndex(orderByList.get(0).expression) != null;
             case MEDIAN:
                 if (distinct) {
                     return false;
                 }
-                return Percentile.getColumnIndex(on) != null;
+                return Percentile.getColumnIndex(args[0]) != null;
             case ENVELOPE:
-                return AggregateDataEnvelope.getGeometryColumnIndex(on) != null;
+                return AggregateDataEnvelope.getGeometryColumnIndex(args[0]) != null;
             default:
                 return false;
             }
         }
-        if (on != null && !on.isEverything(visitor)) {
-            return false;
-        }
-        if (groupConcatSeparator != null && !groupConcatSeparator.isEverything(visitor)) {
-            return false;
+        for (Expression arg : args) {
+            if (!arg.isEverything(visitor)) {
+                return false;
+            }
         }
         if (orderByList != null) {
             for (SelectOrderBy o : orderByList) {
@@ -845,8 +815,13 @@ public class Aggregate extends AbstractAggregate {
     @Override
     public int getCost() {
         int cost = 1;
-        if (on != null) {
-            cost += on.getCost();
+        for (Expression arg : args) {
+            cost += arg.getCost();
+        }
+        if (orderByList != null) {
+            for (SelectOrderBy o : orderByList) {
+                cost += o.expression.getCost();
+            }
         }
         if (filterCondition != null) {
             cost += filterCondition.getCost();

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

@@ -15,8 +15,6 @@ import org.h2.engine.UserAggregate;
 import org.h2.expression.Expression;
 import org.h2.expression.ExpressionVisitor;
 import org.h2.message.DbException;
-import org.h2.table.ColumnResolver;
-import org.h2.table.TableFilter;
 import org.h2.value.DataType;
 import org.h2.value.TypeInfo;
 import org.h2.value.Value;
@@ -30,16 +28,13 @@ import org.h2.value.ValueRow;
 public class JavaAggregate extends AbstractAggregate {
 
     private final UserAggregate userAggregate;
-    private final Expression[] args;
     private int[] argTypes;
-    private TypeInfo type;
     private int dataType;
     private Connection userConnection;
 
     public JavaAggregate(UserAggregate userAggregate, Expression[] args, Select select, boolean distinct) {
-        super(select, distinct);
+        super(select, args, distinct);
         this.userAggregate = userAggregate;
-        this.args = args;
     }
 
     @Override
@@ -62,11 +57,6 @@ public class JavaAggregate extends AbstractAggregate {
         return appendTailConditions(builder);
     }
 
-    @Override
-    public TypeInfo getType() {
-        return type;
-    }
-
     @Override
     public boolean isEverything(ExpressionVisitor visitor) {
         if (!super.isEverything(visitor)) {
@@ -92,14 +82,6 @@ public class JavaAggregate extends AbstractAggregate {
         return filterCondition == null || filterCondition.isEverything(visitor);
     }
 
-    @Override
-    public void mapColumnsAnalysis(ColumnResolver resolver, int level, int innerState) {
-        for (Expression arg : args) {
-            arg.mapColumns(resolver, level, innerState);
-        }
-        super.mapColumnsAnalysis(resolver, level, innerState);
-    }
-
     @Override
     public Expression optimize(Session session) {
         super.optimize(session);
@@ -107,9 +89,7 @@ public class JavaAggregate extends AbstractAggregate {
         int len = args.length;
         argTypes = new int[len];
         for (int i = 0; i < len; i++) {
-            Expression expr = args[i];
-            args[i] = expr.optimize(session);
-            int type = expr.getType().getValueType();
+            int type = args[i].getType().getValueType();
             argTypes[i] = type;
         }
         try {
@@ -122,14 +102,6 @@ public class JavaAggregate extends AbstractAggregate {
         return this;
     }
 
-    @Override
-    public void setEvaluatable(TableFilter tableFilter, boolean b) {
-        for (Expression e : args) {
-            e.setEvaluatable(tableFilter, b);
-        }
-        super.setEvaluatable(tableFilter, b);
-    }
-
     private Aggregate getInstance() {
         Aggregate agg = userAggregate.getInstance();
         try {

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

@@ -67,3 +67,32 @@ select group_concat(distinct v order by v desc) from test;
 
 drop table test;
 > ok
+
+create table test(g varchar, v int) as values ('-', 1), ('-', 2), ('-', 3), ('|', 4), ('|', 5), ('|', 6), ('*', null);
+> ok
+
+select g, group_concat(v separator g) from test group by g;
+> G GROUP_CONCAT(V SEPARATOR G)
+> - ---------------------------
+> * null
+> - 1-2-3
+> | 4|5|6
+> rows: 3
+
+select g, group_concat(v separator g) over (partition by g) from test order by v;
+> G GROUP_CONCAT(V SEPARATOR G) OVER (PARTITION BY G)
+> - -------------------------------------------------
+> * null
+> - 1-2-3
+> - 1-2-3
+> - 1-2-3
+> | 4|5|6
+> | 4|5|6
+> | 4|5|6
+> rows (ordered): 7
+
+select g, group_concat(v separator v) from test group by g;
+> exception INVALID_VALUE_2
+
+drop table test;
+> ok