-
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
Changes from all commits
21878f3
493d553
db14cbf
191a07d
379e55c
7409e6e
c5cdc13
643b09c
cb3b927
c542330
8dbdb68
b5bfe43
1ea1dc1
6d85229
2392fb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,195 @@ | ||
// Copyright 2018 PingCAP, Inc. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain col1 copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package plan | ||
|
||
import ( | ||
"fmt" | ||
|
||
"github.com/juju/errors" | ||
. "github.com/pingcap/check" | ||
"github.com/pingcap/tidb/ast" | ||
"github.com/pingcap/tidb/expression" | ||
"github.com/pingcap/tidb/model" | ||
"github.com/pingcap/tidb/mysql" | ||
"github.com/pingcap/tidb/sessionctx" | ||
"github.com/pingcap/tidb/types" | ||
"github.com/pingcap/tidb/util/testleak" | ||
) | ||
|
||
var _ = Suite(&testUnitTestSuit{}) | ||
|
||
type testUnitTestSuit struct { | ||
ctx sessionctx.Context | ||
} | ||
|
||
func (s *testUnitTestSuit) SetUpSuite(c *C) { | ||
s.ctx = mockContext() | ||
} | ||
|
||
func (s *testUnitTestSuit) newTypeWithFlen(typeByte byte, flen int) *types.FieldType { | ||
tp := types.NewFieldType(typeByte) | ||
tp.Flen = flen | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. We do no need to return error? |
||
switch x := expr.(type) { | ||
case *expression.ScalarFunction: | ||
newArgs := make([]expression.Expression, 0, len(x.GetArgs())) | ||
for _, arg := range x.GetArgs() { | ||
newArg, err := s.SubstituteCol2CorCol(arg, colIDs) | ||
if err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
newArgs = append(newArgs, newArg) | ||
} | ||
newSf, err := expression.NewFunction(x.GetCtx(), x.FuncName.L, x.GetType(), newArgs...) | ||
return newSf, errors.Trace(err) | ||
case *expression.Column: | ||
if _, ok := colIDs[x.UniqueID]; ok { | ||
return &expression.CorrelatedColumn{Column: *x}, nil | ||
} | ||
} | ||
return expr, nil | ||
} | ||
|
||
func (s *testUnitTestSuit) TestIndexPathSplitCorColCond(c *C) { | ||
defer testleak.AfterTest(c)() | ||
totalSchema := expression.NewSchema() | ||
totalSchema.Append(&expression.Column{ | ||
ColName: model.NewCIStr("col1"), | ||
UniqueID: 1, | ||
RetType: types.NewFieldType(mysql.TypeLonglong), | ||
}) | ||
totalSchema.Append(&expression.Column{ | ||
ColName: model.NewCIStr("col2"), | ||
UniqueID: 2, | ||
RetType: types.NewFieldType(mysql.TypeLonglong), | ||
}) | ||
totalSchema.Append(&expression.Column{ | ||
ColName: model.NewCIStr("col3"), | ||
UniqueID: 3, | ||
RetType: s.newTypeWithFlen(mysql.TypeVarchar, 10), | ||
}) | ||
totalSchema.Append(&expression.Column{ | ||
ColName: model.NewCIStr("col4"), | ||
UniqueID: 4, | ||
RetType: s.newTypeWithFlen(mysql.TypeVarchar, 10), | ||
}) | ||
totalSchema.Append(&expression.Column{ | ||
ColName: model.NewCIStr("col5"), | ||
UniqueID: 5, | ||
RetType: types.NewFieldType(mysql.TypeLonglong), | ||
}) | ||
testCases := []struct { | ||
expr string | ||
corColIDs []int | ||
idxColIDs []int | ||
idxColLens []int | ||
access string | ||
remained string | ||
}{ | ||
{ | ||
expr: "col1 = col2", | ||
corColIDs: []int{2}, | ||
idxColIDs: []int{1}, | ||
idxColLens: []int{types.UnspecifiedLength}, | ||
access: "[eq(col1, col2)]", | ||
remained: "[]", | ||
}, | ||
{ | ||
expr: "col1 = col5 and col2 = 1", | ||
corColIDs: []int{5}, | ||
idxColIDs: []int{1, 2}, | ||
idxColLens: []int{types.UnspecifiedLength, types.UnspecifiedLength}, | ||
access: "[eq(col1, col5) eq(col2, 1)]", | ||
remained: "[]", | ||
}, | ||
{ | ||
expr: "col1 = col5 and col2 = 1", | ||
corColIDs: []int{5}, | ||
idxColIDs: []int{2, 1}, | ||
idxColLens: []int{types.UnspecifiedLength, types.UnspecifiedLength}, | ||
access: "[eq(col2, 1) eq(col1, col5)]", | ||
remained: "[]", | ||
}, | ||
{ | ||
expr: "col1 = col5 and col2 = 1", | ||
corColIDs: []int{5}, | ||
idxColIDs: []int{1}, | ||
idxColLens: []int{types.UnspecifiedLength}, | ||
access: "[eq(col1, col5)]", | ||
remained: "[eq(col2, 1)]", | ||
}, | ||
{ | ||
expr: "col2 = 1 and col1 = col5", | ||
corColIDs: []int{5}, | ||
idxColIDs: []int{1}, | ||
idxColLens: []int{types.UnspecifiedLength}, | ||
access: "[eq(col1, col5)]", | ||
remained: "[eq(col2, 1)]", | ||
}, | ||
{ | ||
expr: "col1 = col2 and col3 = col4 and col5 = 1", | ||
corColIDs: []int{2, 4}, | ||
idxColIDs: []int{1, 3}, | ||
idxColLens: []int{types.UnspecifiedLength, types.UnspecifiedLength}, | ||
access: "[eq(col1, col2) eq(col3, col4)]", | ||
remained: "[eq(col5, 1)]", | ||
}, | ||
{ | ||
expr: "col1 = col2 and col3 = col4 and col5 = 1", | ||
corColIDs: []int{2, 4}, | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Because it's a prefix index. |
||
}, | ||
{ | ||
expr: `col1 = col5 and col3 = "col1" and col2 = col5`, | ||
corColIDs: []int{5}, | ||
idxColIDs: []int{1, 2, 3}, | ||
idxColLens: []int{types.UnspecifiedLength, types.UnspecifiedLength, types.UnspecifiedLength}, | ||
access: "[eq(col1, col5) eq(col2, col5) eq(col3, col1)]", | ||
remained: "[]", | ||
}, | ||
} | ||
for _, tt := range testCases { | ||
comment := Commentf("failed at case:\nexpr: %v\ncorColIDs: %v\nidxColIDs: %v\nidxColLens: %v\naccess: %v\nremained: %v\n", tt.expr, tt.corColIDs, tt.idxColIDs, tt.idxColLens, tt.access, tt.remained) | ||
filters, err := expression.ParseSimpleExprsWithSchema(s.ctx, tt.expr, totalSchema) | ||
if sf, ok := filters[0].(*expression.ScalarFunction); ok && sf.FuncName.L == ast.LogicAnd { | ||
filters = expression.FlattenCNFConditions(sf) | ||
} | ||
c.Assert(err, IsNil, comment) | ||
trueFilters := make([]expression.Expression, 0, len(filters)) | ||
idMap := make(map[int]struct{}) | ||
for _, id := range tt.corColIDs { | ||
idMap[id] = struct{}{} | ||
} | ||
for _, filter := range filters { | ||
trueFilter, err := s.SubstituteCol2CorCol(filter, idMap) | ||
c.Assert(err, IsNil, comment) | ||
trueFilters = append(trueFilters, trueFilter) | ||
} | ||
path := accessPath{ | ||
eqCondCount: 0, | ||
tableFilters: trueFilters, | ||
idxCols: expression.FindColumnsByUniqueIDs(totalSchema.Columns, tt.idxColIDs), | ||
idxColLens: tt.idxColLens, | ||
} | ||
access, remained := path.splitCorColAccessCondFromFilters() | ||
c.Assert(fmt.Sprintf("%s", access), Equals, tt.access, comment) | ||
c.Assert(fmt.Sprintf("%s", remained), Equals, tt.remained, comment) | ||
} | ||
} |
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