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.

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

planner: support cost model for tiflash table scan #12868

Merged
merged 18 commits into from
Oct 29, 2019

Conversation

lzmhhh123
Copy link
Contributor

@lzmhhh123 lzmhhh123 commented Oct 22, 2019

What problem does this PR solve?

Add a new table scan cost in the cost model to distinguish TiKV and TiFlash. Also, generate the TiFlash access path when a table has the TiFlash replica.

What is changed and how it works?

There are four steps:

  1. When a table has at least one TiFlash replica, generate a new access path for it. And don't do skyline pruning for it.
  2. When calculating the cost of the scan operation for a physical plan, we should add the key size for TiKV to distinguish the different row size of TiKV and TiFlash.
  3. Due to the column format of the TiFlash storing arrange. The seek time of a range scan is more than TiKV. TiKV has only one seek time for a range, but TiFlash has len(Columns) times. Then we add another cost to deal with the difference.
  4. Avoid build table scan of TiFlash as the inner child of the index join.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity

@lzmhhh123 lzmhhh123 requested a review from a team as a code owner October 23, 2019 08:09
@ghost ghost requested review from wshwsh12 and removed request for a team October 23, 2019 08:10
@pingcap pingcap deleted a comment from CLAassistant Oct 23, 2019
@codecov
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #12868 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12868   +/-   ##
===========================================
  Coverage   80.4636%   80.4636%           
===========================================
  Files           465        465           
  Lines        109514     109514           
===========================================
  Hits          88119      88119           
  Misses        14889      14889           
  Partials       6506       6506

@lzmhhh123 lzmhhh123 removed the request for review from wshwsh12 October 23, 2019 10:12
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

Can you extract the changes of accounting key prefix size of TiKV and related test result changes into a separate PR for easier review and commit management?

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

└─IndexScan_11 10000.00 cop[tikv] table:access_path_selection, index:a, b, range:[NULL,+inf], keep order:false, stats:pseudo
TableReader_7 3323.33 root data:Selection_6
└─Selection_6 3323.33 cop[tikv] lt(Column#2, 3)
└─TableScan_5 10000.00 cop[tikv] table:access_path_selection, range:[-inf,+inf], keep order:false, stats:pseudo
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like many cases in this file are meaningless now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's somehow meaningful compared with other explain tests in the file.

sessionctx/variable/session.go Show resolved Hide resolved
statistics/table.go Show resolved Hide resolved
statistics/table.go Show resolved Hide resolved
statistics/table.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
statistics/table.go Outdated Show resolved Hide resolved
eurekaka
eurekaka previously approved these changes Oct 29, 2019
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Oct 29, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 29, 2019

/run-all-tests

@sre-bot sre-bot merged commit 6fd74f3 into pingcap:master Oct 29, 2019
@lzmhhh123 lzmhhh123 deleted the dev/support_cbo_for_tiflash branch October 29, 2019 08:07
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
lzmhhh123 added a commit to lzmhhh123/tidb that referenced this pull request Jan 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants