branch-3.1: [opt](group_concat) allow args be types other than string #52805

fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/GroupConcat.java

@@ -23,7 +23,6 @@
 import org.apache.doris.nereids.trees.expressions.OrderExpression;
 import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
 import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
-import org.apache.doris.nereids.types.DataType;
 import org.apache.doris.nereids.types.VarcharType;
 import org.apache.doris.nereids.types.coercion.AnyDataType;
 import org.apache.doris.nereids.util.ExpressionUtils;
@@ -39,10 +38,18 @@
 public class GroupConcat extends NullableAggregateFunction
         implements ExplicitlyCastableSignature, SupportMultiDistinct {
 
-    public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
-            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(VarcharType.SYSTEM_DEFAULT),
+    private static final List<FunctionSignature> ONE_ARG = ImmutableList.of(
+            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(VarcharType.SYSTEM_DEFAULT)
+    );
+    private static final List<FunctionSignature> ONE_ARG_WITH_ORDER_BY = ImmutableList.of(
             FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT)
-                    .varArgs(VarcharType.SYSTEM_DEFAULT, AnyDataType.INSTANCE_WITHOUT_INDEX),
+                    .varArgs(VarcharType.SYSTEM_DEFAULT, AnyDataType.INSTANCE_WITHOUT_INDEX)
+    );
+    private static final List<FunctionSignature> TWO_ARGS = ImmutableList.of(
+            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT)
+                    .args(VarcharType.SYSTEM_DEFAULT, VarcharType.SYSTEM_DEFAULT)
+    );
+    private static final List<FunctionSignature> TWO_ARGS_WITH_ORDER_BY = ImmutableList.of(
             FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT)
                     .varArgs(VarcharType.SYSTEM_DEFAULT, VarcharType.SYSTEM_DEFAULT, AnyDataType.INSTANCE_WITHOUT_INDEX)
     );
@@ -93,23 +100,6 @@ public List<Expression> getDistinctArguments() {
         }
     }
 
