plan: remove other `accessPath`s if one is unique key and full matched. #6925

winoros · 2018-06-28T05:59:27Z

What have you changed? (mandatory)

It's a conservative optimization.
If one unique key is full matched by point query. We'll simply remove other choices.

What are the type of the changes (mandatory)?

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested (mandatory)?

explain test added.

Add a few positive/negative examples (optional)

When the statistics is out of date. We may choose a wrong index. This optimization can avoid some bad situation.
And it's conservative enough that we can make sure the decision we made by this optimization is no worse than others.

shenli · 2018-06-28T06:08:06Z

plan/logical_plans.go

@@ -332,7 +332,7 @@ type accessPath struct {
 	forced bool
 }

-func (ds *DataSource) deriveTablePathStats(path *accessPath) error {
+func (ds *DataSource) deriveTablePathStats(path *accessPath) (bool, error) {


Add comment for the newly added return value.

shenli · 2018-06-28T06:08:28Z

plan/logical_plans.go

 }

-func (ds *DataSource) deriveIndexPathStats(path *accessPath) error {
+func (ds *DataSource) deriveIndexPathStats(path *accessPath) (bool, error) {


shenli · 2018-06-28T06:10:23Z

plan/stats.go

 			continue
 		}
-		err := ds.deriveIndexPathStats(path)
+		uniqueAndOnlyPoint, err := ds.deriveIndexPathStats(path)


pointQuery is enough.

I think we need to tell that this key is unique key.

shenli · 2018-06-28T06:11:12Z

plan/logical_plans.go

 	}
 	path.countAfterAccess, err = ds.statisticTable.GetRowCountByIntColumnRanges(sc, pkCol.ID, path.ranges)
-	return errors.Trace(err)
+	noIntervalRange := true


Add comments here.

how about s/noIntervalRange/allPointRanges/? This variable renaming can make code logic more clear.

zz-jason · 2018-06-28T06:39:27Z

plan/logical_plans.go

@@ -388,7 +395,7 @@ func (ds *DataSource) deriveIndexPathStats(path *accessPath) error {
 		}
 		path.countAfterIndex = math.Max(path.countAfterAccess*selectivity, ds.statsAfterSelect.count)
 	}
-	return nil
+	return path.index.Unique && path.eqCondCount == len(path.index.Columns), nil


It's better to only check whether the ranges are all point ranges, just like deriveTablePathStats. We can check path.index.Unique in deriveStats()

winoros · 2018-06-28T08:45:11Z

PTAL @shenli @zz-jason @coocood

zz-jason · 2018-06-28T11:23:29Z

plan/logical_plans.go

 	}
 	path.countAfterAccess, err = ds.statisticTable.GetRowCountByIntColumnRanges(sc, pkCol.ID, path.ranges)
-	return errors.Trace(err)
+	// Check whether the primary key is covered by point query.
+	noIntervalRange := true


s/noIntervalRange/allPointRanges/ or s/noIntervalRange/onlyPoint/

noIntervalRange is better. Since there may be the case that the range is empty.

Any example?

like a > 5 and a < 3...

zz-jason

LGTM

coocood · 2018-06-28T12:42:07Z

@winoros
What if the range is [<nil>, <nil>] or [1 <nil>, 1 <nil>]?

lysu · 2018-06-28T12:57:52Z

@coocood maybe be range query https://github.com/lysu/tidb/blob/12dbd3285495ed4606fe5bdd8763f3d0acaf73bb/util/ranger/types.go#L53, and It seems no problem?

lysu

LGTM

zz-jason · 2018-06-28T13:43:06Z

/run-all-tests

coocood

When we have a unique key a, and most of its value is NULL, the condition is a is NULL, the index path may not be the best.

shenli · 2018-06-29T02:58:44Z

@coocood What's is your suggestion? Add a constraint about the estimated row count? Such as if the accessPath.count < 100, then choose the unique key, else fall to the normal way.

coocood · 2018-06-29T03:03:35Z

@shenli
No, we can just exclude the case when the point range contains NULL.

…cut-off-paths

shenli · 2018-06-29T06:02:51Z

@coocood PTAL

