Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

planner: add projection pushdown #27029

Merged
merged 151 commits into from
Apr 22, 2022
Merged

planner: add projection pushdown #27029

merged 151 commits into from
Apr 22, 2022

Conversation

ichn-hu
Copy link
Contributor

@ichn-hu ichn-hu commented Aug 9, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #26242

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

Support projection on tikv.

The PR on tikv's side: tikv/tikv#10689
Intergration test on copr-test: tikv/copr-test#145

How it Works:

Check List

Tests

  • Unit test
  • Integration test
  • Manual test
    • in addition to current integration tests, we also manually run the explaintest with projection push down enabled, the diff can be seen in
      0001-explaintest-passed.patch.txt

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note 

Introduce session variable `tidb_opt_projection_push_down` that supports push down projection to TiKV,

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 9, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • fzhedu
  • winoros

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.

@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2021
@ichn-hu ichn-hu changed the title add pushdown planner: add projection pushdown to tikv Aug 9, 2021
@sre-bot
Copy link
Contributor

sre-bot commented Aug 9, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

  • *: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

@ichn-hu ichn-hu marked this pull request as ready for review August 11, 2021 11:59
@ti-chi-bot 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 Aug 11, 2021
This was referenced Aug 12, 2021
Merged
Open
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 19, 2021
@ichn-hu ichn-hu requested review from fzhedu and winoros August 19, 2021 06:35
winoros
winoros reviewed Aug 23, 2021
View reviewed changes
planner/core/plan_to_pb.go Outdated Show resolved Hide resolved
fzhedu
fzhedu reviewed Aug 24, 2021
View reviewed changes
planner/core/testdata/plan_suite_in.json Outdated
Comment on lines 609 to 615
{
"name": "TestPushdownProjection",
"cases": [
"select i + 1 from t;",
"select DATE_FORMAT(t, '%Y-%m-%d %H') as date from t;",
"select md5(s) from t;"
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can get some tests those for tiflash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestPushdownProjection -> TestPushdownProjectionForTiKV

fzhedu
fzhedu reviewed Aug 24, 2021
View reviewed changes
Copy link
Contributor

@fzhedu fzhedu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add more tests, like randgen test, corner cases.
Since the expressions in projection do not performe the same with that in filter, in the former case, expressions output different data types, but in the later just output true or not. We should pay attention to the data type precision or compatibility between tikv and tidb.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2021
@winoros
Copy link
Member

winoros commented Nov 17, 2021

What's the current status of this one?

@hawkingrei
Copy link
Member

/run-mysql-test

@wjhuang2016
Copy link
Member

/hold

@ti-chi-bot 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
@hawkingrei
Copy link
Member

/unhold

@ti-chi-bot 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 22, 2022
@hawkingrei
Copy link
Member

/run-unit-test

1 similar comment
@hawkingrei
Copy link
Member

/run-unit-test

@ti-chi-bot ti-chi-bot merged commit 99b871e into pingcap:master Apr 22, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2022

TiDB MergeCI notify

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test 🟢 all 11 tests passed 11 min Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 9 min 4 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 7 min 37 sec Existing passed
idc-jenkins-ci-tidb/common-test 🟢 all 12 tests passed 6 min 53 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 6 min 40 sec Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 5 min 43 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 5 min 20 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 4 min 9 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

espresso98 added a commit to espresso98/tidb that referenced this pull request Apr 25, 2022
@espresso98
planner: add projection pushdown (pingcap#27029)
996a687 
close pingcap#26242
@yibin87 yibin87 mentioned this pull request Mar 19, 2024
Open
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@winoros winoros winoros approved these changes

@fzhedu fzhedu fzhedu approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
component/config release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Let top-level projection can push down to TiFlash when mpp is not enforced.
10 participants
@ichn-hu @ti-chi-bot @sre-bot @winoros @purelind @hawkingrei @wjhuang2016 @fzhedu @XuHuaiyu @djshow832