Make fast scan code mode clean #6058

hongyunyan · 2022-09-29T08:34:06Z

What problem does this PR solve?

Issue Number: ref #5252

Problem Summary:

What is changed and how it works?

Seperate the read process in selraw * and the read process in fast scan, to make the code more understandable.

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-09-29T08:34:08Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
breezewish

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.

hongyunyan · 2022-09-29T08:37:26Z

/run-all-tests

sre-bot · 2022-09-29T09:02:27Z

Coverage for changed files

Filename                         Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DMSegmentThreadInputStream.h          82                 5    93.90%           6                 0   100.00%          94                18    80.85%          32                11    65.62%
DeltaMergeStore.cpp                  896               155    82.70%          47                 2    95.74%        1308               227    82.65%         468               150    67.95%
Segment.cpp                         1131               275    75.69%          56                 1    98.21%        1371               121    91.17%         360               102    71.67%
Segment.h                             41                 3    92.68%          26                 2    92.31%          37                 3    91.89%           4                 2    50.00%
SegmentReadTaskPool.cpp              100                14    86.00%          21                 2    90.48%         208                45    78.37%          64                23    64.06%
SegmentReadTaskPool.h                 40                 1    97.50%          17                 1    94.12%          66                 3    95.45%          10                 1    90.00%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                               2290               453    80.22%         173                 8    95.38%        3084               417    86.48%         938               289    69.19%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18389      7473             59.36%    215330  76915        64.28%

full coverage report (for internal network access only)

dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.cpp

dbms/src/Storages/DeltaMerge/Segment.cpp

hongyunyan · 2022-09-29T09:24:54Z

/cc @breezewish

dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.h

breezewish

Some minor suggestions. This PR is a nice step towards better readability. Good job!

breezewish · 2022-09-30T09:58:47Z

dbms/src/Storages/DeltaMerge/Segment.cpp

+/// We call getInputStreamFast when we read in fast mode.
+/// In this case, we will read all the data in delta and stable, and then merge them without sorting.
+/// Besides, we will do del_mark != 0 filtering to drop the deleted rows.
+/// In conclusion, the output is unsorted, and does not do mvcc filtering.


This is a very nice explanation!

dbms/src/Storages/DeltaMerge/Segment.cpp

dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.h

Co-authored-by: Wenxuan <[email protected]>

dbms/src/Storages/DeltaMerge/Segment.cpp

JinheLin · 2022-10-09T03:20:35Z

dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.cpp

@@ -100,14 +100,35 @@ BlockInputStreamPtr SegmentReadTaskPool::buildInputStream(SegmentReadTaskPtr & t
    MemoryTrackerSetter setter(true, mem_tracker.get());
    auto seg = t->segment;
    BlockInputStreamPtr stream;
-    if (is_raw)
+    auto block_size = std::max(expected_block_size, static_cast<size_t>(dm_context->db_context.getSettingsRef().dt_segment_stable_pack_rows));


Code in dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.cpp L103-L131 is similar to dbms/src/Storages/DeltaMerge/DMSegmentThreadInputStream.h L103-L131.

Maybe we can do some refactoring and make code reusable.

Yes, I try to make a new function for L103-L131 to make code reusable, please take a look.

…unyan/tiflash into hongyunyan_fix_segment_raw

hongyunyan · 2022-10-12T10:53:34Z

/run-all-tests

breezewish · 2022-10-12T13:00:36Z

dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.cpp

-        break;
-    }
-    LOG_FMT_DEBUG(log, "getInputStream succ, pool_id={} segment_id={}", pool_id, seg->segmentId());
+    buildStreamBasedOnReadMode(stream, read_mode, t, dm_context, columns_to_read, filter, max_version, expected_block_size);


How about this?

getInputStream(Mode, ...)

getInputStreamModeNormal(...)

getInputStreamModeFast(...)

getInputStreamModeRaw(...)

Good suggestion! I think getInputStream(Mode, ...) in class Segment is a better way to make the code reuseable and clean.

hongyunyan · 2022-10-13T00:57:24Z

/run-all-tests

sre-bot · 2022-10-13T01:16:49Z

Coverage for changed files

