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

plan: calc access path when doing deriveStats. #6346

Merged
merged 11 commits into from
Apr 28, 2018

Conversation

winoros
Copy link
Member

@winoros winoros commented Apr 23, 2018

  • collect thing as a struct.
  • reduce redundant calculation.
  • don't rely on statsInfo.

@winoros
Copy link
Member Author

winoros commented Apr 23, 2018

simple benchmark result
previous:

BenchmarkOptimize/select_count(*)_from_t_group_by_e-8         	   30000	     47771 ns/op	   38058 B/op	     500 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_e_<=_10_group_by_e-8         	   20000	    106329 ns/op	   51307 B/op	     730 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_e_<=_50-8                    	   20000	     73301 ns/op	   36746 B/op	     561 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_c_>_'1'_group_by_b-8         	   10000	    104546 ns/op	   55571 B/op	     811 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_e_=_1_group_by_b-8           	   10000	    103423 ns/op	   59859 B/op	     871 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_e_>_1_group_by_b-8           	   10000	    117617 ns/op	   62900 B/op	     888 allocs/op
BenchmarkOptimize/select_count(e)_from_t_where_t.b_<=_20-8                  	   10000	    101968 ns/op	   60443 B/op	     848 allocs/op
BenchmarkOptimize/select_count(e)_from_t_where_t.b_<=_30-8                  	   10000	    112738 ns/op	   60443 B/op	     848 allocs/op
BenchmarkOptimize/select_count(e)_from_t_where_t.b_<=_40-8                  	   10000	    102269 ns/op	   60443 B/op	     848 allocs/op
BenchmarkOptimize/select_count(e)_from_t_where_t.b_<=_50-8                  	   10000	    103319 ns/op	   60443 B/op	     848 allocs/op
BenchmarkOptimize/select_*_from_t_where_t.b_<=_40-8                         	   30000	     47411 ns/op	   27488 B/op	     359 allocs/op
BenchmarkOptimize/select_*_from_t_where_t.b_<=_50-8                         	   30000	     49996 ns/op	   27488 B/op	     359 allocs/op
BenchmarkOptimize/select_*_from_t_where_1_and_t.b_<=_50-8                   	   30000	     46535 ns/op	   27696 B/op	     364 allocs/op
BenchmarkOptimize/select_*_from_t_where_t.b_<=_100_order_by_t.a_limit_1-8   	   10000	    191054 ns/op	  119193 B/op	    1527 allocs/op
BenchmarkOptimize/select_*_from_t_where_t.b_<=_1_order_by_t.a_limit_10-8    	   10000	    181012 ns/op	  119192 B/op	    1527 allocs/op
BenchmarkOptimize/select_*_from_t_use_index(b)_where_b_=_1_order_by_a-8     	   30000	     39580 ns/op	   24854 B/op	     350 allocs/op
BenchmarkOptimize/select_*_from_t_where_d_<_cast('1991-09-05'_as_datetime)-8         	   30000	     42636 ns/op	   21480 B/op	     296 allocs/op
plan: calc `index path` when doing `deriveStats`.
BenchmarkOptimize/select_*_from_t_where_ts_<_'1991-09-05'-8                          	   30000	     44727 ns/op	   21384 B/op	     294 allocs/op

Now:

