planner: generate physical plan for IndexMergePath #11245

hailanwhu · 2019-07-14T13:15:35Z

What problem does this PR solve?

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 · 2019-07-14T13:20:04Z

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 · 2019-07-14T13:20:04Z

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 · 2019-07-14T13:23:56Z

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

planner/core/find_best_task.go

planner/core/physical_plans.go

planner/core/task.go

planner/core/find_best_task.go

sre-bot · 2019-07-29T03:34:54Z

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 · 2019-07-29T07:38:07Z

/ok-to-test

planner/core/find_best_task.go

eurekaka · 2019-08-06T08:10:20Z

planner/core/task.go

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


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?

Can we put the stats assignment into Init as well?

planner/core/find_best_task.go

eurekaka

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

eurekaka · 2019-08-19T08:26:14Z

planner/core/find_best_task.go

+	}
+	cop.idxMergePartPlans = scans
+	cop.cst = totalCost
+	task = cop


Please remove this line.

planner/core/find_best_task.go

eurekaka · 2019-08-19T08:34:52Z

planner/core/find_best_task.go

+	return ts, partialCost, rowCount, true
+}
+
+func (ds *DataSource) buildIndexMergeTableScan(prop *property.PhysicalProperty, tableFilters []expression.Expression, totalRowCount float64) (PhysicalPlan, float64) {


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

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

PTAL @hailanwhu

eurekaka · 2019-08-19T08:36:17Z

planner/core/task.go

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


Can we put the stats assignment into Init as well?

alivxxx

Can you add some test?

planner/core/cbo_test.go

planner/core/find_best_task.go

eurekaka

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

alivxxx · 2019-09-12T03:08:17Z

planner/core/logical_plans.go

@@ -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


Why change the countAfterAccess?

sorry, it should be countAfterIndex.

planner/core/find_best_task.go

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 · 2019-09-12T05:14:09Z

/rebuild

zz-jason

LGTM

eurekaka

LGTM

sre-bot · 2019-09-12T05:41:07Z

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

sre-bot · 2019-09-12T06:02:17Z

/run-all-tests

eurekaka added sig/planner SIG: Planner contribution This PR is from a community contributor. type/new-feature labels Jul 15, 2019

eurekaka self-requested a review July 15, 2019 03:24

eurekaka added this to In Progress in IndexMerge access method for DNF filters via automation Jul 15, 2019

qw4990 self-requested a review July 18, 2019 05:52

foreyes self-requested a review July 18, 2019 07:39

foreyes reviewed Jul 18, 2019

View reviewed changes

planner/core/find_best_task.go Outdated Show resolved Hide resolved

eurekaka reviewed Jul 19, 2019

View reviewed changes

eurekaka reviewed Jul 25, 2019

View reviewed changes

eurekaka changed the title ~~Generate physical plan for IndexMergePath~~ planner: generate physical plan for IndexMergePath Jul 25, 2019

qw4990 removed their request for review July 31, 2019 06:31

zz-jason requested review from eurekaka and foreyes August 3, 2019 08:20

zz-jason added the status/PTAL label Aug 3, 2019

eurekaka reviewed Aug 6, 2019

View reviewed changes

eurekaka reviewed Aug 19, 2019

View reviewed changes

eurekaka requested review from alivxxx, winoros, lzmhhh123 and zz-jason August 19, 2019 08:38

alivxxx reviewed Aug 20, 2019

View reviewed changes

zz-jason reviewed Aug 30, 2019

View reviewed changes

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

eurekaka reviewed Sep 3, 2019

View reviewed changes

eurekaka requested a review from zz-jason September 3, 2019 07:24

eurekaka requested a review from alivxxx September 3, 2019 07:24

alivxxx removed their request for review September 9, 2019 08:51

hailanwhu requested a review from alivxxx September 11, 2019 12:03

alivxxx reviewed Sep 12, 2019

View reviewed changes

hailanwhu added 6 commits September 12, 2019 15:02

add indexmerge test in explain-test

1514c26

new stats.json

010fa4f

common part

4365c03

rename function

2a4136a

fix comments

a8afe3a

zz-jason reviewed Sep 12, 2019

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 12, 2019

zz-jason requested review from eurekaka and alivxxx and removed request for foreyes, winoros and lzmhhh123 September 12, 2019 05:21

eurekaka approved these changes Sep 12, 2019

View reviewed changes

Merge branch 'master' into IndexMergePhysicalPlan

80f7c24

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

Merge branch 'master' into IndexMergePhysicalPlan

bc88d42

sre-bot merged commit 65edb2d into pingcap:master Sep 12, 2019

IndexMerge access method for DNF filters automation moved this from In Progress to Done Sep 12, 2019

hailanwhu mentioned this pull request Sep 23, 2019

executor: support IndexMergeReaderExecutor #12305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: generate physical plan for IndexMergePath #11245

planner: generate physical plan for IndexMergePath #11245

hailanwhu commented Jul 14, 2019 •

edited

codecov bot commented Jul 14, 2019

codecov bot commented Jul 14, 2019 •

edited

hailanwhu commented Jul 14, 2019 •

edited

sre-bot commented Jul 29, 2019

eurekaka commented Jul 29, 2019

eurekaka Aug 6, 2019

eurekaka Aug 19, 2019

eurekaka left a comment •

edited

eurekaka Aug 19, 2019

eurekaka Aug 19, 2019

alivxxx Sep 4, 2019

alivxxx Sep 9, 2019

eurekaka Aug 19, 2019

alivxxx left a comment

eurekaka left a comment

alivxxx Sep 12, 2019

hailanwhu Sep 12, 2019

zz-jason commented Sep 12, 2019

zz-jason left a comment

eurekaka left a comment

sre-bot commented Sep 12, 2019

sre-bot commented Sep 12, 2019

planner: generate physical plan for IndexMergePath #11245

planner: generate physical plan for IndexMergePath #11245

Conversation

hailanwhu commented Jul 14, 2019 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Jul 14, 2019

Codecov Report

codecov bot commented Jul 14, 2019 • edited

Codecov Report

hailanwhu commented Jul 14, 2019 • edited

sre-bot commented Jul 29, 2019

eurekaka commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eurekaka left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alivxxx left a comment

Choose a reason for hiding this comment

eurekaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Sep 12, 2019

zz-jason left a comment

Choose a reason for hiding this comment

eurekaka left a comment

Choose a reason for hiding this comment

sre-bot commented Sep 12, 2019

sre-bot commented Sep 12, 2019

hailanwhu commented Jul 14, 2019 •

edited

codecov bot commented Jul 14, 2019 •

edited

hailanwhu commented Jul 14, 2019 •

edited

eurekaka left a comment •

edited