code optimization (#33797)

features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/aggregation/AggregationUnitFactory.java

@@ -28,18 +28,6 @@
 @NoArgsConstructor(access = AccessLevel.PRIVATE)
 public final class AggregationUnitFactory {
 
-    /**
-     * Create aggregation unit instance.
-     *
-     * @param type aggregation function type
-     * @param isDistinct is distinct
-     * @return aggregation unit instance
-     * @throws UnsupportedSQLOperationException unsupported SQL operation exception
-     */
-    public static AggregationUnit create(final AggregationType type, final boolean isDistinct) {
-        return create(type, isDistinct, null);
-    }
-
     /**
      * Create aggregation unit instance.
      *

features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/aggregation/DistinctGroupConcatAggregationUnit.java

@@ -20,33 +20,32 @@
 import java.util.Collection;
 import java.util.LinkedHashSet;
 import java.util.List;
-import java.util.stream.Collectors;
 
-public class DistinctGroupConcatAggregationUnit implements AggregationUnit {
+/**
+ *  Distinct group concat aggregation unit.
+ */
+public final class DistinctGroupConcatAggregationUnit implements AggregationUnit {
 
     private static final String DEFAULT_SEPARATOR = ",";
 
-    private final Collection<Comparable<?>> values = new LinkedHashSet<>();
+    private final Collection<String> values = new LinkedHashSet<>();
 
-    private String separator;
+    private final String separator;
 
     public DistinctGroupConcatAggregationUnit(final String separator) {
-        this.separator = separator;
+        this.separator = null == separator ? DEFAULT_SEPARATOR : separator;
     }
 
     @Override
     public void merge(final List<Comparable<?>> values) {
         if (null == values || null == values.get(0)) {
             return;
         }
-        this.values.add(values.get(0));
+        this.values.add(String.valueOf(values.get(0)));
     }
 
     @Override
     public Comparable<?> getResult() {
-        if (null == separator) {
-            separator = DEFAULT_SEPARATOR;
-        }
-        return values.stream().map(Object::toString).collect(Collectors.joining(separator));
+        return String.join(separator, values);
     }
 }

features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/aggregation/GroupConcatAggregationUnit.java

@@ -17,39 +17,35 @@
 
 package org.apache.shardingsphere.sharding.merge.dql.groupby.aggregation;
 
-import lombok.NoArgsConstructor;
-
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
-import java.util.stream.Collectors;
 
-@NoArgsConstructor
-public class GroupConcatAggregationUnit implements AggregationUnit {
+/**
+ * Group concat aggregation unit.
+ */
+public final class GroupConcatAggregationUnit implements AggregationUnit {
 
     private static final String DEFAULT_SEPARATOR = ",";
 
-    private final Collection<Comparable<?>> values = new ArrayList<>();
+    private final Collection<String> values = new ArrayList<>();
 
-    private String separator;
+    private final String separator;
 
     public GroupConcatAggregationUnit(final String separator) {
-        this.separator = separator;
+        this.separator = null == separator ? DEFAULT_SEPARATOR : separator;
     }
 
     @Override
     public void merge(final List<Comparable<?>> values) {
         if (null == values || null == values.get(0)) {
             return;
         }
-        this.values.add(values.get(0));
+        this.values.add(String.valueOf(values.get(0)));
     }
 
     @Override
     public Comparable<?> getResult() {
-        if (null == separator) {
-            separator = DEFAULT_SEPARATOR;
-        }
-        return values.stream().map(Object::toString).collect(Collectors.joining(separator));
+        return String.join(separator, values);
     }
 }

features/sharding/core/src/test/java/org/apache/shardingsphere/sharding/merge/dql/groupby/aggregation/AggregationUnitFactoryTest.java

@@ -27,44 +27,50 @@ class AggregationUnitFactoryTest {
 
     @Test
     void assertCreateComparableAggregationUnit() {
-        assertThat(AggregationUnitFactory.create(AggregationType.MIN, false), instanceOf(ComparableAggregationUnit.class));
-        assertThat(AggregationUnitFactory.create(AggregationType.MAX, false), instanceOf(ComparableAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.MIN, false, null), instanceOf(ComparableAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.MAX, false, null), instanceOf(ComparableAggregationUnit.class));
     }
 
     @Test
     void assertCreateAccumulationAggregationUnit() {
-        assertThat(AggregationUnitFactory.create(AggregationType.SUM, false), instanceOf(AccumulationAggregationUnit.class));
-        assertThat(AggregationUnitFactory.create(AggregationType.COUNT, false), instanceOf(AccumulationAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.SUM, false, null), instanceOf(AccumulationAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.COUNT, false, null), instanceOf(AccumulationAggregationUnit.class));
     }
 
     @Test
     void assertCreateAverageAggregationUnit() {
-        assertThat(AggregationUnitFactory.create(AggregationType.AVG, false), instanceOf(AverageAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.AVG, false, null), instanceOf(AverageAggregationUnit.class));
     }
 
     @Test
     void assertCreateDistinctSumAggregationUnit() {
-        assertThat(AggregationUnitFactory.create(AggregationType.SUM, true), instanceOf(DistinctSumAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.SUM, true, null), instanceOf(DistinctSumAggregationUnit.class));
     }
 
     @Test
     void assertCreateDistinctCountAggregationUnit() {
-        assertThat(AggregationUnitFactory.create(AggregationType.COUNT, true), instanceOf(DistinctCountAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.COUNT, true, null), instanceOf(DistinctCountAggregationUnit.class));
     }
 
     @Test
     void assertCreateDistinctAverageAggregationUnit() {
-        assertThat(AggregationUnitFactory.create(AggregationType.AVG, true), instanceOf(DistinctAverageAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.AVG, true, null), instanceOf(DistinctAverageAggregationUnit.class));
     }
 
     @Test
     void assertCreateBitXorAggregationUnit() {
-        assertThat(AggregationUnitFactory.create(AggregationType.BIT_XOR, false), instanceOf(BitXorAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.BIT_XOR, false, null), instanceOf(BitXorAggregationUnit.class));
     }
 
     @Test
     void assertGroupConcatAggregationUnit() {
-        assertThat(AggregationUnitFactory.create(AggregationType.GROUP_CONCAT, true), instanceOf(DistinctGroupConcatAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.GROUP_CONCAT, false, null), instanceOf(GroupConcatAggregationUnit.class));
+        assertThat(AggregationUnitFactory.create(AggregationType.GROUP_CONCAT, false, " "), instanceOf(GroupConcatAggregationUnit.class));
+    }
+
+    @Test
+    void assertDistinctGroupConcatAggregationUnit() {
+        assertThat(AggregationUnitFactory.create(AggregationType.GROUP_CONCAT, true, null), instanceOf(DistinctGroupConcatAggregationUnit.class));
         assertThat(AggregationUnitFactory.create(AggregationType.GROUP_CONCAT, true, " "), instanceOf(DistinctGroupConcatAggregationUnit.class));
     }
 }

parser/sql/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/statement/MySQLStatementVisitor.java

@@ -25,7 +25,7 @@
 import org.antlr.v4.runtime.tree.TerminalNode;
 import org.apache.shardingsphere.sql.parser.api.ASTNode;
 import org.apache.shardingsphere.sql.parser.autogen.MySQLStatementBaseVisitor;
-import org.apache.shardingsphere.sql.parser.autogen.MySQLStatementParser;
+import org.apache.shardingsphere.sql.parser.autogen.MySQLStatementParser.SeparatorNameContext;
 import org.apache.shardingsphere.sql.parser.autogen.MySQLStatementParser.AggregationFunctionContext;
 import org.apache.shardingsphere.sql.parser.autogen.MySQLStatementParser.AliasContext;
 import org.apache.shardingsphere.sql.parser.autogen.MySQLStatementParser.AssignmentContext;
@@ -918,7 +918,7 @@ public final ASTNode visitAggregationFunction(final AggregationFunctionContext c
     }
 
     @Override
-    public ASTNode visitSeparatorName(final MySQLStatementParser.SeparatorNameContext ctx) {
+    public ASTNode visitSeparatorName(final SeparatorNameContext ctx) {
         return new StringLiteralValue(ctx.string_().getText());
     }
 

test/it/parser/src/main/resources/case/dml/select-aggregate.xml

@@ -518,4 +518,28 @@
             <index-item index="1" start-index="199" stop-index="199"/>
         </order-by>
     </select>
+    <select sql-case-id="select_group_concat">
+        <projections start-index="7" stop-index="51">
+            <aggregation-projection text="GROUP_CONCAT(user_id)" start-index="7" stop-index="27">
+                <parameter>
+                    <column name="user_id" start-index="20" stop-index="26" />
+                </parameter>
+            </aggregation-projection>
+        </projections>
+        <from>
+            <simple-table start-index="58" stop-index="64"/>
+        </from>
+    </select>
+    <select sql-case-id="select_group_concat_with_distinct_with_separator">
+        <projections start-index="7" stop-index="74">
+            <aggregation-projection text="GROUP_CONCAT(distinct user_id SEPARATOR ' ')" start-index="7" stop-index="50">
+                <parameter>
+                    <column name="user_id" start-index="29" stop-index="35" />
+                </parameter>
+            </aggregation-projection>
+        </projections>
+        <from>
+            <simple-table start-index="81" stop-index="87"/>
+        </from>
+    </select>
 </sql-parser-test-cases>

test/it/parser/src/main/resources/sql/supported/dml/select-aggregate.xml

@@ -38,4 +38,6 @@
     <sql-case id="select_approx_count"
               value="select owner, approx_count(*) , approx_rank(partition by owner order by approx_count(*) desc) from t group by owner having approx_rank(partition by owner order by approx_count(*) desc) &lt;= 1 order by 1"
               db-types="Oracle"/>
+    <sql-case id="select_group_concat" value="SELECT GROUP_CONCAT(user_id) AS user_id_group_concat FROM t_order" db-types="MySQL"/>
+    <sql-case id="select_group_concat_with_distinct_with_separator" value="SELECT GROUP_CONCAT(distinct user_id SEPARATOR ' ') AS user_id_group_concat FROM t_order" db-types="MySQL"/>
 </sql-cases>