-    @Override
-    public void checkLegalityBeforeTypeCoercion() {
-        DataType typeOrArg0 = getArgumentType(0);
-        if (!typeOrArg0.isStringLikeType() && !typeOrArg0.isNullType()) {
-            throw new AnalysisException(
-                    "group_concat requires first parameter to be of type STRING: " + this.toSql());
-        }
-
-        if (nonOrderArguments == 2) {
-            DataType typeOrArg1 = getArgumentType(1);
-            if (!typeOrArg1.isStringLikeType() && !typeOrArg1.isNullType()) {
-                throw new AnalysisException(
-                        "group_concat requires second parameter to be of type STRING: " + this.toSql());
-            }
-        }
-    }
-
     @Override
     public GroupConcat withAlwaysNullable(boolean alwaysNullable) {
         return new GroupConcat(distinct, alwaysNullable, children);
@@ -130,7 +120,17 @@ public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {
 
     @Override
     public List<FunctionSignature> getSignatures() {
-        return SIGNATURES;
+        if (nonOrderArguments == 2) {
+            if (arity() >= 3) {
+                return TWO_ARGS_WITH_ORDER_BY;
+            }
+            return TWO_ARGS;
+        } else {
+            if (arity() >= 2) {
+                return ONE_ARG_WITH_ORDER_BY;
+            }
+            return ONE_ARG;
+        }
     }
 
     @Override

fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/MultiDistinctGroupConcat.java

@@ -23,8 +23,6 @@
 import org.apache.doris.nereids.trees.expressions.OrderExpression;
 import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
 import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
-import org.apache.doris.nereids.types.CharType;
-import org.apache.doris.nereids.types.StringType;
 import org.apache.doris.nereids.types.VarcharType;
 import org.apache.doris.nereids.types.coercion.AnyDataType;
 import org.apache.doris.nereids.util.ExpressionUtils;
@@ -37,26 +35,24 @@
 /** MultiDistinctGroupConcat */
 public class MultiDistinctGroupConcat extends NullableAggregateFunction
         implements ExplicitlyCastableSignature, MultiDistinction {
-    public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
-            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(VarcharType.SYSTEM_DEFAULT),
-            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).varArgs(VarcharType.SYSTEM_DEFAULT,
-                    AnyDataType.INSTANCE_WITHOUT_INDEX),
-            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).varArgs(VarcharType.SYSTEM_DEFAULT,
-                    VarcharType.SYSTEM_DEFAULT, AnyDataType.INSTANCE_WITHOUT_INDEX),
-
-            FunctionSignature.ret(StringType.INSTANCE).args(StringType.INSTANCE),
-            FunctionSignature.ret(StringType.INSTANCE).varArgs(StringType.INSTANCE,
-                    AnyDataType.INSTANCE_WITHOUT_INDEX),
-            FunctionSignature.ret(StringType.INSTANCE).varArgs(StringType.INSTANCE,
-                    StringType.INSTANCE, AnyDataType.INSTANCE_WITHOUT_INDEX),
-
-            FunctionSignature.ret(CharType.SYSTEM_DEFAULT).args(CharType.SYSTEM_DEFAULT),
-            FunctionSignature.ret(CharType.SYSTEM_DEFAULT).varArgs(CharType.SYSTEM_DEFAULT,
-                    AnyDataType.INSTANCE_WITHOUT_INDEX),
-            FunctionSignature.ret(CharType.SYSTEM_DEFAULT).varArgs(CharType.SYSTEM_DEFAULT,
-                    CharType.SYSTEM_DEFAULT, AnyDataType.INSTANCE_WITHOUT_INDEX));
+    private static final List<FunctionSignature> ONE_ARG = ImmutableList.of(
+            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT).args(VarcharType.SYSTEM_DEFAULT)
+    );
+    private static final List<FunctionSignature> ONE_ARG_WITH_ORDER_BY = ImmutableList.of(
+            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT)
+                    .varArgs(VarcharType.SYSTEM_DEFAULT, AnyDataType.INSTANCE_WITHOUT_INDEX)
+    );
+    private static final List<FunctionSignature> TWO_ARGS = ImmutableList.of(
+            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT)
+                    .args(VarcharType.SYSTEM_DEFAULT, VarcharType.SYSTEM_DEFAULT)
+    );
+    private static final List<FunctionSignature> TWO_ARGS_WITH_ORDER_BY = ImmutableList.of(
+            FunctionSignature.ret(VarcharType.SYSTEM_DEFAULT)
+                    .varArgs(VarcharType.SYSTEM_DEFAULT, VarcharType.SYSTEM_DEFAULT, AnyDataType.INSTANCE_WITHOUT_INDEX)
+    );
 
     private final boolean mustUseMultiDistinctAgg;
+    private final int nonOrderArguments;
 
     /**
      * constructor with 1 argument with other arguments.
@@ -79,8 +75,8 @@ private MultiDistinctGroupConcat(boolean alwaysNullable, Expression arg,
 
     private MultiDistinctGroupConcat(boolean mustUseMultiDistinctAgg, boolean alwaysNullable, List<Expression> args) {
         super("multi_distinct_group_concat", false, alwaysNullable, args);
-        checkArguments(children);
         this.mustUseMultiDistinctAgg = mustUseMultiDistinctAgg;
+        this.nonOrderArguments = findOrderExprIndex(children);
     }
 
     @Override
@@ -99,7 +95,6 @@ public MultiDistinctGroupConcat withAlwaysNullable(boolean alwaysNullable) {
      */
     @Override
     public MultiDistinctGroupConcat withDistinctAndChildren(boolean distinct, List<Expression> children) {
-        checkArguments(children);
         return new MultiDistinctGroupConcat(mustUseMultiDistinctAgg, alwaysNullable, children);
     }
 
