Support colllation for planner refactory test framework #5449

xzhangxian1008 · 2022-07-22T05:02:26Z

What problem does this PR solve?

Issue Number: close #5432

Problem Summary:

Support colllation for planner refactory test framework

What is changed and how it works?

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-22T05:02:27Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise
ywqzzy

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-22T05:02:48Z

/cc @ywqzzy /cc @SeaRise

ti-chi-bot · 2022-07-22T05:02:50Z

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

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.

ywqzzy

Others LGTM

dbms/src/Flash/tests/gtest_collation.cpp

ywqzzy

LGTM

Willendless · 2022-07-23T17:20:27Z

dbms/src/TestUtils/mockExecutor.h

+    explicit DAGRequestBuilder(size_t & index, Int32 collator = TiDB::ITiDBCollator::UTF8MB4_BIN)
        : executor_index(index)
    {
+        properties.collator = -abs(collator);


Why do we use -abs(collator) instead of just collator?

Why do we use -abs(collator) instead of just collator?

collator passed by tidb is negative, so we need to guarantee the collator < 0. See https://github.com/pingcap/tiflash/blob/master/dbms/src/Flash/Coprocessor/DAGUtils.cpp#L1357

…cola

SeaRise · 2022-07-25T09:26:43Z

dbms/src/TestUtils/mockExecutor.h

@@ -101,6 +103,9 @@ class DAGRequestBuilder
    DAGRequestBuilder & sort(MockOrderByItem order_by, bool is_partial_sort, uint64_t fine_grained_shuffle_stream_count = 0);
    DAGRequestBuilder & sort(MockOrderByItemVec order_by_vec, bool is_partial_sort, uint64_t fine_grained_shuffle_stream_count = 0);

+    void setCollation(Int32 collator_) { properties.collator = -abs(collator_); }


may be we can wrap a function for -abs(collator_)

/// add some comments Int32 inline getXXXCollation(Int32 collator_) { return -abs(collator_); }

may be we can wrap a function for -abs(collator_)

/// add some comments Int32 inline getXXXCollation(Int32 collator_) { return -abs(collator_); }

ok

SeaRise · 2022-07-25T09:43:09Z

dbms/src/Flash/tests/gtest_collation.cpp

+    {
+        context.setCollation(collation);
+        auto request = context.scan(db_name, table_name).aggregation(MockAstVec{}, {col(col_name)}).project({col_name}).build(context);
+        std::cout << request->DebugString() << std::endl;


useless code?

useless code?

I will delete it

SeaRise · 2022-07-25T09:44:06Z

dbms/src/Flash/tests/gtest_collation.cpp

+        context.setCollation(collation);
+        auto request = context.scan(db_name, table_name).aggregation(MockAstVec{}, {col(col_name)}).project({col_name}).build(context);
+        std::cout << request->DebugString() << std::endl;
+        ASSERT_COLUMNS_EQ_UR(expect, executeStreams(request, 1));


seems executeStreams(reuqest) is enough.

seems executeStreams(reuqest) is enough.

ok

SeaRise · 2022-07-25T09:46:12Z

dbms/src/Flash/tests/gtest_collation.cpp

+    std::queue<tipb::Executor *> executors;
+    tipb::Executor * executor = dag_request->mutable_root_executor();
+    executors.push(executor);


We can use https://github.com/pingcap/tiflash/blob/e289119502cb4f2643eb1810dffb4434a611b0a9/dbms/src/Flash/Statistics/traverseExecutors.h to visit DAGRequest

We can use https://github.com/pingcap/tiflash/blob/e289119502cb4f2643eb1810dffb4434a611b0a9/dbms/src/Flash/Statistics/traverseExecutors.h to visit DAGRequest

Done

SeaRise · 2022-07-25T09:46:51Z

dbms/src/Flash/tests/gtest_collation.cpp

+{
+    std::queue<tipb::Executor *> executors;
+    std::queue<const tipb::Expr *> exprs;
+    tipb::Executor * executor = dag_request->mutable_root_executor();


SeaRise · 2022-07-25T09:47:45Z

dbms/src/TestUtils/mockExecutor.h

@@ -117,8 +122,9 @@ class DAGRequestBuilder
 class MockDAGRequestContext
 {
 public:
-    explicit MockDAGRequestContext(Context context_)
+    explicit MockDAGRequestContext(Context context_, Int32 collation_ = TiDB::ITiDBCollator::UTF8MB4_BIN)


Why is the default parameter not no collation?

Why is the default parameter not no collation?

Because tidb opens the collation by default recently, and sets it as UTF8MB4_BIN.

…cola

SeaRise

LGTM

ywqzzy · 2022-07-26T03:27:53Z

/merge

ti-chi-bot · 2022-07-26T03:27:53Z

@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-26T03:27:56Z

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

Commit hash: 9880766

sre-bot · 2022-07-26T03:48:49Z

Coverage for changed files

Filename                            Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                 579               137    76.34%          53                 3    94.34%        1528               348    77.23%         572               186    67.48%
Debug/astToExecutor.h                    33                 9    72.73%          22                 4    81.82%          50                13    74.00%          10                 6    40.00%
Flash/tests/gtest_collation.cpp         280                82    70.71%          10                 0   100.00%         349                57    83.67%         124                48    61.29%
TestUtils/mockExecutor.cpp               76                 4    94.74%          43                 2    95.35%         280                27    90.36%          38                 2    94.74%
TestUtils/mockExecutor.h                 11                 4    63.64%          11                 4    63.64%          23                 6    73.91%           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                   979               236    75.89%         139                13    90.65%        2230               451    79.78%         744               242    67.47%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18826      9434             49.89%    214679  95365        55.58%

full coverage report (for internal network access only)

xzhangxian1008 added 2 commits July 22, 2022 12:59

submit

46d2465

Merge branch 'master' into add_cola

e7a67a0

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 22, 2022

ti-chi-bot requested a review from ywqzzy July 22, 2022 05:02

ti-chi-bot requested a review from SeaRise July 22, 2022 05:02

foramt

7e36a55

ywqzzy reviewed Jul 22, 2022

View reviewed changes

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

SeaRise reviewed Jul 22, 2022

View reviewed changes

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

simplify

4c7ce38

ywqzzy approved these changes Jul 22, 2022

View reviewed changes

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

Willendless reviewed Jul 23, 2022

View reviewed changes

xzhangxian1008 added 2 commits July 25, 2022 15:46

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

8a0053d

…cola

update

7cbdff7

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 25, 2022

tweaking

46e8942

SeaRise self-requested a review July 25, 2022 08:13

format

5164252

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 25, 2022

SeaRise reviewed Jul 25, 2022

View reviewed changes

xzhangxian1008 added 2 commits July 26, 2022 09:01

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

d2fd9dc

…cola

update

9880766

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 26, 2022

SeaRise self-requested a review July 26, 2022 03:00

SeaRise approved these changes Jul 26, 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 26, 2022

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

ti-chi-bot merged commit 679eda6 into pingcap:master Jul 26, 2022

xzhangxian1008 deleted the add_cola branch July 26, 2022 05:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support colllation for planner refactory test framework #5449

Support colllation for planner refactory test framework #5449

xzhangxian1008 commented Jul 22, 2022 •

edited

Loading

ti-chi-bot commented Jul 22, 2022 •

edited

Loading

xzhangxian1008 commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ywqzzy left a comment

ywqzzy left a comment

Willendless Jul 23, 2022 •

edited

Loading

xzhangxian1008 Jul 25, 2022

SeaRise Jul 25, 2022

xzhangxian1008 Jul 26, 2022

SeaRise Jul 25, 2022

xzhangxian1008 Jul 26, 2022

SeaRise Jul 25, 2022

xzhangxian1008 Jul 26, 2022

SeaRise Jul 25, 2022

xzhangxian1008 Jul 26, 2022

SeaRise Jul 25, 2022

SeaRise Jul 25, 2022

xzhangxian1008 Jul 26, 2022

SeaRise left a comment

ywqzzy commented Jul 26, 2022

ti-chi-bot commented Jul 26, 2022

ti-chi-bot commented Jul 26, 2022

sre-bot commented Jul 26, 2022

Support colllation for planner refactory test framework #5449

Support colllation for planner refactory test framework #5449

Conversation

xzhangxian1008 commented Jul 22, 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 22, 2022 • edited Loading

xzhangxian1008 commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ywqzzy left a comment

Choose a reason for hiding this comment

ywqzzy left a comment

Choose a reason for hiding this comment

Willendless Jul 23, 2022 • edited Loading

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

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

SeaRise left a comment

Choose a reason for hiding this comment

ywqzzy commented Jul 26, 2022

ti-chi-bot commented Jul 26, 2022

ti-chi-bot commented Jul 26, 2022

sre-bot commented Jul 26, 2022

Coverage for changed files

Coverage summary

xzhangxian1008 commented Jul 22, 2022 •

edited

Loading

ti-chi-bot commented Jul 22, 2022 •

edited

Loading

Willendless Jul 23, 2022 •

edited

Loading