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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 15 commits into from
Aug 24, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
21878f3
plan: fix a bug when using correlated column as index
winoros Aug 10, 2018
493d553
don't check the already marked filter
winoros Aug 10, 2018
db14cbf
add test
winoros Aug 10, 2018
191a07d
Merge branch 'master' into corcol-bug
winoros Aug 14, 2018
379e55c
add test.
winoros Aug 14, 2018
7409e6e
address comment.
winoros Aug 15, 2018
c5cdc13
address comments.
winoros Aug 15, 2018
643b09c
tiny change
winoros Aug 15, 2018
cb3b927
typo
winoros Aug 20, 2018
c542330
address comment
winoros Aug 21, 2018
8dbdb68
Merge branch 'master' into corcol-bug
zz-jason Aug 21, 2018
b5bfe43
fix test
winoros Aug 22, 2018
1ea1dc1
Merge branch 'master' into corcol-bug
zz-jason Aug 24, 2018
6d85229
address comments
winoros Aug 24, 2018
2392fb2
Merge branch 'master' into corcol-bug
winoros Aug 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion plan/cbo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ func (s *testAnalyzeSuite) TestCorrelatedEstimation(c *C) {
store.Close()
}()
tk.MustExec("use test")
tk.MustExec("create table t(a int, b int, c int)")
tk.MustExec("create table t(a int, b int, c int, index idx(c))")
tk.MustExec("insert into t values(1,1,1), (2,2,2), (3,3,3), (4,4,4), (5,5,5), (6,6,6), (7,7,7), (8,8,8), (9,9,9),(10,10,10)")
tk.MustExec("analyze table t")
tk.MustQuery("explain select t.c in (select count(*) from t s , t t1 where s.a = t.a and s.a = t1.a) from t;").
Expand All @@ -619,6 +619,18 @@ func (s *testAnalyzeSuite) TestCorrelatedEstimation(c *C) {
" └─TableReader_27 10.00 root data:TableScan_26",
" └─TableScan_26 10.00 cop table:t1, range:[-inf,+inf], keep order:false",
))
tk.MustQuery("explain select (select concat(t1.a, \",\", t1.b) from t t1 where t1.a=t.a and t1.c=t.c) from t").
Check(testkit.Rows(
"Projection_8 10.00 root concat(t1.a, \",\", t1.b)",
"└─Apply_10 10.00 root left outer join, inner:MaxOneRow_13",
" ├─TableReader_12 10.00 root data:TableScan_11",
" │ └─TableScan_11 10.00 cop table:t, range:[-inf,+inf], keep order:false",
" └─MaxOneRow_13 1.00 root ",
" └─Projection_14 0.80 root concat(cast(t1.a), \",\", cast(t1.b))",
" └─IndexLookUp_21 0.80 root ",
" ├─IndexScan_18 1.00 cop table:t1, index:c, range: decided by [eq(t1.c, test.t.c)], keep order:false",
" └─Selection_20 0.80 cop eq(t1.a, test.t.a)] [ └─TableScan_19 1.00 cop table:t, keep order:false",
))
}

func (s *testAnalyzeSuite) TestInconsistentEstimation(c *C) {
Expand Down
5 changes: 3 additions & 2 deletions plan/logical_plans.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {
Copy link
Member

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.

Copy link
Member Author

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.

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 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

continue
}
matched = true
access[i-path.eqCondCount] = filter
if path.idxColLens[i] == types.UnspecifiedLength {
used[j] = true
}
break
}
if !matched {
access = access[:i-path.eqCondCount]
Expand Down