Test aggregation functions #5364

xzhangxian1008 · 2022-07-13T08:28:00Z

What problem does this PR solve?

Issue Number: ref #5347

Problem Summary:

What is changed and how it works?

Add some tests with different types to guarantee the correctness of group by.

Add some tests for the following aggregation functions:

Max
Min
Count

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2022-07-13T08:28:02Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Lloyd-Pottiger
SeaRise

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

xzhangxian1008 · 2022-07-13T08:28:42Z

/cc @ywqzzy /cc @Willendless

ti-chi-bot · 2022-07-13T08:28:44Z

@xzhangxian1008: GitHub didn't allow me to request PR reviews from the following users: /cc.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @ywqzzy /cc @Willendless

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

ywqzzy · 2022-07-13T09:03:22Z

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

+
+    void executeWithConcurrency(const std::shared_ptr<tipb::DAGRequest> & request, const ColumnsWithTypeAndName & expect_columns)
+    {
+        for (size_t i = 1; i < max_concurrency; i += step)


Suggested change

for (size_t i = 1; i < max_concurrency; i += step)

for (size_t i = 1; i <= max_concurrency; i += step)

ywqzzy · 2022-07-13T09:05:12Z

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

+    {
+        /// We can filter the group by column with project operator.
+        /// topN is applied to get stable results in concurrency environment.
+        return context.scan(db_name, table_name).aggregation(agg_funcs, group_by_exprs).topN(order_by_items, 100).project(proj).build(context);


maybe explain why we need a projection?

maybe explain why we need a projection?

Done

Willendless · 2022-07-13T16:14:17Z

/run-all-tests

sre-bot · 2022-07-13T16:32:55Z

Coverage for changed files

Filename                                       Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/tests/gtest_aggregation_executor.cpp          87                41    52.87%           5                 0   100.00%          79                 0   100.00%          22                11    50.00%
Flash/tests/gtest_topn_executor.cpp                219                63    71.23%           5                 0   100.00%         132                 2    98.48%          70                32    54.29%
TestUtils/mockExecutor.h                             6                 1    83.33%           6                 1    83.33%          15                 3    80.00%           0                 0         -
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                              312               105    66.35%          16                 1    93.75%         226                 5    97.79%          92                43    53.26%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18514      9445             48.98%    208791  95404        54.31%

full coverage report (for internal network access only)

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

Lloyd-Pottiger · 2022-07-15T05:32:54Z

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

+    {
+        ExecutorTest::initializeContext();
+
+        context.addMockTable({db_name, table_name},


how about add /* parameter_name = */ {db_name, table_name} in this case, otherwise it is confusing.

ywqzzy

Others LGTM

ywqzzy · 2022-07-19T03:42:26Z

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

+    const String table_name{"clerk"};
+    const std::vector<String> col_name{"age", "gender", "country", "salary"};
+    ColumnWithNullableInt32 col_age{30, {}, 27, 32, 25, 36, {}, 22, 34};
+    ColumnWithNullableString col_gender{


in one line？

in one line？

It's clang-tidy who forces me to write like this.

ywqzzy · 2022-07-19T03:52:08Z

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

+    std::shared_ptr<tipb::DAGRequest> buildDAGRequest(std::pair<String, String> src, MockAstVec agg_funcs, MockAstVec group_by_exprs, MockColumnNameVec proj)
+    {
+        /// We can filter the group by column with project operator.
+        /// project is applied to get single column for comparison


More comments of why use project, like “partial agg output group by columns"

More comments of why use project, like “partial agg output group by columns"

Done

Willendless · 2022-07-19T04:05:10Z

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

+    size_t max_concurrency = 10;
+    size_t step = 2;


Maybe use static const?

Maybe use static const?

Get

xzhangxian1008 · 2022-07-19T04:43:54Z

/run-all-tests

sre-bot · 2022-07-19T05:02:29Z

Coverage for changed files

Filename                                       Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/tests/gtest_aggregation_executor.cpp         230                72    68.70%           6                 0   100.00%         155                 1    99.35%          54                26    51.85%
Flash/tests/gtest_topn_executor.cpp                219                63    71.23%           5                 0   100.00%         132                 2    98.48%          70                32    54.29%
TestUtils/mockExecutor.h                             6                 1    83.33%           6                 1    83.33%          15                 3    80.00%           0                 0         -
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                              455               136    70.11%          17                 1    94.12%         302                 6    98.01%         124                58    53.23%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18623      9392             49.57%    212305  94876        55.31%

full coverage report (for internal network access only)

SeaRise

other LGTM

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

SeaRise · 2022-07-19T04:03:22Z

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

+}
+CATCH
+
+// TODO more aggregation functions...


and add TODO more data types...?

SeaRise · 2022-07-19T08:50:41Z

dbms/src/Flash/tests/gtest_aggregation_executor.cpp

+}
+CATCH
+
+TEST_F(ExecutorAggTestRunner, AggregationMaxAndMin)


add comment like TODO: support more type of min, max, count？

add comment like TODO: support more type of min, max, count？

okk

…test

ywqzzy · 2022-07-19T09:30:47Z

/merge

ti-chi-bot · 2022-07-19T09:30:48Z

@ywqzzy: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-07-19T09:30:50Z

This pull request has been accepted and is ready to merge.

Commit hash: c1a18c8

Co-authored-by: SeaRise <hhssearise@foxmail.com>

sre-bot · 2022-07-19T09:49:40Z

Coverage for changed files

Filename                                       Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/tests/gtest_aggregation_executor.cpp         230                72    68.70%           6                 0   100.00%         155                 1    99.35%          54                26    51.85%
Flash/tests/gtest_topn_executor.cpp                219                63    71.23%           5                 0   100.00%         132                 2    98.48%          70                32    54.29%
TestUtils/mockExecutor.h                             6                 1    83.33%           6                 1    83.33%          15                 3    80.00%           0                 0         -
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                              455               136    70.11%          17                 1    94.12%         302                 6    98.01%         124                58    53.23%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18625      9394             49.56%    212345  94924        55.30%

full coverage report (for internal network access only)

SeaRise · 2022-07-19T10:18:40Z

/run-all-tests

sre-bot · 2022-07-19T10:36:26Z

Coverage for changed files

Filename                                       Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/tests/gtest_aggregation_executor.cpp         230                72    68.70%           6                 0   100.00%         155                 1    99.35%          54                26    51.85%
Flash/tests/gtest_topn_executor.cpp                219                63    71.23%           5                 0   100.00%         132                 2    98.48%          70                32    54.29%
TestUtils/mockExecutor.h                             6                 1    83.33%           6                 1    83.33%          15                 3    80.00%           0                 0         -
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                              455               136    70.11%          17                 1    94.12%         302                 6    98.01%         124                58    53.23%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18625      9393             49.57%    212345  94871        55.32%

full coverage report (for internal network access only)

sre-bot · 2022-07-19T10:56:16Z

Coverage for changed files

Filename                                       Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/tests/gtest_aggregation_executor.cpp         230                72    68.70%           6                 0   100.00%         155                 1    99.35%          54                26    51.85%
Flash/tests/gtest_topn_executor.cpp                219                63    71.23%           5                 0   100.00%         132                 2    98.48%          70                32    54.29%
TestUtils/mockExecutor.h                             6                 1    83.33%           6                 1    83.33%          15                 3    80.00%           0                 0         -
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                              455               136    70.11%          17                 1    94.12%         302                 6    98.01%         124                58    53.23%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18625      9393             49.57%    212349  94925        55.30%

full coverage report (for internal network access only)

ti-chi-bot · 2022-07-19T11:01:18Z

@xzhangxian1008: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

sre-bot · 2022-07-19T11:22:11Z

Coverage for changed files

Filename                                       Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/tests/gtest_aggregation_executor.cpp         230                72    68.70%           6                 0   100.00%         155                 1    99.35%          54                26    51.85%
Flash/tests/gtest_topn_executor.cpp                219                63    71.23%           5                 0   100.00%         132                 2    98.48%          70                32    54.29%
TestUtils/mockExecutor.h                             6                 1    83.33%           6                 1    83.33%          15                 3    80.00%           0                 0         -
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                              455               136    70.11%          17                 1    94.12%         302                 6    98.01%         124                58    53.23%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18625      9393             49.57%    212350  94869        55.32%

full coverage report (for internal network access only)

xzhangxian1008 added 6 commits June 29, 2022 16:49

init agg works

d7f9ca4

chase after"

f001b8b

Merge branch 'master' into agg_test

55dd1c3

add more tests

9e42c8e

format

9e81bd2

Merge branch 'master' into agg_test

016a3c3

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 13, 2022

ti-chi-bot requested a review from ywqzzy July 13, 2022 08:28

ti-chi-bot requested a review from Willendless July 13, 2022 08:28

ywqzzy reviewed Jul 13, 2022

View reviewed changes

dbms/src/Flash/tests/gtest_aggregation_executor.cpp Show resolved Hide resolved

ywqzzy reviewed Jul 13, 2022

View reviewed changes

ti-chi-bot added do-not-merge/needs-linked-issue and removed do-not-merge/needs-linked-issue labels Jul 13, 2022

Merge branch 'master' into agg_test

9a0577f

SeaRise reviewed Jul 14, 2022

View reviewed changes

dbms/src/Flash/tests/gtest_aggregation_executor.cpp Outdated Show resolved Hide resolved

xzhangxian1008 added 2 commits July 14, 2022 14:35

update

49d74b7

Merge branch 'master' into agg_test

ae55d91

Lloyd-Pottiger reviewed Jul 15, 2022

View reviewed changes

ywqzzy requested review from ywqzzy, SeaRise and Lloyd-Pottiger July 15, 2022 07:13

refine

afbcb6e

Lloyd-Pottiger approved these changes Jul 15, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 15, 2022

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 19, 2022

xzhangxian1008 added 3 commits July 19, 2022 11:34

update

80c412d

format

6db334b

update

1bb7904

ywqzzy reviewed Jul 19, 2022

View reviewed changes

Willendless reviewed Jul 19, 2022

View reviewed changes

tweaking

e831683

SeaRise self-requested a review July 19, 2022 08:45

SeaRise approved these changes Jul 19, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 19, 2022

xzhangxian1008 added 2 commits July 19, 2022 17:24

Merge branch 'master' of https://github.com/pingcap/tiflash into agg_…

6ae1b2b

…test

update

c1a18c8

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 19, 2022

Update dbms/src/Flash/tests/gtest_aggregation_executor.cpp

2b01a23

Co-authored-by: SeaRise <hhssearise@foxmail.com>

Merge branch 'master' into agg_test

f8f1714

Merge branch 'master' into agg_test

6478c2b

ti-chi-bot merged commit 29330a2 into pingcap:master Jul 19, 2022

xzhangxian1008 deleted the agg_test branch July 20, 2022 01:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test aggregation functions #5364

Test aggregation functions #5364

xzhangxian1008 commented Jul 13, 2022 •

edited

Loading

ti-chi-bot commented Jul 13, 2022 •

edited

Loading

xzhangxian1008 commented Jul 13, 2022

ti-chi-bot commented Jul 13, 2022

ywqzzy Jul 13, 2022

ywqzzy Jul 13, 2022

xzhangxian1008 Jul 14, 2022

Willendless commented Jul 13, 2022

sre-bot commented Jul 13, 2022

Lloyd-Pottiger Jul 15, 2022

ywqzzy left a comment

ywqzzy Jul 19, 2022

xzhangxian1008 Jul 19, 2022

ywqzzy Jul 19, 2022

xzhangxian1008 Jul 19, 2022

Willendless Jul 19, 2022

xzhangxian1008 Jul 19, 2022

xzhangxian1008 commented Jul 19, 2022

sre-bot commented Jul 19, 2022

SeaRise left a comment

SeaRise Jul 19, 2022

SeaRise Jul 19, 2022

xzhangxian1008 Jul 19, 2022

ywqzzy commented Jul 19, 2022

ti-chi-bot commented Jul 19, 2022

ti-chi-bot commented Jul 19, 2022

sre-bot commented Jul 19, 2022

SeaRise commented Jul 19, 2022

sre-bot commented Jul 19, 2022

sre-bot commented Jul 19, 2022

ti-chi-bot commented Jul 19, 2022

sre-bot commented Jul 19, 2022

	for (size_t i = 1; i < max_concurrency; i += step)
	for (size_t i = 1; i <= max_concurrency; i += step)

Test aggregation functions #5364

Test aggregation functions #5364

Conversation

xzhangxian1008 commented Jul 13, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jul 13, 2022 • edited Loading

xzhangxian1008 commented Jul 13, 2022

ti-chi-bot commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Willendless commented Jul 13, 2022

sre-bot commented Jul 13, 2022

Coverage for changed files

Coverage summary

Choose a reason for hiding this comment

ywqzzy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhangxian1008 commented Jul 19, 2022

sre-bot commented Jul 19, 2022

Coverage for changed files

Coverage summary

SeaRise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywqzzy commented Jul 19, 2022

ti-chi-bot commented Jul 19, 2022

ti-chi-bot commented Jul 19, 2022

sre-bot commented Jul 19, 2022

Coverage for changed files

Coverage summary

SeaRise commented Jul 19, 2022

sre-bot commented Jul 19, 2022

Coverage for changed files

Coverage summary

sre-bot commented Jul 19, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented Jul 19, 2022

sre-bot commented Jul 19, 2022

Coverage for changed files

Coverage summary

xzhangxian1008 commented Jul 13, 2022 •

edited

Loading

ti-chi-bot commented Jul 13, 2022 •

edited

Loading