BenchmarkOptimize/select_count(*)_from_t_group_by_e-8         	   30000	     46541 ns/op	   39234 B/op	     495 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_e_<=_10_group_by_e-8         	   20000	     68654 ns/op	   47858 B/op	     645 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_e_<=_50-8                    	   30000	     55205 ns/op	   33458 B/op	     492 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_c_>_'1'_group_by_b-8         	   20000	     83212 ns/op	   54291 B/op	     744 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_e_=_1_group_by_b-8           	   20000	     80127 ns/op	   57179 B/op	     750 allocs/op
BenchmarkOptimize/select_count(*)_from_t_where_e_>_1_group_by_b-8           	   20000	     90272 ns/op	   58171 B/op	     755 allocs/op
BenchmarkOptimize/select_count(e)_from_t_where_t.b_<=_20-8                  	   20000	     80168 ns/op	   52451 B/op	     693 allocs/op
BenchmarkOptimize/select_count(e)_from_t_where_t.b_<=_30-8                  	   20000	     77870 ns/op	   52451 B/op	     693 allocs/op
BenchmarkOptimize/select_count(e)_from_t_where_t.b_<=_40-8                  	   20000	     79757 ns/op	   52451 B/op	     693 allocs/op
BenchmarkOptimize/select_count(e)_from_t_where_t.b_<=_50-8                  	   20000	     80678 ns/op	   52450 B/op	     693 allocs/op
BenchmarkOptimize/select_*_from_t_where_t.b_<=_40-8                         	   30000	     52173 ns/op	   30704 B/op	     447 allocs/op
BenchmarkOptimize/select_*_from_t_where_t.b_<=_50-8                         	   30000	     54103 ns/op	   30704 B/op	     447 allocs/op
BenchmarkOptimize/select_*_from_t_where_1_and_t.b_<=_50-8                   	   30000	     53793 ns/op	   30912 B/op	     452 allocs/op
BenchmarkOptimize/select_*_from_t_where_t.b_<=_100_order_by_t.a_limit_1-8   	   10000	    111318 ns/op	   98032 B/op	    1033 allocs/op
BenchmarkOptimize/select_*_from_t_where_t.b_<=_1_order_by_t.a_limit_10-8    	   10000	    115871 ns/op	   98032 B/op	    1033 allocs/op
BenchmarkOptimize/select_*_from_t_use_index(b)_where_b_=_1_order_by_a-8     	   30000	     38680 ns/op	   25318 B/op	     338 allocs/op
BenchmarkOptimize/select_*_from_t_where_d_<_cast('1991-09-05'_as_datetime)-8         	   30000	     52790 ns/op	   25760 B/op	     410 allocs/op
BenchmarkOptimize/select_*_from_t_where_ts_<_'1991-09-05'-8                          	   30000	     58391 ns/op	   25664 B/op	     408 allocs/op

If the sql have props to push down, it will reduce a lot of time.
But if there's no prop, it will cost more because all index calculation is done.
We can use a lazy calcultion to optimize this.

@winoros
Copy link
Member Author

winoros commented Apr 23, 2018

sysbench
select: 2% improve.
oltp: not changed too much.

@winoros
Copy link
Member Author

winoros commented Apr 23, 2018

PTAL @zz-jason @coocood @XuHuaiyu

@winoros
Copy link
Member Author

winoros commented Apr 23, 2018

/run-all-tests