…d. (pingcap#6925)

…d. (#6925) (#6966)

plan: remove other accessPaths if one is unique key and full matched.

d2b83a6

shenli reviewed Jun 28, 2018

View reviewed changes

shenli added sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement. labels Jun 28, 2018

winoros added 2 commits June 28, 2018 14:33

Merge branch 'master' into cut-off-paths

1aea370

fix test.

a352724

zz-jason reviewed Jun 28, 2018

View reviewed changes

zz-jason added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Jun 28, 2018

winoros added 3 commits June 28, 2018 14:48

address comments.

5ae90d3

address comments.

d5058ee

add explain test.

baa97cf

winoros added 2 commits June 28, 2018 17:52

Merge branch 'master' into cut-off-paths

d098c09

fix merge error.

4593783

zz-jason reviewed Jun 28, 2018

View reviewed changes

unify the name of variable.

929fc2e

zz-jason reviewed Jun 28, 2018

View reviewed changes

lysu reviewed Jun 28, 2018

View reviewed changes

Merge branch 'master' into cut-off-paths

c4770a0

zz-jason previously approved these changes Jun 28, 2018

View reviewed changes

zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Jun 28, 2018

coocood requested changes Jun 28, 2018

View reviewed changes

winoros added 2 commits June 29, 2018 13:38

address comments.

b71b70f

Merge branch 'cut-off-paths' of https://github.com/winoros/tidb into …

b385a40

…cut-off-paths

winoros dismissed zz-jason’s stale review via b385a40 June 29, 2018 05:39

Merge branch 'master' into cut-off-paths

3dd9749

coocood approved these changes Jun 29, 2018

View reviewed changes

coocood merged commit 6a931e5 into pingcap:master Jun 29, 2018

winoros deleted the cut-off-paths branch June 29, 2018 07:00

winoros added a commit to winoros/tidb that referenced this pull request Jul 3, 2018

plan: remove other accessPaths if one is unique key and full matche…

0aa88e9

…d. (pingcap#6925)

winoros mentioned this pull request Jul 3, 2018

plan: remove other accessPaths if one is unique key and full matched. (#6925) #6966

Merged

zz-jason pushed a commit that referenced this pull request Jul 3, 2018

plan: remove other accessPaths if one is unique key and full matche…

bfcea6d

…d. (#6925) (#6966)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plan: remove other `accessPath`s if one is unique key and full matched. #6925

plan: remove other `accessPath`s if one is unique key and full matched. #6925

winoros commented Jun 28, 2018 •

edited

Loading

shenli Jun 28, 2018

shenli Jun 28, 2018

shenli Jun 28, 2018

winoros Jun 28, 2018

shenli Jun 28, 2018

zz-jason Jun 28, 2018

shenli Jun 28, 2018

zz-jason Jun 28, 2018 •

edited

Loading

winoros commented Jun 28, 2018

zz-jason Jun 28, 2018

winoros Jun 28, 2018

zz-jason Jun 28, 2018

winoros Jun 28, 2018

zz-jason Jun 28, 2018

zz-jason left a comment

coocood commented Jun 28, 2018

lysu commented Jun 28, 2018 •

edited

Loading

lysu left a comment

zz-jason commented Jun 28, 2018

coocood left a comment

shenli commented Jun 29, 2018

coocood commented Jun 29, 2018 •

edited

Loading

shenli commented Jun 29, 2018

plan: remove other accessPaths if one is unique key and full matched. #6925

plan: remove other accessPaths if one is unique key and full matched. #6925

Conversation

winoros commented Jun 28, 2018 • edited Loading

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

Add a few positive/negative examples (optional)

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

Choose a reason for hiding this comment

zz-jason Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

winoros commented Jun 28, 2018

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

zz-jason left a comment

Choose a reason for hiding this comment

coocood commented Jun 28, 2018

lysu commented Jun 28, 2018 • edited Loading

lysu left a comment

Choose a reason for hiding this comment

zz-jason commented Jun 28, 2018

coocood left a comment

Choose a reason for hiding this comment

shenli commented Jun 29, 2018

coocood commented Jun 29, 2018 • edited Loading

shenli commented Jun 29, 2018

plan: remove other `accessPath`s if one is unique key and full matched. #6925

plan: remove other `accessPath`s if one is unique key and full matched. #6925

winoros commented Jun 28, 2018 •

edited

Loading

zz-jason Jun 28, 2018 •

edited

Loading

lysu commented Jun 28, 2018 •

edited

Loading

coocood commented Jun 29, 2018 •

edited

Loading