Interpreter: Mock exchange executors. by ywqzzy · Pull Request #4706 · pingcap/tiflash

ywqzzy · 2022-04-19T10:56:20Z

What problem does this PR solve?

Issue Number: ref #4609

Problem Summary:
I didn't support mock exchange executors function, in order to fully supprt mock dagrequest, need to implement all of them.

What is changed and how it works?

Mock exchangeSender&Receivers.
Little refactor of Interpreter test utils in order to mock exchange executors easily.

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-04-19T10:56:21Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise
yibin87

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.

Details

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

dbms/src/Debug/astToExecutor.cpp

dbms/src/TestUtils/mockExecutor.cpp

SeaRise · 2022-04-19T11:28:29Z

dbms/src/TestUtils/mockExecutor.h


    ExecutorPtr root;
    DAGProperties properties;
+    std::unordered_map<String, std::vector<Int64>> receiver_source_task_ids_map;


What is its use?

…_exchange_executors

yibin87

LGTM

dbms/src/TestUtils/mockExecutor.h

dbms/src/TestUtils/InterpreterTestUtils.cpp

SeaRise · 2022-04-21T07:43:04Z

dbms/src/TestUtils/mockExecutor.cpp

+    MPPInfo mpp_info(properties.start_ts, -1, -1, {}, mock_context.receiver_source_task_ids_map);
    std::shared_ptr<tipb::DAGRequest> dag_request_ptr = std::make_shared<tipb::DAGRequest>();
    tipb::DAGRequest & dag_request = *dag_request_ptr;
+    root->toTiPBExecutor(dag_request.mutable_root_executor(), properties.collator, mpp_info, mock_context.context);


why move this line to here?

dbms/src/TestUtils/mockExecutor.cpp

Lloyd-Pottiger

LGTM, cool!

ywqzzy · 2022-04-21T08:00:26Z

/merge

ti-chi-bot · 2022-04-21T08:00:26Z

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

Details

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-04-21T08:00:28Z

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

Details

Commit hash: b215d3f

sre-bot · 2022-04-21T08:24:22Z

Coverage for changed files

Filename                                     Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Debug/astToExecutor.cpp                          502               247    50.80%          48                10    79.17%        1320               631    52.20%         498               263    47.19%
Debug/astToExecutor.h                             29                10    65.52%          18                 5    72.22%          44                16    63.64%          10                 6    40.00%
TestUtils/InterpreterTestUtils.cpp                32                 5    84.38%           6                 1    83.33%          75                19    74.67%          12                 4    66.67%
TestUtils/InterpreterTestUtils.h                   7                 3    57.14%           5                 2    60.00%          21                 9    57.14%           0                 0         -
TestUtils/mockExecutor.cpp                        56                13    76.79%          34                 8    76.47%         215                47    78.14%          26                 7    73.08%
TestUtils/mockExecutor.h                           5                 1    80.00%           5                 1    80.00%          14                 3    78.57%           0                 0         -
TestUtils/tests/gtest_mock_executors.cpp         232               171    26.29%          10                 0   100.00%         151                 0   100.00%          36                36     0.00%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                            863               450    47.86%         126                27    78.57%        1840               725    60.60%         582               316    45.70%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
17142      9494             44.62%    192949  96261        50.11%

full coverage report (for internal network access only)

ywqzzy · 2022-04-21T09:29:02Z

/hold

ywqzzy · 2022-04-21T09:29:18Z

/unhold

ywqzzy added 3 commits April 19, 2022 15:11

update.

831deaa

update.

81f26cb

more tests.

9f4442a

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 Apr 19, 2022

ywqzzy requested a review from SeaRise April 19, 2022 10:58

ywqzzy changed the title ~~Interpreter: Mock exchange executors.~~ [WIP]Interpreter: Mock exchange executors. Apr 19, 2022

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2022

SeaRise reviewed Apr 19, 2022

View reviewed changes

ywqzzy added 4 commits April 20, 2022 14:15

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

1bb0f97

…_exchange_executors

address comments.

27f331f

update.

296b012

update.

037841e

ywqzzy changed the title ~~[WIP]Interpreter: Mock exchange executors.~~ Interpreter: Mock exchange executors. Apr 20, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2022

ywqzzy requested a review from yibin87 April 21, 2022 03:12

yibin87 approved these changes Apr 21, 2022

View reviewed changes

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

ywqzzy requested a review from SeaRise April 21, 2022 03:38

ywqzzy commented Apr 21, 2022

View reviewed changes

dbms/src/TestUtils/mockExecutor.h Outdated Show resolved Hide resolved

SeaRise reviewed Apr 21, 2022

View reviewed changes

dbms/src/TestUtils/InterpreterTestUtils.cpp Show resolved Hide resolved

address comment.

51b1e40

ywqzzy requested review from Lloyd-Pottiger and SeaRise April 21, 2022 07:08

remove useless code.

f62677c

SeaRise approved these changes Apr 21, 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 Apr 21, 2022

Lloyd-Pottiger reviewed Apr 21, 2022

View reviewed changes

dbms/src/TestUtils/mockExecutor.cpp Outdated Show resolved Hide resolved

update.

1675156

Lloyd-Pottiger approved these changes Apr 21, 2022

View reviewed changes

Merge branch 'master' into mock_exchange_executors

b215d3f

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

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2022

ti-chi-bot merged commit 28a97fd into pingcap:master Apr 21, 2022

ywqzzy mentioned this pull request Apr 21, 2022

Interpreter & Executor & MPP unit test framework #4609

Closed

54 tasks

Conversation

ywqzzy commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SeaRise Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

yibin87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SeaRise Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

Uh oh!

ywqzzy commented Apr 21, 2022

Uh oh!

ti-chi-bot commented Apr 21, 2022

Uh oh!

ti-chi-bot commented Apr 21, 2022

Uh oh!

sre-bot commented Apr 21, 2022

Coverage for changed files

Coverage summary

Uh oh!

ywqzzy commented Apr 21, 2022

Uh oh!

ywqzzy commented Apr 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ywqzzy commented Apr 19, 2022 •

edited

Loading

ti-chi-bot commented Apr 19, 2022 •

edited

Loading