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: generate physical plan for IndexMergePath #11245

Merged
merged 8 commits into from
Sep 12, 2019
Merged

planner: generate physical plan for IndexMergePath #11245

merged 8 commits into from
Sep 12, 2019

Conversation

hailanwhu
Copy link
Contributor

@hailanwhu hailanwhu commented Jul 14, 2019

What problem does this PR solve?

Fix #11113.

What is changed and how it works?

When meeting the IndexMergePath in (ds *DataSource) findBestTask , it will call function convertToIndexMergeScan() to generate the physical plan for the path.

Check List

Tests

  • No code

Code changes

  • Has exported function/method change
    Add convertToIndexMergeScan() and getIndexMergeCandidate() in planner/core/find_best_task.go.
    Modify some codes in (ds *DataSource) findBestTask and skylinePruning(), and some related functions for copTask.
  • Has exported variable/fields change
    Add PhysicalIndexMergeReader to present the physical plan for IndexMergePath.

Side effects

  • Possible performance regression

Related changes

@codecov
Copy link

codecov bot commented Jul 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@c8ed781). Click here to learn what that means.
The diff coverage is 10.3896%.

@@             Coverage Diff             @@
##             master     #11245   +/-   ##
===========================================
  Coverage          ?   81.1477%           
===========================================
  Files             ?        423           
  Lines             ?      90249           
  Branches          ?          0           
===========================================
  Hits              ?      73235           
  Misses            ?      11714           
  Partials          ?       5300

@codecov
Copy link

codecov bot commented Jul 14, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11245   +/-   ##
===========================================
  Coverage   81.4765%   81.4765%           
===========================================
  Files           453        453           
  Lines         98216      98216           
===========================================
  Hits          80023      80023           
  Misses        12528      12528           
  Partials       5665       5665

@hailanwhu
Copy link
Contributor Author

hailanwhu commented Jul 14, 2019

@eurekaka This the first version for generating physical plan for IndexMergePath. And please add it to the project.

@eurekaka eurekaka added sig/planner SIG: Planner contribution This PR is from a community contributor. type/new-feature labels Jul 15, 2019
@eurekaka eurekaka self-requested a review July 15, 2019 03:24
@qw4990 qw4990 self-requested a review July 18, 2019 05:52
@foreyes foreyes self-requested a review July 18, 2019 07:39
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.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
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.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
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/physical_plans.go Outdated Show resolved Hide resolved
planner/core/task.go Outdated Show resolved Hide resolved
planner/core/task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go 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
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
@eurekaka eurekaka changed the title Generate physical plan for IndexMergePath planner: generate physical plan for IndexMergePath Jul 25, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 29, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@eurekaka
Copy link
Contributor

/ok-to-test

@qw4990 qw4990 removed their request for review July 31, 2019 06:31
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
totalRowCount += partPlan.StatsCount()
}
p.stats = property.NewSimpleStats(totalRowCount)
p.stats.StatsVersion = t.idxMergePartPlans[0].statsInfo().StatsVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using t.idxMergePartPlans[0].statsInfo().Scale() to keep the NDV of the stats info? Also, we can put the stats assignment into Init as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put the stats assignment into Init as well?

planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Show resolved Hide resolved
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.

The code is almost fine for me, except for those trivial commented problems. Please add tests for this PR, thanks.

planner/core/find_best_task.go Outdated Show resolved Hide resolved
}
cop.idxMergePartPlans = scans
cop.cst = totalCost
task = cop
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this line.

planner/core/find_best_task.go Outdated Show resolved Hide resolved
return ts, partialCost, rowCount, true
}

func (ds *DataSource) buildIndexMergeTableScan(prop *property.PhysicalProperty, tableFilters []expression.Expression, totalRowCount float64) (PhysicalPlan, float64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function shares a lot of code with convertToPartialTableScan, can we extract a common utility function for reuse?

Copy link
Contributor

Choose a reason for hiding this comment

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

And also with convertToTableScan, convertToPartialIndexScan, it is better to use small functions that can compose rather than long and repeated codes.

Copy link
Contributor

Choose a reason for hiding this comment

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

PTAL @hailanwhu

totalRowCount += partPlan.StatsCount()
}
p.stats = property.NewSimpleStats(totalRowCount)
p.stats.StatsVersion = t.idxMergePartPlans[0].statsInfo().StatsVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put the stats assignment into Init as well?

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.

Can you add some test?

planner/core/cbo_test.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
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.

Why is the stats json file containing so many lines? should it be only one line?

@alivxxx alivxxx removed their request for review September 9, 2019 08:51
@@ -593,7 +595,11 @@ func (ds *DataSource) deriveIndexPathStats(path *accessPath, conds []expression.
logutil.BgLogger().Debug("calculate selectivity failed, use selection factor", zap.Error(err))
selectivity = selectionFactor
}
path.countAfterIndex = math.Max(path.countAfterAccess*selectivity, ds.stats.RowCount)
if isIm {
path.countAfterAccess = path.countAfterAccess * selectivity
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change the countAfterAccess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, it should be countAfterIndex.

planner/core/find_best_task.go Outdated Show resolved Hide resolved
hailanwhu added 6 commits September 12, 2019 15:02
temporary implement of generate IndexMergePlan

add functions to generate partial scans
refactor some code

refactor convertToIndexMergeScan

fix comments

(p PhysicalIndexMergeReader) Init(): p.schema

fix comments

temp

delete the comment of convertToPartialIndexScan function

add test for indexmerge; implement explain;
fix some bugs

temp

explian-text
@zz-jason
Copy link
Member

/rebuild

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 12, 2019
@zz-jason zz-jason requested review from eurekaka and alivxxx and removed request for foreyes, winoros and lzmhhh123 September 12, 2019 05:21
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

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. status/PTAL labels Sep 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 12, 2019

Your auto merge job has been accepted, waiting for 12164, 12083, 12055

@sre-bot
Copy link
Contributor

sre-bot commented Sep 12, 2019

/run-all-tests

@sre-bot sre-bot merged commit 65edb2d into pingcap:master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. 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.

Generate physical plan for IndexMergePath
6 participants