Filename                                     Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DMSegmentThreadInputStream.h                      76                 3    96.05%           6                 0   100.00%          66                 2    96.97%          24                 7    70.83%
DeltaMergeStore.cpp                              816               107    86.89%          44                 2    95.45%        1154               181    84.32%         440               134    69.55%
Segment.cpp                                     1113               276    75.20%          58                 1    98.28%        1432               124    91.34%         368               103    72.01%
Segment.h                                         41                 3    92.68%          27                 2    92.59%          38                 3    92.11%           4                 2    50.00%
SegmentReadTaskPool.cpp                           94                14    85.11%          21                 2    90.48%         179                45    74.86%          56                22    60.71%
SegmentReadTaskPool.h                             39                 1    97.44%          17                 1    94.12%          66                 3    95.45%          10                 1    90.00%
tests/gtest_dm_segment.cpp                       402               131    67.41%          36                 0   100.00%        1134                 6    99.47%         164                77    53.05%
tests/gtest_dm_segment_common_handle.cpp         253                64    74.70%          35                 0   100.00%         748                 0   100.00%          64                37    42.19%
tests/gtest_segment_replace_data.cpp             189                77    59.26%          12                 0   100.00%         312                 1    99.68%          54                44    18.52%
tests/gtest_segment_test_basic.cpp               729               155    78.74%          24                 0   100.00%         447                14    96.87%         200                75    62.50%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                           3752               831    77.85%         280                 8    97.14%        5576               379    93.20%        1384               502    63.73%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18347      7397             59.68%    215143  76107        64.62%

full coverage report (for internal network access only)

dbms/src/Storages/DeltaMerge/Segment.cpp

dbms/src/Storages/DeltaMerge/Segment.h

hongyunyan · 2022-10-13T07:33:31Z

/merge

ti-chi-bot · 2022-10-13T07:33:32Z

@hongyunyan: 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-10-13T07:33:34Z

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

Commit hash: 0320e01

ti-chi-bot · 2022-10-13T07:33:47Z

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

hongyunyan · 2022-10-13T07:45:04Z

/run-unit-test

sre-bot · 2022-10-13T07:55:33Z

Coverage for changed files

Filename                                     Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DMSegmentThreadInputStream.h                      76                 3    96.05%           6                 0   100.00%          66                 2    96.97%          24                 7    70.83%
DeltaMergeStore.cpp                              816               107    86.89%          44                 2    95.45%        1154               181    84.32%         440               132    70.00%
Segment.cpp                                     1113               276    75.20%          58                 1    98.28%        1432               124    91.34%         368               106    71.20%
Segment.h                                         41                 3    92.68%          27                 2    92.59%          38                 3    92.11%           4                 2    50.00%
SegmentReadTaskPool.cpp                           94                14    85.11%          21                 2    90.48%         179                45    74.86%          56                22    60.71%
SegmentReadTaskPool.h                             39                 1    97.44%          17                 1    94.12%          66                 3    95.45%          10                 1    90.00%
tests/gtest_dm_segment.cpp                       402               131    67.41%          36                 0   100.00%        1134                 6    99.47%         164                77    53.05%
tests/gtest_dm_segment_common_handle.cpp         253                64    74.70%          35                 0   100.00%         748                 0   100.00%          64                37    42.19%
tests/gtest_segment_replace_data.cpp             189                77    59.26%          12                 0   100.00%         312                 1    99.68%          54                44    18.52%
tests/gtest_segment_test_basic.cpp               729               155    78.74%          24                 0   100.00%         447                14    96.87%         200                75    62.50%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                           3752               831    77.85%         280                 8    97.14%        5576               379    93.20%        1384               503    63.66%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18347      7394             59.70%    215143  76024        64.66%

full coverage report (for internal network access only)

sre-bot · 2022-10-13T08:05:03Z

Coverage for changed files

Filename                                     Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
DMSegmentThreadInputStream.h                      76                 3    96.05%           6                 0   100.00%          66                 2    96.97%          24                 7    70.83%
DeltaMergeStore.cpp                              816               107    86.89%          44                 2    95.45%        1154               181    84.32%         440               132    70.00%
Segment.cpp                                     1113               276    75.20%          58                 1    98.28%        1432               124    91.34%         368               105    71.47%
Segment.h                                         41                 3    92.68%          27                 2    92.59%          38                 3    92.11%           4                 2    50.00%
SegmentReadTaskPool.cpp                           94                14    85.11%          21                 2    90.48%         179                45    74.86%          56                22    60.71%
SegmentReadTaskPool.h                             39                 1    97.44%          17                 1    94.12%          66                 3    95.45%          10                 1    90.00%
tests/gtest_dm_segment.cpp                       402               131    67.41%          36                 0   100.00%        1134                 6    99.47%         164                77    53.05%
tests/gtest_dm_segment_common_handle.cpp         253                64    74.70%          35                 0   100.00%         748                 0   100.00%          64                37    42.19%
tests/gtest_segment_replace_data.cpp             189                77    59.26%          12                 0   100.00%         312                 1    99.68%          54                44    18.52%
tests/gtest_segment_test_basic.cpp               729               155    78.74%          24                 0   100.00%         447                14    96.87%         200                75    62.50%
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                           3752               831    77.85%         280                 8    97.14%        5576               379    93.20%        1384               502    63.73%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18347      7398             59.68%    215147  76081        64.64%

full coverage report (for internal network access only)

hongyunyan added 2 commits September 29, 2022 16:28

refactor code

b2e7620

update code

b0988f8

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 Sep 29, 2022

Lloyd-Pottiger reviewed Sep 29, 2022

