-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: fix a bug when using correlated column as index #7357
Conversation
@@ -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]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need UT
plan/logical_plans_test.go
Outdated
defer testleak.AfterTest(c)() | ||
totalSchema := expression.NewSchema() | ||
totalSchema.Append(&expression.Column{ | ||
ColName: model.NewCIStr("a"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
expression/column.go
Outdated
} | ||
} | ||
if !found { | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check can be removed. we can make this function to find all the columns in the unique id list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we still need this check. In such case, we need to return nil or a full slice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function caller can check whether there is any column can not be found in cols
by:
len(ids) == len(retCols)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@XuHuaiyu @lamxTyler PTAL |
plan/logical_plans_test.go
Outdated
@@ -0,0 +1,197 @@ | |||
// Copyright 2016 PingCAP, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2018
plan/logical_plans_test.go
Outdated
ctx sessionctx.Context | ||
} | ||
|
||
func (s *testUnitTestSuit) SetUpSuite(col3 *C) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why name it col3
?
plan/logical_plans_test.go
Outdated
return expr, nil | ||
} | ||
|
||
func (s *testUnitTestSuit) TestIndexPathSplitCorColCond(col3 *C) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
plan/logical_plans_test.go
Outdated
if _, ok := colIDs[x.UniqueID]; ok { | ||
return &expression.CorrelatedColumn{Column: *x}, nil | ||
} | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant default? line#66 is enough.
/run-unit-test |
/run-all-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@crazycs520 @XuHuaiyu PTAL |
expression/column.go
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does This id list
mean?
Should we change it to `ids`?
return tp | ||
} | ||
|
||
func (s *testUnitTestSuit) SubstituteCol2CorCol(expr expression.Expression, colIDs map[int]struct{}) (expression.Expression, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do no need to return error?
idxColIDs: []int{1, 3}, | ||
idxColLens: []int{types.UnspecifiedLength, 2}, | ||
access: "[eq(col1, col2) eq(col3, col4)]", | ||
remained: "[eq(col3, col4) eq(col5, 1)]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why eq(col3 col4) be access and remained at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's a prefix index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT
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
Will be added later.
Related changes
Fix the case that index may not use correlated column correctly.