fix: SQL COUNT with GROUP BY to prevent incorrect row returns

[Malaydewangan09]

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 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.

[strongduanmu]

Hi @Malaydewangan09, can you add unit test and e2e test for this change?

[Malaydewangan09]

@strongduanmu I have included unit tests for my change locally.
Can you help with e2e test?
Thanks!

[strongduanmu]

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

[Malaydewangan09]

Hey @strongduanmu
Can you check this if these failing checks are from my side or any changes are required ?
Thanks!

[strongduanmu]

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.

[strongduanmu]

@strongduanmu I have updated the unit tests.

Good job, let's wait ci result.

[strongduanmu]

LGTM, merged. @Malaydewangan09 Thank you for your contribution.

[strongduanmu]

Hi @Malaydewangan09, can you add a e2e test case for this issue - #4680

[Malaydewangan09]

@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?

[strongduanmu]

@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.

[Malaydewangan09]

Okay @strongduanmu
As I understand the issue #4680 was
#4680 (comment) all these clauses were having problem
So can I write one e2e case for any one of these ?

[strongduanmu]

Okay @strongduanmu As I understand the issue #4680 was #4680 (comment) 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.

[strongduanmu]

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.

[Malaydewangan09]

@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.

[strongduanmu]

@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.

[Malaydewangan09]

@strongduanmu
I guess the changes made are no longer working for the current issue e2e case?

[strongduanmu]

@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.

[strongduanmu]

LGTM, @Malaydewangan09 Thank you for your contribution.

[Malaydewangan09]

Thanks @strongduanmu for constant feedback!