Test: support push down filter in ut by ywqzzy · Pull Request #6573 · pingcap/tiflash

ywqzzy · 2023-01-04T00:46:46Z

What problem does this PR solve?

Issue Number: ref #4609

Problem Summary:
We don't push down filter to storage layer in current ut framework.

What is changed and how it works?

In getStreamFromDeltaMerge, mock DAGQueryInfo with PushDownFilter from tipb:Selection

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 · 2023-01-04T00:46:47Z

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

Details

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

ywqzzy · 2023-01-04T03:11:53Z

/run-all-tests

ywqzzy · 2023-01-04T03:59:49Z

/run-all-tests

ywqzzy · 2023-01-04T06:27:53Z

/run-all-tests

Lloyd-Pottiger · 2023-01-04T07:33:27Z

dbms/src/Debug/MockStorage.cpp

 }

-BlockInputStreamPtr MockStorage::getStreamFromDeltaMerge(Context & context, Int64 id)
+BlockInputStreamPtr MockStorage::getStreamFromDeltaMerge(Context & context, Int64 table_id, const PushDownFilter * push_down_filter)


why not use shared_ptr

why not use shared_ptr

The filter will be used only once

Lloyd-Pottiger · 2023-01-04T07:46:24Z

dbms/src/Debug/MockStorage.cpp

+        BlockInputStreamPtr in = ins[0];
+        in = std::make_shared<FilterBlockInputStream>(in, before_where, filter_column_name, "test");


add a assert for the size of ins

The size will be changed by num_streams, so I didn't check it.
In future, I will pass num_streams into storage->read()

Lloyd-Pottiger · 2023-01-04T07:49:40Z

dbms/src/Flash/tests/gtest_filter_executor.cpp

 }
 CATCH

+TEST_F(FilterExecutorTestRunner, PushDownFilter)


maybe need more tests

maybe need more tests

Ok, I will test more filters

ywqzzy · 2023-01-04T08:24:22Z

/run-all-tests

dbms/src/Flash/Planner/PhysicalPlan.cpp

SeaRise

Others LGTM

dbms/src/Flash/tests/gtest_filter_executor.cpp

dbms/src/TestUtils/ExecutorTestUtils.cpp

SeaRise

lgtm

dbms/src/Flash/Planner/PhysicalPlan.cpp

Co-authored-by: SeaRise <hhssearise@foxmail.com>

ywqzzy · 2023-01-05T03:11:51Z

/merge

ti-chi-bot · 2023-01-05T03:11:52Z

@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 · 2023-01-05T03:11:54Z

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

Details

Commit hash: 1db7ea0

ywqzzy · 2023-01-05T04:03:04Z

/run-all-tests

support push down filter in ut

00c5083

ywqzzy mentioned this pull request Jan 4, 2023

Interpreter & Executor & MPP unit test framework #4609

Closed

54 tasks

show stream

1c2fd70

ywqzzy changed the title ~~[WIP]Test: support push down filter in ut~~ Test: support push down filter in ut Jan 4, 2023

ti-chi-bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue labels Jan 4, 2023

ywqzzy added 3 commits January 4, 2023 11:36

more assert

0c2b479

refine

4fb26ce

refine

77cace2

fmt

6678b77

ywqzzy force-pushed the support_lm_ut branch from 720f95e to 6678b77 Compare January 4, 2023 04:05

Lloyd-Pottiger reviewed Jan 4, 2023

View reviewed changes

ywqzzy added 2 commits January 4, 2023 15:52

fix some wrong col name.

65cdfab

add more tests

a37200d

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 Jan 4, 2023

fmt

fea1ac9

ywqzzy force-pushed the support_lm_ut branch from 443409f to fea1ac9 Compare January 4, 2023 09:47

SeaRise reviewed Jan 4, 2023

View reviewed changes

dbms/src/Flash/Planner/PhysicalPlan.cpp Outdated Show resolved Hide resolved

SeaRise reviewed Jan 4, 2023

View reviewed changes

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

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

address comment

7827292

ywqzzy force-pushed the support_lm_ut branch from 068a194 to 7827292 Compare January 5, 2023 03:01

ywqzzy requested review from Lloyd-Pottiger and SeaRise January 5, 2023 03:02

SeaRise approved these changes Jan 5, 2023

View reviewed changes

dbms/src/Flash/Planner/PhysicalPlan.cpp Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 5, 2023

Update dbms/src/Flash/Planner/PhysicalPlan.cpp

b0021a9

Co-authored-by: SeaRise <hhssearise@foxmail.com>

Lloyd-Pottiger approved these changes Jan 5, 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 Jan 5, 2023

Merge branch 'master' into support_lm_ut

1db7ea0

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

ti-chi-bot merged commit 3350580 into pingcap:master Jan 5, 2023

		BlockInputStreamPtr in = ins[0];
		in = std::make_shared<FilterBlockInputStream>(in, before_where, filter_column_name, "test");

Conversation

ywqzzy commented Jan 4, 2023 • 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 Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywqzzy commented Jan 4, 2023

Uh oh!

ywqzzy commented Jan 4, 2023

Uh oh!

ywqzzy commented Jan 4, 2023

Uh oh!

Lloyd-Pottiger Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

ywqzzy Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

Lloyd-Pottiger Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

ywqzzy Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

Lloyd-Pottiger Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

ywqzzy Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

ywqzzy commented Jan 4, 2023

Uh oh!

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ywqzzy commented Jan 5, 2023

Uh oh!

ti-chi-bot commented Jan 5, 2023

Uh oh!

ti-chi-bot commented Jan 5, 2023

Uh oh!

ywqzzy commented Jan 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ywqzzy commented Jan 4, 2023 •

edited

Loading

ti-chi-bot commented Jan 4, 2023 •

edited

Loading