Storage: Refactor DMFileReader #8927

Lloyd-Pottiger · 2024-04-10T07:37:54Z

What problem does this PR solve?

Issue Number: close #8904

Problem Summary:

In the previous implementation, I used insertManyFrom to insert data in ConstColumn into the target column.

Lines 216 to 220 in adc57e2

    
           void insertManyFrom(const IColumn & src, size_t position, size_t length) override 
        
           { 
        
               const auto & value = static_cast<const Self &>(src).getData()[position]; 
        
               data.resize_fill(data.size() + length, value); 
        
           }

In insertManyFrom, ColumnVector::insertManyFrom only expected calling src with exactly the same type of Self rather than any subclass of IColumn. If we do so, static_cast<const Self &>(src).getData() just returns a random memory location. It causes the del tag column contains too much non-zero value, leading to return less data than expected.

What is changed and how it works?

Add a new interface insertMany in IColumn Add insertMany interface for IColumn #8925
Use insertMany instead of insertManyFrom

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

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ref pingcap#8889

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

CalvinNeo · 2024-04-10T08:33:05Z

Could you elaborate the bug in this issue

Lloyd-Pottiger · 2024-04-10T08:46:22Z

Could you elaborate the bug in this issue

done

JaySon-Huang · 2024-04-10T17:53:19Z

In insertManyFrom, the data is get from the container, but ConstColumn does not have any container.

Actually, ColumnVector::insertManyFrom only expected calling src with exactly the same type of Self rather than any subclass of IColumn. If we do so, static_cast<const Self &>(src).getData() just returns a random memory location.

Lloyd-Pottiger · 2024-04-11T05:06:16Z

In insertManyFrom, the data is get from the container, but ConstColumn does not have any container.

Actually, ColumnVector::insertManyFrom only expected calling src with exactly the same type of Self rather than any subclass of IColumn. If we do so, static_cast<const Self &>(src).getData() just returns a random memory location.

updated

JaySon-Huang · 2024-04-11T08:51:23Z

/hold
need to verify with coverage report

Lloyd-Pottiger · 2024-04-15T03:26:20Z

Lloyd-Pottiger · 2024-04-15T03:26:31Z

/unhold

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp

Co-authored-by: jinhelin <linjinhe33@gmail.com>

dbms/src/Storages/DeltaMerge/File/ColumnCache.cpp

release-centos7-llvm/gen_coverage.sh

JaySon-Huang · 2024-04-15T14:08:17Z

dbms/src/Storages/DeltaMerge/File/DMFileReader.cpp

            Block block = read();
+            size_t rows = block.rows();

-            IColumn::Filter block_filter(filter.cbegin() + read_rows, filter.cbegin() + read_rows + block.rows());
-            read_rows += block.rows();
-
-            if (size_t passed_count = countBytesInFilter(block_filter); passed_count != block.rows())
+            if (size_t passed_count = countBytesInFilter(filter, offset, rows); passed_count != rows)
            {
-                for (auto & col : block)
+                std::vector<size_t> positions;
+                positions.reserve(passed_count);
+                for (size_t p = offset; p < offset + rows; ++p)
                {
-                    col.column = col.column->filter(block_filter, passed_count);
+                    if (filter[p])
+                        positions.push_back(p - offset);
+                }
+                for (size_t i = 0; i < block.columns(); ++i)
+                {
+                    columns[i]->insertDisjunctFrom(*block.getByPosition(i).column, positions);
                }
            }
-
-            blocks.emplace_back(std::move(block));
+            else
+            {
+                for (size_t i = 0; i < block.columns(); ++i)
+                {
+                    columns[i]->insertRangeFrom(
+                        *block.getByPosition(i).column,
+                        0,
+                        block.getByPosition(i).column->size());
+                }
+            }


We assume readWithFilter will never be called on the three hidden columns, so we don't need to take care about whether the block.getByPosition(i).column is ColumnConst or not?

clean read is not enabled when there are hidden columns.

JaySon-Huang · 2024-04-15T14:10:38Z

dbms/src/Storages/DeltaMerge/File/DMFileReader.cpp

+                for (size_t i = 0; i < block.columns(); ++i)
+                {
+                    columns[i]->insertRangeFrom(
+                        *block.getByPosition(i).column,
+                        0,
+                        block.getByPosition(i).column->size());
+                }


Are there any cases cover this branch?

It is kind of hard to construct.... #8927 (comment) never do clean read here, so the source and target column are must be the same.

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

JaySon-Huang

LGTM

ti-chi-bot · 2024-04-26T09:46:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, JinheLin

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JaySon-Huang,JinheLin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-04-26T09:46:17Z

[LGTM Timeline notifier]

Timeline:

2024-04-22 03:04:37.48262518 +0000 UTC m=+825696.593671617: ☑️ agreed by JaySon-Huang.
2024-04-26 09:46:16.912907791 +0000 UTC m=+4930.670043361: ☑️ agreed by JinheLin.

