-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Support GroupConcat sql for aggregating multiple shards(#33797) #33808
Conversation
@strongduanmu PTAL. I don't think my changes will affect the "E2E - Operation / E2E - pipeline on postgres:12-alpine (pull_request) ", |
Hi @YaoFly, can you add e2e test in https://github.com/apache/shardingsphere/blob/master/test/e2e/sql/src/test/resources/cases/dql/e2e-dql-select-aggregate.xml Another question is, besides MySQL database, are there any other databases that support GROUP_CONCAT? If so, please add them to the E2E Case. |
ok, I will |
e2e test added. besides MySQL database, other database required additional code development, which I suggest supporting them in other pull requests? |
OK, you are welcome to submit more PRs to support other database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @YaoFly, thanks for everything you did on this PR, it still has some minor issues that need your help.
...org/apache/shardingsphere/sharding/merge/dql/groupby/aggregation/AggregationUnitFactory.java
Outdated
Show resolved
Hide resolved
...hardingsphere/sharding/merge/dql/groupby/aggregation/DistinctGroupConcatAggregationUnit.java
Outdated
Show resolved
Hide resolved
...hardingsphere/sharding/merge/dql/groupby/aggregation/DistinctGroupConcatAggregationUnit.java
Outdated
Show resolved
Hide resolved
...hardingsphere/sharding/merge/dql/groupby/aggregation/DistinctGroupConcatAggregationUnit.java
Outdated
Show resolved
Hide resolved
...hardingsphere/sharding/merge/dql/groupby/aggregation/DistinctGroupConcatAggregationUnit.java
Outdated
Show resolved
Hide resolved
...apache/shardingsphere/sharding/merge/dql/groupby/aggregation/GroupConcatAggregationUnit.java
Outdated
Show resolved
Hide resolved
...apache/shardingsphere/sharding/merge/dql/groupby/aggregation/GroupConcatAggregationUnit.java
Outdated
Show resolved
Hide resolved
...apache/shardingsphere/sharding/merge/dql/groupby/aggregation/AggregationUnitFactoryTest.java
Show resolved
Hide resolved
...java/org/apache/shardingsphere/sql/parser/mysql/visitor/statement/MySQLStatementVisitor.java
Outdated
Show resolved
Hide resolved
@YaoFly In addition, you need to update this PR to the RELEASE-NOTE of 5.5.2-SNAPSHOT. |
@YaoFly From this PR, we can see that you are familiar with ShardingSphere. If you want to further participate in the development, you can send me your WeChat and I may be able to provide you with more help. |
wx id : yaoflycn |
...hardingsphere/sharding/merge/dql/groupby/aggregation/DistinctGroupConcatAggregationUnit.java
Show resolved
Hide resolved
...he/shardingsphere/sharding/merge/dql/groupby/aggregation/GroupConcatAggregationUnitTest.java
Show resolved
Hide resolved
...phere/infra/binder/context/segment/select/projection/impl/AggregationDistinctProjection.java
Outdated
Show resolved
Hide resolved
...hardingsphere/infra/binder/context/segment/select/projection/impl/AggregationProjection.java
Show resolved
Hide resolved
.../shardingsphere/sql/parser/statement/core/segment/dml/item/AggregationProjectionSegment.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks so great, merged.
Fixes #33797.
Changes proposed in this pull request:
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e
.