@@ -110,10 +105,30 @@ public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {
 
     @Override
     public List<FunctionSignature> getSignatures() {
-        return SIGNATURES;
+        if (nonOrderArguments == 2) {
+            if (arity() >= 3) {
+                return TWO_ARGS_WITH_ORDER_BY;
+            }
+            return TWO_ARGS;
+        } else {
+            if (arity() >= 2) {
+                return ONE_ARG_WITH_ORDER_BY;
+            }
+            return ONE_ARG;
+        }
+    }
+
+    @Override
+    public boolean mustUseMultiDistinctAgg() {
+        return mustUseMultiDistinctAgg || children.stream().anyMatch(OrderExpression.class::isInstance);
     }
 
-    private void checkArguments(List<Expression> children) {
+    @Override
+    public Expression withMustUseMultiDistinctAgg(boolean mustUseMultiDistinctAgg) {
+        return new MultiDistinctGroupConcat(mustUseMultiDistinctAgg, alwaysNullable, children);
+    }
+
+    private int findOrderExprIndex(List<Expression> children) {
         Preconditions.checkArgument(children().size() >= 1, "children's size should >= 1");
         boolean foundOrderExpr = false;
         int firstOrderExrIndex = 0;
@@ -133,15 +148,6 @@ private void checkArguments(List<Expression> children) {
             throw new AnalysisException(
                     "multi_distinct_group_concat requires one or two parameters: " + children);
         }
-    }
-
-    @Override
-    public boolean mustUseMultiDistinctAgg() {
-        return mustUseMultiDistinctAgg || children.stream().anyMatch(OrderExpression.class::isInstance);
-    }
-
-    @Override
-    public Expression withMustUseMultiDistinctAgg(boolean mustUseMultiDistinctAgg) {
-        return new MultiDistinctGroupConcat(mustUseMultiDistinctAgg, alwaysNullable, children);
+        return firstOrderExrIndex;
     }
 }

regression-test/data/query_p0/group_concat/test_group_concat.out

@@ -39,6 +39,10 @@ false
 1	2
 1	2
 
+-- !select_12 --
+1	2
+1	2
+
 -- !select_13 --
 1	2
 1	2
@@ -68,7 +72,7 @@ false
 2	23,222,22,211,21
 
 -- !select_group_concat_order_by1 --
-1,11,2,21,21,211,22,222,23,3	3,23,222,22,211,21,21,2,11,1
+1,11,2,21,21,211,22,222,23,3	3223222222222112212212221121
 
 -- !select_group_concat_order_by2 --
 1,11,2,21,21,211,22,222,23,3	3,23,222,22,211,21,21,2,11,1

regression-test/suites/query_p0/group_concat/test_group_concat.groovy

@@ -72,7 +72,16 @@ suite("test_group_concat", "query,p0,arrow_flight_sql") {
 
     qt_select_12 """
                 select
-                group_concat( distinct b1, '?'), group_concat( distinct b3, '?')
+                group_concat( distinct b1, 123), group_concat( distinct b3, '?')
+                from
+                table_group_concat
+                group by 
+                b2;
+              """
+
+    qt_select_12 """
+                select
+                multi_distinct_group_concat(b1, 123), multi_distinct_group_concat(b3, '?')
                 from
                 table_group_concat
                 group by 
@@ -109,7 +118,7 @@ suite("test_group_concat", "query,p0,arrow_flight_sql") {
       select * from table_group_concat order by b1, b2, b3;
     """
     qt_select_group_concat_order_by_desc1 """
-                SELECT b1, group_concat(cast(abs(b2) as varchar) order by abs(b2) desc) FROM table_group_concat  group by b1 order by b1
+                SELECT b1, group_concat(abs(b2) order by abs(b2) desc) FROM table_group_concat  group by b1 order by b1
               """
 
     qt_select_group_concat_order_by_desc2 """
@@ -119,12 +128,13 @@ suite("test_group_concat", "query,p0,arrow_flight_sql") {
                 SELECT b1, group_concat(cast(abs(b3) as varchar) order by abs(b2) desc, b3 desc) FROM table_group_concat  group by b1 order by b1
               """
     qt_select_group_concat_order_by1 """
-                select group_concat(b3,',' order by b3 asc),group_concat(b3,',' order by b3 desc) from table_group_concat;
+                select group_concat(b3,',' order by b3 asc),group_concat(b3,'2' order by b3 desc) from table_group_concat;
     """
 
     sql """create view if not exists test_view as select group_concat(b3,',' order by b3 asc),group_concat(b3,',' order by b3 desc) from table_group_concat;"""
     qt_select_group_concat_order_by2 """
                 select * from test_view;
     """
+
     sql """drop view if exists test_view"""
 }