ti-chi-bot · 2024-04-26T10:18:06Z

@Lloyd-Pottiger: 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

trigger some heavy tests which will not run always when PR updated.

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.

Lloyd-Pottiger and others added 5 commits April 10, 2024 10:52

Add insertMany interface for IColumn

9cf1c24

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

add comments

83867a0

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

address comments

432aecb

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

tests: Add code coverage scripts (pingcap#8922)

9ac28b4

ref pingcap#8889

Refactor DMFileReader

be62450

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot 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 10, 2024

Merge branch 'master' into refactor-dmfile-reader

cc97520

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

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

fix

8972ec8

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger force-pushed the refactor-dmfile-reader branch from 00a6838 to 8972ec8 Compare April 15, 2024 03:27

Merge branch 'master' into refactor-dmfile-reader

6da0d5c

Lloyd-Pottiger requested review from JaySon-Huang, JinheLin and CalvinNeo April 15, 2024 03:28

Merge branch 'master' into refactor-dmfile-reader

2858ff4

JinheLin reviewed Apr 15, 2024

View reviewed changes

dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp Outdated Show resolved Hide resolved

JinheLin reviewed Apr 15, 2024

View reviewed changes

dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

780609d

Co-authored-by: jinhelin <linjinhe33@gmail.com>

JaySon-Huang reviewed Apr 15, 2024

View reviewed changes

refine ut

880378e

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger and others added 2 commits April 16, 2024 10:39

refine ut

cdcb8f2

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Add some comments

05936ea

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2024

Merge branch 'master' into refactor-dmfile-reader

c4ce37f

Lloyd-Pottiger requested a review from JaySon-Huang April 19, 2024 01:46

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

Lloyd-Pottiger requested a review from JinheLin April 19, 2024 01:46

JaySon-Huang approved these changes Apr 22, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Apr 22, 2024

Lloyd-Pottiger added 2 commits April 22, 2024 11:46

Merge branch 'master' into refactor-dmfile-reader

f252db7

Merge branch 'master' into refactor-dmfile-reader

bef6755

JinheLin approved these changes Apr 26, 2024

View reviewed changes

ti-chi-bot bot added the lgtm label Apr 26, 2024

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 26, 2024

Merge branch 'master' into refactor-dmfile-reader

3028526

ti-chi-bot bot merged commit 98e4a7c into pingcap:master Apr 26, 2024
6 checks passed

Lloyd-Pottiger deleted the refactor-dmfile-reader branch May 8, 2024 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage: Refactor DMFileReader #8927

Storage: Refactor DMFileReader #8927

Lloyd-Pottiger commented Apr 10, 2024 •

edited

Loading

CalvinNeo commented Apr 10, 2024

Lloyd-Pottiger commented Apr 10, 2024

JaySon-Huang commented Apr 10, 2024

Lloyd-Pottiger commented Apr 11, 2024

JaySon-Huang commented Apr 11, 2024

Lloyd-Pottiger commented Apr 15, 2024

Lloyd-Pottiger commented Apr 15, 2024

JaySon-Huang Apr 15, 2024

Lloyd-Pottiger Apr 16, 2024

JaySon-Huang Apr 15, 2024

Lloyd-Pottiger Apr 16, 2024

JaySon-Huang left a comment

ti-chi-bot bot commented Apr 26, 2024

ti-chi-bot bot commented Apr 26, 2024

ti-chi-bot bot commented Apr 26, 2024

	void insertManyFrom(const IColumn & src, size_t position, size_t length) override
	{
	const auto & value = static_cast<const Self &>(src).getData()[position];
	data.resize_fill(data.size() + length, value);
	}

Storage: Refactor DMFileReader #8927

Storage: Refactor DMFileReader #8927

Conversation

Lloyd-Pottiger commented Apr 10, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

CalvinNeo commented Apr 10, 2024

Lloyd-Pottiger commented Apr 10, 2024

JaySon-Huang commented Apr 10, 2024

Lloyd-Pottiger commented Apr 11, 2024

JaySon-Huang commented Apr 11, 2024

Lloyd-Pottiger commented Apr 15, 2024

Lloyd-Pottiger commented Apr 15, 2024

JaySon-Huang Apr 15, 2024

Choose a reason for hiding this comment

Lloyd-Pottiger Apr 16, 2024

Choose a reason for hiding this comment

JaySon-Huang Apr 15, 2024

Choose a reason for hiding this comment

Lloyd-Pottiger Apr 16, 2024

Choose a reason for hiding this comment

JaySon-Huang left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Apr 26, 2024

ti-chi-bot bot commented Apr 26, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Apr 26, 2024

Lloyd-Pottiger commented Apr 10, 2024 •

edited

Loading