You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "Malaydewangan09 (via GitHub)" <gi...@apache.org> on 2024/10/23 20:01:25 UTC
[PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Malaydewangan09 opened a new pull request, #33380:
URL: https://github.com/apache/shardingsphere/pull/33380
Fixes #33219
Changes proposed in this pull request:
-
This pull request addresses issue #33219, which reports that executing a SQL query with COUNT(1) and a GROUP BY clause returns one row with a count of zero when no data exists in the table. The fix ensures that no rows are generated in such cases, aligning the behavior with expected SQL standards.
###
- Modified the logic in GroupByMemoryMergedResult to prevent the generation of a result row when there is no data available.
###
---
Before committing this PR, I'm sure that I have checked the following options:
- [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [ ] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [ ] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [ ] I have made corresponding changes to the documentation.
- [ ] I have added corresponding unit tests for my changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2443186756
Hi @Malaydewangan09, i have added e2e test for #4680 in #33449, you can merge master branch, and test the only one case which related to your issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2434779251
@strongduanmu
I have included tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2445750300
@strongduanmu
I guess the changes made are no longer working for the current issue e2e case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2446323368
Yes I'll look into this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2434347981
Ah, got it
Thanks @strongduanmu
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2440037662
Cool, Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on code in PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#discussion_r1817995465
##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/merge/dql/groupby/GroupByMemoryMergedResult.java:
##########
@@ -143,22 +138,10 @@ private boolean getValueCaseSensitiveFromTables(final QueryResult queryResult,
private List<MemoryQueryResultRow> getMemoryResultSetRows(final SelectStatementContext selectStatementContext,
final Map<GroupByValue, MemoryQueryResultRow> dataMap, final List<Boolean> valueCaseSensitive) {
if (dataMap.isEmpty()) {
- Object[] data = generateReturnData(selectStatementContext);
- return Arrays.stream(data).anyMatch(Objects::nonNull) ? Collections.singletonList(new MemoryQueryResultRow(data)) : Collections.emptyList();
+ return Collections.emptyList();
}
List<MemoryQueryResultRow> result = new ArrayList<>(dataMap.values());
result.sort(new GroupByRowComparator(selectStatementContext, valueCaseSensitive));
return result;
}
-
- private Object[] generateReturnData(final SelectStatementContext selectStatementContext) {
Review Comment:
Hi @Malaydewangan09, can you add a e2e test case for this issue - https://github.com/apache/shardingsphere/issues/4680
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2440112080
@strongduanmu
Please check this once
I have added e2e tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2437180708
Hey @strongduanmu
Can you check this if these failing checks are from my side or any changes are required ?
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu merged PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2443228810
Okay I have resolved the conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2439559883
@strongduanmu I have updated the unit tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2440008045
Okay @strongduanmu
As I understand the issue #4680 was
https://github.com/apache/shardingsphere/issues/4680#issuecomment-645992089 all these clauses were having problem
So can I write one e2e case for any one of these ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2443065275
@strongduanmu can you help me with this?
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2439812487
> @strongduanmu I have updated the unit tests.
Good job, let's wait ci result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2437185222
> Hey @strongduanmu Can you check this if these failing checks are from my side or any changes are required ? Thanks!
You can take a look for https://github.com/apache/shardingsphere/actions/runs/11511684449/job/32047873796?pr=33380, and fix the unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2440005489
> @strongduanmu Thanks! To add an e2e test for the issue #4680 Is there an issue open to add an e2e test? Or should I create a new PR for this change?
Since you deleted the code added in that issue, you need to test whether those cases will cause problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2448914545
Thanks @strongduanmu for constant feedback!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2440009438
> Okay @strongduanmu As I understand the issue #4680 was [#4680 (comment)](https://github.com/apache/shardingsphere/issues/4680#issuecomment-645992089) all these clauses were having problem So can I write one e2e case for any one of these ?
It is best to add all E2E Cases to ensure the reliability of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2443211867
@strongduanmu so the code which I corrected earlier was incorrect?
```
private List<MemoryQueryResultRow> getMemoryResultSetRows(final SelectStatementContext selectStatementContext,
final Map<GroupByValue, MemoryQueryResultRow> dataMap, final List<Boolean> valueCaseSensitive) {
if (dataMap.isEmpty()) {
return Collections.emptyList();
}
List<MemoryQueryResultRow> result = new ArrayList<>(dataMap.values());
result.sort(new GroupByRowComparator(selectStatementContext, valueCaseSensitive));
return result;
}
```
Since the all test cases are similar to what i wrote.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2445764213
> @strongduanmu I guess the changes made are no longer working for the current issue e2e case?我猜所做的更改不再适用于当前问题的 e2e 案例？ 
Yes, this modification does not seem to make sense, you need to re-investigate the solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2439902518
@strongduanmu Thanks!
To add an e2e for the issue #4680
Is there an issue open for this ?
Or i can just add and push the changes ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2434268453
@strongduanmu I have included unit tests for my change locally.
Can you help with e2e test?
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2434308444
Hi @Malaydewangan09, you could add `SELECT COUNT(1) ... GROUP BY` test case in https://github.com/apache/shardingsphere/blob/master/test/e2e/sql/src/test/resources/cases/dql/e2e-dql-select-group-by.xml file, and config this case with `db-types="MySQL,PostgreSQL,openGauss" scenario-types="db,tbl`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "Malaydewangan09 (via GitHub)" <gi...@apache.org>.
Malaydewangan09 commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2441966501
Hey @strongduanmu
Can you help me with this ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: SQL COUNT with GROUP BY to prevent incorrect row returns [shardingsphere]
Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #33380:
URL: https://github.com/apache/shardingsphere/pull/33380#issuecomment-2443221245
> @strongduanmu I have merged the master branch So the code which I corrected earlier was incorrect?
>
> ```
> private List<MemoryQueryResultRow> getMemoryResultSetRows(final SelectStatementContext selectStatementContext,
> final Map<GroupByValue, MemoryQueryResultRow> dataMap, final List<Boolean> valueCaseSensitive) {
> if (dataMap.isEmpty()) {
> return Collections.emptyList();
> }
> List<MemoryQueryResultRow> result = new ArrayList<>(dataMap.values());
> result.sort(new GroupByRowComparator(selectStatementContext, valueCaseSensitive));
> return result;
> }
> ```
>
> Since the all test cases that you have updated are similar to what i wrote.
Yes, you can't remove this logic, it's meaningful for #4680. You can keep the first case and delete the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org