plan: fix a bug when using correlated column as index #7357

winoros · 2018-08-10T12:07:29Z

What problem does this PR solve?

The for loop was wrongly breaked. Hence there may be case that we misjudged.

In the for loop, we check that whether this index column have corresponding equal condition which has correlated column.
So we should break once we found it. If one condition is not matched, we should continue to next condition rather than breaking the loop.

What is changed and how it works?

Break the for loop at the correct position.

Check List

Tests

Unit test

Will be added later.

Related changes

Need to be included in the release note

Fix the case that index may not use correlated column correctly.

zz-jason · 2018-08-10T12:10:19Z

plan/logical_plans.go

@@ -484,14 +484,15 @@ func (path *accessPath) splitCorColAccessCondFromFilters() (access, remained []e
 	for i := path.eqCondCount; i < len(path.idxCols); i++ {
 		matched := false
 		for j, filter := range path.tableFilters {
-			if !isColEqCorColOrConstant(filter, path.idxCols[i]) {
-				break
+			if used[j] || !isColEqCorColOrConstant(filter, path.idxCols[i]) {


It's better to add a UT for function splitCorColAccessCondFromFilters though we already have integration tests, with UT we can test some corner cases that are not easy to be covered.

I think this function is not big enough and not special enough to add test that tests this method seperately.
This is involved in how should we add test in plan package.

If we have UT for this function, even without the integration test you added in this PR, we can avoid this bug in the very beginning.

You can see the advantage of unit test through Wikipedia: https://en.wikipedia.org/wiki/Unit_testing#Advantages

zz-jason

need UT

zz-jason · 2018-08-14T13:47:25Z

plan/logical_plans_test.go

+	defer testleak.AfterTest(c)()
+	totalSchema := expression.NewSchema()
+	totalSchema.Append(&expression.Column{
+		ColName:  model.NewCIStr("a"),


I think a better way is to:

s/a/col1/ s/b/col2/ s/c/col3/ s/d/col4/ s/e/col5/

It helps us understanding the test case.

zz-jason · 2018-08-14T13:51:45Z

expression/column.go

+			}
+		}
+		if !found {
+			break


This check can be removed. we can make this function to find all the columns in the unique id list.

But we still need this check. In such case, we need to return nil or a full slice?

The function caller can check whether there is any column can not be found in cols by:

len(ids) == len(retCols)

Another consideration is, this function is only used in the test currently, all the column identified by ids can be found in cols. We can extend this function in the future when we meet more complicated scenarios. For now, I think we'd better to keep it simple.

zz-jason

LGTM

need one more reviewer

zz-jason · 2018-08-15T09:07:42Z

@XuHuaiyu @lamxTyler PTAL

alivxxx · 2018-08-16T03:28:57Z

plan/logical_plans_test.go

@@ -0,0 +1,197 @@
+// Copyright 2016 PingCAP, Inc.


alivxxx · 2018-08-16T05:33:17Z

plan/logical_plans_test.go

+	ctx sessionctx.Context
+}
+
+func (s *testUnitTestSuit) SetUpSuite(col3 *C) {


Why name it col3?

alivxxx · 2018-08-16T05:33:29Z

plan/logical_plans_test.go

+	return expr, nil
+}
+
+func (s *testUnitTestSuit) TestIndexPathSplitCorColCond(col3 *C) {


crazycs520 · 2018-08-21T05:56:30Z

plan/logical_plans_test.go

+		if _, ok := colIDs[x.UniqueID]; ok {
+			return &expression.CorrelatedColumn{Column: *x}, nil
+		}
+	default:


redundant default？ line#66 is enough.

winoros · 2018-08-21T10:32:13Z

/run-unit-test

zz-jason · 2018-08-21T10:39:02Z

/run-all-tests

zz-jason

LGTM

zz-jason · 2018-08-24T05:39:40Z

@crazycs520 @XuHuaiyu PTAL

XuHuaiyu · 2018-08-24T08:02:36Z

expression/column.go

@@ -375,3 +375,18 @@ func IndexInfo2Cols(cols []*Column, index *model.IndexInfo) ([]*Column, []int) {
 	}
 	return retCols, lengths
 }
+
+// FindColumnsByUniqueIDs will find columns by checking the unique id.
+// Note: This id list must be a subset of the column slice.


What does This id list mean?

Should we change it to `ids`?

XuHuaiyu · 2018-08-24T08:15:42Z

plan/logical_plans_test.go

+	return tp
+}
+
+func (s *testUnitTestSuit) SubstituteCol2CorCol(expr expression.Expression, colIDs map[int]struct{}) (expression.Expression, error) {


We do no need to return error?

XuHuaiyu · 2018-08-24T08:20:47Z

plan/logical_plans_test.go

+			idxColIDs:  []int{1, 3},
+			idxColLens: []int{types.UnspecifiedLength, 2},
+			access:     "[eq(col1, col2) eq(col3, col4)]",
+			remained:   "[eq(col3, col4) eq(col5, 1)]",


Why eq(col3 col4) be access and remained at the same time?

Because it's a prefix index.

XuHuaiyu

LGMT

winoros added 3 commits August 10, 2018 17:04

plan: fix a bug when using correlated column as index

21878f3

don't check the already marked filter

493d553

add test

db14cbf

winoros added type/bugfix This PR fixes a bug. sig/planner SIG: Planner release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 10, 2018

winoros requested review from zz-jason, XuHuaiyu and alivxxx August 10, 2018 12:07

zz-jason reviewed Aug 10, 2018

View reviewed changes

zz-jason requested changes Aug 10, 2018

View reviewed changes

winoros added 2 commits August 14, 2018 15:28

Merge branch 'master' into corcol-bug

191a07d

add test.

379e55c

zz-jason reviewed Aug 14, 2018

View reviewed changes

winoros added 2 commits August 15, 2018 15:55

address comment.

7409e6e

address comments.

c5cdc13

winoros force-pushed the corcol-bug branch from 799c765 to c5cdc13 Compare August 15, 2018 08:55

tiny change

643b09c

zz-jason previously approved these changes Aug 15, 2018

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 15, 2018

alivxxx reviewed Aug 16, 2018

View reviewed changes

winoros force-pushed the corcol-bug branch from f5be520 to 28a81ba Compare August 20, 2018 12:46

typo

cb3b927

winoros force-pushed the corcol-bug branch from 28a81ba to cb3b927 Compare August 21, 2018 05:06

crazycs520 reviewed Aug 21, 2018

View reviewed changes

address comment

c542330

Merge branch 'master' into corcol-bug

8dbdb68

winoros and others added 2 commits August 22, 2018 14:45

fix test

b5bfe43

Merge branch 'master' into corcol-bug

1ea1dc1

zz-jason previously approved these changes Aug 24, 2018

View reviewed changes

XuHuaiyu reviewed Aug 24, 2018

View reviewed changes

address comments

6d85229

winoros dismissed zz-jason’s stale review via 6d85229 August 24, 2018 08:27

XuHuaiyu approved these changes Aug 24, 2018

View reviewed changes

XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 24, 2018

Merge branch 'master' into corcol-bug

2392fb2

winoros merged commit d9f7ffe into pingcap:master Aug 24, 2018

winoros deleted the corcol-bug branch August 24, 2018 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plan: fix a bug when using correlated column as index #7357

plan: fix a bug when using correlated column as index #7357

winoros commented Aug 10, 2018

zz-jason Aug 10, 2018

winoros Aug 10, 2018

zz-jason Aug 10, 2018

zz-jason left a comment

zz-jason Aug 14, 2018

zz-jason Aug 14, 2018

winoros Aug 15, 2018

zz-jason Aug 15, 2018

zz-jason Aug 15, 2018

zz-jason left a comment

zz-jason commented Aug 15, 2018

alivxxx Aug 16, 2018

alivxxx Aug 16, 2018

alivxxx Aug 16, 2018

crazycs520 Aug 21, 2018

winoros commented Aug 21, 2018

zz-jason commented Aug 21, 2018

zz-jason left a comment

zz-jason commented Aug 24, 2018

XuHuaiyu Aug 24, 2018

XuHuaiyu Aug 24, 2018

XuHuaiyu Aug 24, 2018

winoros Aug 24, 2018

XuHuaiyu left a comment

plan: fix a bug when using correlated column as index #7357

plan: fix a bug when using correlated column as index #7357

Conversation

winoros commented Aug 10, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

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

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

zz-jason commented Aug 15, 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

winoros commented Aug 21, 2018

zz-jason commented Aug 21, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 24, 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

XuHuaiyu left a comment

Choose a reason for hiding this comment