View reviewed changes

dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.cpp Outdated Show resolved Hide resolved

dbms/src/Storages/DeltaMerge/Segment.cpp Outdated Show resolved Hide resolved

ti-chi-bot requested a review from breezewish September 29, 2022 09:24

remove useless code

2be41bb

JinheLin reviewed Sep 29, 2022

View reviewed changes

dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.h Outdated Show resolved Hide resolved

update enum class

51ad9a3

breezewish approved these changes Sep 30, 2022

View reviewed changes

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

hongyunyan and others added 2 commits September 30, 2022 21:04

Update dbms/src/Storages/DeltaMerge/Segment.cpp

a31ed93

Co-authored-by: Wenxuan <[email protected]>

Update dbms/src/Storages/DeltaMerge/SegmentReadTaskPool.h

b7a9a74

Co-authored-by: Wenxuan <[email protected]>

Lloyd-Pottiger reviewed Sep 30, 2022

View reviewed changes

dbms/src/Storages/DeltaMerge/Segment.cpp Show resolved Hide resolved

dbms/src/Storages/DeltaMerge/Segment.cpp Outdated Show resolved Hide resolved

JinheLin reviewed Oct 9, 2022

View reviewed changes

JaySon-Huang approved these changes Oct 12, 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 Oct 12, 2022

hongyunyan added 3 commits October 12, 2022 17:58

add some comments

768f657

Merge branch 'hongyunyan_fix_segment_raw' of https://github.com/hongy…

e360390

…unyan/tiflash into hongyunyan_fix_segment_raw

make some improvements for code and comments

8a7274f

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Oct 12, 2022

merge master

2afa87f

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2022

Lloyd-Pottiger approved these changes Oct 12, 2022

View reviewed changes

breezewish reviewed Oct 12, 2022

View reviewed changes

hongyunyan added 2 commits October 13, 2022 08:51

refactor

2a1be54

refactor

58ec367

Lloyd-Pottiger reviewed Oct 13, 2022

View reviewed changes

dbms/src/Storages/DeltaMerge/Segment.cpp Outdated Show resolved Hide resolved

dbms/src/Storages/DeltaMerge/Segment.h Outdated Show resolved Hide resolved

hongyunyan added 2 commits October 13, 2022 09:31

fix typo

4b2530e

fix typo

0320e01

JinheLin approved these changes Oct 13, 2022

View reviewed changes

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

Merge branch 'master' into hongyunyan_fix_segment_raw

5898b5b

ti-chi-bot merged commit 6a9e9ac into pingcap:master Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make fast scan code mode clean #6058

Make fast scan code mode clean #6058

hongyunyan commented Sep 29, 2022

ti-chi-bot commented Sep 29, 2022 •

edited

Loading

hongyunyan commented Sep 29, 2022

sre-bot commented Sep 29, 2022

hongyunyan commented Sep 29, 2022

breezewish left a comment

breezewish Sep 30, 2022

JinheLin Oct 9, 2022

hongyunyan Oct 12, 2022

hongyunyan commented Oct 12, 2022

breezewish Oct 12, 2022

hongyunyan Oct 13, 2022

hongyunyan commented Oct 13, 2022

sre-bot commented Oct 13, 2022

hongyunyan commented Oct 13, 2022

ti-chi-bot commented Oct 13, 2022

ti-chi-bot commented Oct 13, 2022

ti-chi-bot commented Oct 13, 2022

hongyunyan commented Oct 13, 2022

sre-bot commented Oct 13, 2022

sre-bot commented Oct 13, 2022

Make fast scan code mode clean #6058

Make fast scan code mode clean #6058

Conversation

hongyunyan commented Sep 29, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 29, 2022 • edited Loading

hongyunyan commented Sep 29, 2022

sre-bot commented Sep 29, 2022

Coverage for changed files

Coverage summary

hongyunyan commented Sep 29, 2022

breezewish left a comment

Choose a reason for hiding this comment

breezewish Sep 30, 2022

Choose a reason for hiding this comment

JinheLin Oct 9, 2022

Choose a reason for hiding this comment

hongyunyan Oct 12, 2022

Choose a reason for hiding this comment

hongyunyan commented Oct 12, 2022

breezewish Oct 12, 2022

Choose a reason for hiding this comment

hongyunyan Oct 13, 2022

Choose a reason for hiding this comment

hongyunyan commented Oct 13, 2022

sre-bot commented Oct 13, 2022

Coverage for changed files

Coverage summary

hongyunyan commented Oct 13, 2022

ti-chi-bot commented Oct 13, 2022

ti-chi-bot commented Oct 13, 2022

ti-chi-bot commented Oct 13, 2022

hongyunyan commented Oct 13, 2022

sre-bot commented Oct 13, 2022

Coverage for changed files

Coverage summary

sre-bot commented Oct 13, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented Sep 29, 2022 •

edited

Loading