type availableIndices struct {
indices []*model.IndexInfo
includeTableScan bool
type indexPath struct {
Copy link
Member

Choose a reason for hiding this comment

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

Please add comments for this type.

// availableIndices is used for storing result of availableIndices function.
availableIndices *availableIndices
// possibleIndexPaths stores all the possible index path for physical plan, including table scan.
// Please make true table scan is always the first element.
Copy link
Member

Choose a reason for hiding this comment

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

make sure

eqCondCount int
indexFilters []expression.Expression
tableFilters []expression.Expression
filterUnmatched bool
Copy link
Member

Choose a reason for hiding this comment

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

not used?

index *model.IndexInfo
ranges []*ranger.NewRange
countAfterAccess float64
countAfterIndex float64
Copy link
Member

Choose a reason for hiding this comment

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

Add comments about the two count.

plan/stats.go Outdated
path.countAfterAccess = float64(ds.statisticTable.Count)
path.countAfterIndex = float64(ds.statisticTable.Count)
var err error
if path.isRowID {
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract a function for table path and another function for index path?

types/datum.go Outdated
@@ -122,6 +122,11 @@ func (d *Datum) IsNull() bool {
return d.k == KindNull
}

// IsMaxValue checks if datum is max value.
func (d *Datum) IsMaxValue() bool {
Copy link
Member

Choose a reason for hiding this comment

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

Not used?

@shenli
Copy link
Member

shenli commented Apr 23, 2018

/run-all-tests

@winoros winoros changed the title plan: calc index path when doing deriveStats. plan: calc access path when doing deriveStats. Apr 24, 2018
// possibleIndexPaths stores all the possible index path for physical plan, including table scan.
// Please make true table scan is always the first element.
possibleIndexPaths []*indexPath
// possibleAccessPaths stores all the possible index path for physical plan, including table scan.
Copy link
Member

Choose a reason for hiding this comment

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

all the possible access paths

forced bool
}

func (ds *DataSource) prepareTablePath(path *accessPath) error {
Copy link
Member

Choose a reason for hiding this comment

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

Since it's called by deriveStats, How about renaming it to deriveTablePathStats?

return nil
}

func (ds *DataSource) prepareIndexPath(path *accessPath) error {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -308,25 +310,10 @@ func (ds *DataSource) convertToIndexScan(prop *requiredProp, idx *model.IndexInf
is.Hist = &statsTbl.Indices[idx.ID].Histogram
}
rowCount := float64(statsTbl.Count)
Copy link
Member

Choose a reason for hiding this comment

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

can be removed

plan/stats.go Outdated
// PushDownNot here can convert query 'not (a != 1)' to 'a = 1'.
for i, expr := range ds.pushedDownConds {
ds.pushedDownConds[i] = expression.PushDownNot(nil, expr, false)
}
ds.statsAfterSelect = ds.getStatsByFilter(ds.pushedDownConds)
return ds.statsAfterSelect
for _, path := range ds.possibleAccessPaths {
var err error
Copy link
Member

Choose a reason for hiding this comment

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

we can remove this line and use err := in line 135 and 141

t = idxTask
if tblTask.cost() < t.cost() {
t = tblTask
}
Copy link
Member

Choose a reason for hiding this comment

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

how about adding a continue in this code block ?

} else {
path.ranges = ranger.FullIntNewRange(false)
}
path.countAfterAccess = float64(ds.statisticTable.Count)
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated with line 333?

} else {
path.indexFilters, path.tableFilters = splitIndexFilterConditions(ds.pushedDownConds, path.index.Columns, ds.tableInfo)
}
path.countAfterIndex = path.countAfterAccess
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated with line 364?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed one.

includeTableScan := x.availableIndices.includeTableScan
if includeTableScan && len(innerJoinKeys) == 1 {
indexPaths := x.possibleAccessPaths
if len(x.possibleAccessPaths) > 0 && x.possibleAccessPaths[0].isRowID {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use len(indexPaths) > 0 ...

sc := ds.ctx.GetSessionVars().StmtCtx
path.countAfterAccess = float64(ds.statisticTable.Count)
var pkCol *expression.Column
if pkCol != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

when will pkCol != nil?

}
path.countAfterAccess = float64(ds.statisticTable.Count)
if len(ds.pushedDownConds) > 0 {
if pkCol != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

if len(ds.pushedDownConds) > 0 {
if pkCol != nil {
path.accessConds, path.tableFilters = ranger.DetachCondsForTableRange(ds.ctx, ds.pushedDownConds, pkCol)
path.ranges, err = ranger.BuildTableRange(path.accessConds, sc, pkCol.RetType)
Copy link
Contributor

Choose a reason for hiding this comment

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

We may put sc as the first parameter.

type availableIndices struct {
indices []*model.IndexInfo
includeTableScan bool
func (ds *DataSource) prepareTablePath(path *accessPath) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be more concise?

func (ds *DataSource) prepareTablePath(path *accessPath) error {
	var err error
	var pkCol *expression.Column
	sc := ds.ctx.GetSessionVars().StmtCtx
	path.countAfterAccess = float64(ds.statisticTable.Count)
	path.ranges = ranger.FullIntNewRange(false)
	path.tableFilters = ds.pushedDownConds
	if pkCol == nil {
		return nil
	}
	path.ranges = ranger.FullIntNewRange(mysql.HasUnsignedFlag(pkCol.RetType.Flag))
	if len(ds.pushedDownConds) == 0 {
		return nil
	}
	path.accessConds, path.tableFilters = ranger.DetachCondsForTableRange(ds.ctx, ds.pushedDownConds, pkCol)
	path.ranges, err = ranger.BuildTableRange(path.accessConds, sc, pkCol.RetType)
	if err != nil {
		return errors.Trace(err)
	}
	path.countAfterAccess, err = ds.statisticTable.GetRowCountByIntColumnRanges(sc, pkCol.ID, path.ranges)
	return errors.Trace(err)
}

@winoros
Copy link
Member Author

winoros commented Apr 25, 2018

PTAL @coocood @zz-jason @XuHuaiyu @lamxTyler

@winoros
Copy link
Member Author

winoros commented Apr 25, 2018

/run-all-tests

includeTableScan := x.availableIndices.includeTableScan
if includeTableScan && len(innerJoinKeys) == 1 {
accessPaths := x.possibleAccessPaths
if len(accessPaths) > 0 && x.possibleAccessPaths[0].isRowID {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to check the length of innerJoinKeys?

@iamxy
Copy link
Member

iamxy commented Apr 26, 2018

/run-all-tests

// PushDownNot here can convert query 'not (a != 1)' to 'a = 1'.
for i, expr := range ds.pushedDownConds {
ds.pushedDownConds[i] = expression.PushDownNot(nil, expr, false)
}
ds.statsAfterSelect = ds.getStatsByFilter(ds.pushedDownConds)
return ds.statsAfterSelect
for _, path := range ds.possibleAccessPaths {
Copy link
Member

Choose a reason for hiding this comment

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

how about:

if !ds. possibleAccessPaths[0].isRowID {
    panic("table path should always be the first access path"
}
err := ds.deriveTablePathStats(path)
...
for _, path := range ds.possibleAccessPaths[1:] {
    err := ds.deriveIndexPathStats(path)
    ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the first element can be index path.

cols, _ := expression.IndexInfo2Cols(ds.schema.Columns, idx)
if len(cols) > 0 {
result = append(result, cols)
for _, path := range ds.possibleAccessPaths {
Copy link
Member

Choose a reason for hiding this comment

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

how about:

if !ds.possibleAccessPaths[0].isRowID {
    panic("table path should always be the first access path")
}
col := ds.getPKIsHandleCol()
...
for _, path := range ds.possibleAccessPaths[1:] {
    cols, _ := expression.IndexInfo2Cols(ds.schema.Columns, path.index)
    ...
}

innerPlan := x.forceToTableScan(pkCol)
return p.constructIndexJoin(prop, innerJoinKeys, outerJoinKeys, outerIdx, innerPlan, nil, nil)
accessPaths := x.possibleAccessPaths
if len(accessPaths) > 0 && x.possibleAccessPaths[0].isRowID {
Copy link
Member

Choose a reason for hiding this comment

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

we can just:

if len(accessPaths) > 0 {
    ...
}

if !ds.relevantIndices[i] && len(prop.cols) == 0 {
continue

for _, path := range ds.possibleAccessPaths {
Copy link
Member

Choose a reason for hiding this comment

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

how about:

if !ds.possibleAccessPaths[0].isRowID {
    panic("xxx")
}
tblTask, err := ds.convertToTableScan(prop, ds.possibleAccessPaths[0])
...
for _, path := range ds.possibleAccessPaths[1:] {
    if !(len(path.accessConds) > 0 || len(prop.cols) > 0 || path.forced) {
        continue
    }
    ...
}

is.filterCondition = ds.pushedDownConds
}
}
is.AccessCondition, is.Ranges, is.filterCondition, eqCount = path.accessConds, path.ranges, path.indexFilters, path.eqCondCount
Copy link
Member

Choose a reason for hiding this comment

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

this line is too long, can we split it to multi-lines ?

if pkCol != nil {
path.ranges = ranger.FullIntRange(mysql.HasUnsignedFlag(pkCol.RetType.Flag))
} else {
if pkCol == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We'better add some test cases to cover pkCol == nil and pkCol !=nil.

return errors.Trace(err)
}
path.countAfterAccess, err = ds.statisticTable.GetRowCountByIntColumnRanges(sc, pkCol.ID, path.ranges)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to check,
return errors.Trace(err) is ok.

innerPlan := x.forceToTableScan(pkCol)
return p.constructIndexJoin(prop, innerJoinKeys, outerJoinKeys, outerIdx, innerPlan, nil, nil)
accessPaths := x.possibleAccessPaths
if len(accessPaths) > 0 && x.possibleAccessPaths[0].isRowID {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ x.possibleAccessPaths[0]/ accessPaths[0]

indexFilters []expression.Expression
tableFilters []expression.Expression
// isRowID indicates this path stores the information for table scan.
isRowID bool
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just name it as isTablePath?

func getAvailableIndices(indexHints []*ast.IndexHint, tableInfo *model.TableInfo) (*availableIndices, error) {
publicIndices := make([]*model.IndexInfo, 0, len(tableInfo.Indices))
for _, index := range tableInfo.Indices {
func matchPathByIndexName(paths []*accessPath, idxName model.CIStr) *accessPath {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think matchXxx may return a bool value,
s/match/get

// If we have got "FORCE" or "USE" index hint but got no available index,
// we have to use table scan.
return &availableIndices{available, len(available) == 0}, nil
if len(available) == 0 {
available = append(available, &accessPath{isRowID: true})
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 just use publicPath[0] here without building a new accessPath ?

continue
}
// Currently we don't distinguish between "FORCE" and "USE" because
// our cost estimation is not reliable.
hasUseOrForce = true
available = append(available, idx)
path.forced = true
available = append(available, path)
}
}

if !hasScanHint {
Copy link
Contributor

Choose a reason for hiding this comment

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

if !hasScanHint || !hasUseOrForce {
    available = publicPaths
}
available = removeIgnorePaths(available, ignored)
if len(available) == 0 {
    ....
} 
return avaibale, nil

tableFilters []expression.Expression
// isRowID indicates this path stores the information for table scan.
isRowID bool
// forced means this index is generated by `use/force index()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ this index/ this path

eqCondCount int
indexFilters []expression.Expression
tableFilters []expression.Expression
// isRowID indicates this path stores the information for table scan.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ this path stores the information for table scan/ whether this path is a table path

// availableIndices is used for storing result of availableIndices function.
availableIndices *availableIndices
// possibleAccessPaths stores all the possible access path for physical plan, including table scan.
// Please make sure table path is always the first element.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment that:
Note: The first element is `not` always the table path.

idxTask, err := ds.convertToIndexScan(prop, idx)
continue
}
if len(path.accessConds) > 0 || len(prop.cols) > 0 || path.forced {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Please add a comment on this check.
  2. path.forced is checked only here,
    we do not really need this attribute,
    since if an indexPath is involved, path.forced must be true.

Copy link
Member Author

Choose a reason for hiding this comment

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

But, path.accessConds and prop.cols may be nil.
Such as select * from t use index(idx); We need to choose idx, but there' no access cond and prop.

plan/stats.go Outdated
return p.stats
var err error
p.stats, err = p.children[0].deriveStats()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

return p.stats, errors.Trace(err)

@XuHuaiyu
Copy link
Contributor

@winoros Please address the above comments.

// availableIndices is used for storing result of availableIndices function.
availableIndices *availableIndices
// possibleAccessPaths stores all the possible access path for physical plan, including table scan.
// Please make sure table path is always the first element if we have table path.
Copy link
Member

Choose a reason for hiding this comment

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

If we check isTablePath in each path, then why do we need to make sure the table path is the first?

Copy link
Member Author

Choose a reason for hiding this comment

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

For index join, if there's pk and match the condition, we will directly choose it.
Use a loop to find the tablePath first.
So i removed this constraint.

pkCol = expression.ColInfo2Col(ds.schema.Columns, pkColInfo)
}
}
if pkCol == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We'better add some test cases to cover pkCol == nil and pkCol !=nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

the status of pkCol is covered executor's unit test

Copy link
Contributor

Choose a reason for hiding this comment

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

But why all test cases can be passed when line 336~340 was missed before,
we may need some test for checking this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

let me check it first.

innerPlan := x.forceToTableScan(pkCol)
return p.constructIndexJoin(prop, innerJoinKeys, outerJoinKeys, outerIdx, innerPlan, nil, nil)
accessPaths := x.possibleAccessPaths
if len(accessPaths) > 0 && accessPaths[0].isTablePath {
Copy link
Contributor

Choose a reason for hiding this comment

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

when will len(accessPaths) == 0?

for i, path := range accessPaths {
if path.isTablePath {
tblPath = path
accessPaths = append(accessPaths[:i], accessPaths[i+1:]...)
Copy link
Member

Choose a reason for hiding this comment

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

This operation modifies possibleAccessPaths.

Copy link
Member

@coocood coocood Apr 28, 2018

Choose a reason for hiding this comment

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

We just iterate possibleAccessPaths and ignore table path at line 278.

// We will use index to generate physical plan if:
// this path's access cond is not nil or
// we have prop to match or
// this index is force to choose.
Copy link
Member

Choose a reason for hiding this comment

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

is forced

col := ds.getPKIsHandleCol()
if col != nil {
result = append(result, []*expression.Column{col})
}
Copy link
Member

Choose a reason for hiding this comment

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

add a continue here so we do not need the following else

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

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@coocood coocood added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 28, 2018
@zz-jason zz-jason merged commit 50426a9 into pingcap:master Apr 28, 2018
@winoros winoros deleted the move-index-calc-front branch May 15, 2018 07:55
winoros added a commit to winoros/tidb that referenced this pull request Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants