Pipeline: Support pipeline table scan fullstack part 1 #7225

ywqzzy · 2023-04-06T06:03:55Z

What problem does this PR solve?

Issue Number: ref #6518

Problem Summary:

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

…ort_pipeline_table_scan_fullstack

ti-chi-bot · 2023-04-06T06:03:57Z

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

ywqzzy · 2023-04-06T06:04:53Z

/run-all-tests

…ort_pipeline_table_scan_fullstack

dbms/src/Flash/Planner/Plans/PhysicalTableScan.cpp

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

ywqzzy · 2023-04-12T09:20:38Z

/run-all-tests

SeaRise

And use failpoint region_exception_after_read_from_storage_some_error and region_exception_after_read_from_storage_all_error to add test for pipeline remote read.

dbms/src/Operators/CoprocessorReaderSourceOp.cpp

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

SeaRise · 2023-04-13T06:00:04Z

Seems that

tiflash/dbms/src/Flash/executeQuery.cpp

Lines 169 to 170 in ef2e8e9

    
           // now only support pipeline model in test mode. 
        
           if (context.isTest()

need to be modified.
Support non-test-mode and forbid Disagg mode.
Others LGTM

dbms/src/DataStreams/GeneratedColumnPlaceHolderTransformAction.h

dbms/src/Operators/SourceOp_fwd.h

…ort_pipeline_table_scan_fullstack

ywqzzy · 2023-04-19T02:34:54Z

/run-all-tests

xzhangxian1008 · 2023-04-19T06:59:34Z

dbms/src/DataStreams/GeneratedColumnPlaceHolderTransformAction.cpp

+        const auto & col_index = std::get<0>(ele);
+        const auto & col_name = std::get<1>(ele);
+        const auto & data_type = std::get<2>(ele);


Suggested change

const auto & col_index = std::get<0>(ele);

const auto & col_name = std::get<1>(ele);

const auto & data_type = std::get<2>(ele);

constexpr int col_index_pos = 1;

constexpr int col_name_pos = 2;

constexpr int data_type_pos = 3;

const auto & col_index = std::get<col_index_pos>(ele);

const auto & col_name = std::get<col_name_pos>(ele);

const auto & data_type = std::get<data_type_pos>(ele);

Maybe get tuple's value with meaningful const name is better

Maybe get tuple's value with meaningful const name is better

Only use for 1 time. I will keep it

xzhangxian1008 · 2023-04-19T07:01:09Z

dbms/src/DataStreams/GeneratedColumnPlaceHolderTransformAction.cpp

+    // Validation check.
+    for (size_t i = 1; i < generated_column_infos.size(); ++i)
+    {
+        RUNTIME_CHECK(std::get<0>(generated_column_infos[i]) > std::get<0>(generated_column_infos[i - 1]));


xzhangxian1008 · 2023-04-19T08:50:20Z

dbms/src/Flash/Planner/PhysicalPlan.cpp

 {
    assert(root_node);
    PipelineBuilder builder{log->identifier()};
-    root_node->buildPipeline(builder);
+    root_node->buildPipeline(builder, context, exec_status);


Maybe we can define a helper struct to contain buildPipeline parameters?
such as:

struct buildPipelineParams { PipelineExecutorStatus exec_status; Context * context; };

Maybe we can define a helper struct to contain buildPipeline parameters? such as:

struct buildPipelineParams { PipelineExecutorStatus exec_status; Context * context; };

Not necessary? Since so many place can use struct as Param, IMHO only if the api is not stable and need to change frequently, we can use struct to hold all the param

xzhangxian1008

lgtm

ti-chi-bot · 2023-04-20T02:54:12Z

@xzhangxian1008: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

lgtm

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.

ywqzzy · 2023-04-20T03:40:19Z

/merge

ti-chi-bot · 2023-04-20T03:40:20Z

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

SeaRise · 2023-04-20T04:21:07Z

/merge cancel

ywqzzy · 2023-04-20T05:57:53Z

/merge

ti-chi-bot · 2023-04-20T05:57:54Z

@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 · 2023-04-20T05:57:56Z

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

Commit hash: 76956e0

ywqzzy · 2023-04-20T07:05:41Z

/run-unit-tests

ywqzzy added 12 commits March 9, 2023 18:22

tmp save

c1f9554

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

5cab781

…ort_pipeline_table_scan_fullstack

can run simple queries

373d0d8

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

fd8087f

…ort_pipeline_table_scan_fullstack

tmp save

8243ccd

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

1450a7a

…ort_pipeline_table_scan_fullstack

tiny refine.

2fab85d

fmt

841cabd

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

b7a469e

…ort_pipeline_table_scan_fullstack

fix remote

dc24eef

support generated column

c09481f

disable pipeline

7c87eaa

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 6, 2023

ywqzzy added 2 commits April 7, 2023 11:45

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

1845fc1

…ort_pipeline_table_scan_fullstack

remote read aysnc

e6314d5

ywqzzy force-pushed the support_pipeline_table_scan_fullstack branch from de73ae5 to e6314d5 Compare April 7, 2023 06:29

client change

11c520a

SeaRise reviewed Apr 12, 2023

View reviewed changes

dbms/src/Flash/Planner/Plans/PhysicalTableScan.cpp Outdated Show resolved Hide resolved

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp Outdated Show resolved Hide resolved

ywqzzy added 3 commits April 12, 2023 16:40

fix cop read

067ceaa

address comments

e33ae47

change client-c

3b98ab0

fix mpp statistics

84f9ad3

SeaRise reviewed Apr 13, 2023

View reviewed changes

address comments

388fa9b

ywqzzy requested a review from SeaRise April 13, 2023 06:25

xzhangxian1008 reviewed Apr 18, 2023

View reviewed changes

dbms/src/DataStreams/GeneratedColumnPlaceHolderTransformAction.h Outdated Show resolved Hide resolved

xzhangxian1008 reviewed Apr 18, 2023

View reviewed changes

dbms/src/Operators/SourceOp_fwd.h Show resolved Hide resolved

ywqzzy requested a review from Lloyd-Pottiger April 18, 2023 08:36

ywqzzy added 3 commits April 18, 2023 16:40

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

26793a5

…ort_pipeline_table_scan_fullstack

keep consistent with master

091c4d1

fix build

7833379

ywqzzy requested a review from xzhangxian1008 April 19, 2023 02:35

Lloyd-Pottiger approved these changes Apr 19, 2023

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 19, 2023

SeaRise mentioned this pull request Apr 19, 2023

Support pipeline model #6518

Closed

25 tasks

xzhangxian1008 reviewed Apr 19, 2023

View reviewed changes

xzhangxian1008 approved these changes Apr 20, 2023

View reviewed changes

Merge branch 'master' into support_pipeline_table_scan_fullstack

b516e56

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

Merge branch 'master' into support_pipeline_table_scan_fullstack

76956e0

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 20, 2023

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

xzhangxian1008 approved these changes Apr 20, 2023

View reviewed changes

ti-chi-bot merged commit 68412d1 into pingcap:master Apr 20, 2023

windtalker mentioned this pull request Apr 21, 2023

Infinite loop if coprocess reader is cancelled before exit #7337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pipeline: Support pipeline table scan fullstack part 1 #7225

Pipeline: Support pipeline table scan fullstack part 1 #7225

ywqzzy commented Apr 6, 2023

ti-chi-bot commented Apr 6, 2023 •

edited

Loading

ywqzzy commented Apr 6, 2023

ywqzzy commented Apr 12, 2023

SeaRise left a comment •

edited

Loading

SeaRise commented Apr 13, 2023

ywqzzy commented Apr 19, 2023

xzhangxian1008 Apr 19, 2023

ywqzzy Apr 20, 2023

xzhangxian1008 Apr 19, 2023

xzhangxian1008 Apr 19, 2023

ywqzzy Apr 20, 2023

xzhangxian1008 left a comment

ti-chi-bot commented Apr 20, 2023

ywqzzy commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

SeaRise commented Apr 20, 2023

ywqzzy commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

ywqzzy commented Apr 20, 2023

-        const auto & col_index = std::get<0>(ele);
-        const auto & col_name = std::get<1>(ele);
-        const auto & data_type = std::get<2>(ele);
+        constexpr int col_index_pos = 1;
+        constexpr int col_name_pos = 2;
+        constexpr int data_type_pos = 3;
+        const auto & col_index = std::get<col_index_pos>(ele);
+        const auto & col_name = std::get<col_name_pos>(ele);
+        const auto & data_type = std::get<data_type_pos>(ele);

Pipeline: Support pipeline table scan fullstack part 1 #7225

Pipeline: Support pipeline table scan fullstack part 1 #7225

Conversation

ywqzzy commented Apr 6, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 6, 2023 • edited Loading

ywqzzy commented Apr 6, 2023

ywqzzy commented Apr 12, 2023

SeaRise left a comment • edited Loading

Choose a reason for hiding this comment

SeaRise commented Apr 13, 2023

ywqzzy commented Apr 19, 2023

xzhangxian1008 Apr 19, 2023

Choose a reason for hiding this comment

ywqzzy Apr 20, 2023

Choose a reason for hiding this comment

xzhangxian1008 Apr 19, 2023

Choose a reason for hiding this comment

xzhangxian1008 Apr 19, 2023

Choose a reason for hiding this comment

ywqzzy Apr 20, 2023

Choose a reason for hiding this comment

xzhangxian1008 left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Apr 20, 2023

ywqzzy commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

SeaRise commented Apr 20, 2023

ywqzzy commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

ywqzzy commented Apr 20, 2023

ti-chi-bot commented Apr 6, 2023 •

edited

Loading

SeaRise left a comment •

edited

Loading