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.

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

ddl: fix invalid index on multi-layer virtual columns #11260

Merged
merged 18 commits into from
Jul 24, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
30 changes: 30 additions & 0 deletions ddl/db_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -797,6 +797,36 @@ func (s *testIntegrationSuite5) TestModifyingColumnOption(c *C) {
assertErrCode("alter table t2 modify column c int references t1(a)", errMsg)
}

func (s *testIntegrationSuite1) TestIndexOnMultipleGeneratedColumn(c *C) {
tk := testkit.NewTestKit(c, s.store)

tk.MustExec("create database if not exists test")
tk.MustExec("use test")
tk.MustExec("drop table if exists t")
tk.MustExec("create table t (a int, b int as (a + 1), c int as (b + 1))")
tk.MustExec("insert into t (a) values (1)")
tk.MustExec("create index idx on t (c)")
tk.MustQuery("select * from t where c > 1").Check(testkit.Rows("1 2 3"))
tk.MustExec("admin check table t")

tk.MustExec("drop table if exists t")
tk.MustExec("create table t (a int, b int as (a + 1), c int as (b + 1), d int as (c + 1))")
tangenta marked this conversation as resolved.
Show resolved Hide resolved
tk.MustExec("insert into t (a) values (1)")
tk.MustExec("create index idx on t (d)")
tk.MustQuery("select * from t where d > 2").Check(testkit.Rows("1 2 3 4"))
tk.MustExec("admin check table t")

tk.MustExec("drop table if exists t")
tk.MustExec("create table t(a bigint, b bigint as (a+1) virtual, c bigint as (b+1) virtual)")
tk.MustExec("alter table t add index idx_b(b)")
tk.MustExec("alter table t add index idx_c(c)")
tk.MustExec("insert into t(a) values(1)")
tk.MustExec("alter table t add column(d bigint as (c+1) virtual)")
tk.MustExec("alter table t add index idx_d(d)")
tk.MustQuery("select * from t where d > 2").Check(testkit.Rows("1 2 3 4"))
tangenta marked this conversation as resolved.
Show resolved Hide resolved
tk.MustExec("admin check table t")
}

func (s *testIntegrationSuite2) TestCaseInsensitiveCharsetAndCollate(c *C) {
tk := testkit.NewTestKit(c, s.store)

Expand Down
45 changes: 33 additions & 12 deletions ddl/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -908,27 +908,48 @@ func (w *addIndexWorker) run(d *ddlCtx) {

func makeupDecodeColMap(sessCtx sessionctx.Context, t table.Table, indexInfo *model.IndexInfo) (map[int64]decoder.Column, error) {
cols := t.Cols()
decodeColMap := make(map[int64]decoder.Column, len(indexInfo.Columns))
for _, v := range indexInfo.Columns {
col := cols[v.Offset]
tpExpr := decoder.Column{
Col: col,
}
indexedCols := make([]*table.Column, len(indexInfo.Columns))
for i, v := range indexInfo.Columns {
indexedCols[i] = cols[v.Offset]
}

indexedColsCpy := make([]*table.Column, len(indexedCols))
copy(indexedColsCpy, indexedCols)
tangenta marked this conversation as resolved.
Show resolved Hide resolved
decodeColMap, err := buildFullDecodeColMap(indexedColsCpy, sessCtx, t)
if err != nil {
return nil, err
}

decoder.SubstituteGenColsInDecodeColMap(decodeColMap)
decoder.RemoveUnusedVirtualCols(decodeColMap, indexedCols)

return decodeColMap, nil
}

func buildFullDecodeColMap(pendingCols []*table.Column, sessCtx sessionctx.Context, t table.Table) (map[int64]decoder.Column, error) {
decodeColMap := make(map[int64]decoder.Column, len(pendingCols))
for i := 0; i < len(pendingCols); i++ {
col := pendingCols[i]
if col.IsGenerated() && !col.GeneratedStored {
for _, c := range cols {
// Find depended columns and put them into pendingCols.
for _, c := range t.Cols() {
tangenta marked this conversation as resolved.
Show resolved Hide resolved
if _, ok := col.Dependences[c.Name.L]; ok {
decodeColMap[c.ID] = decoder.Column{
Col: c,
}
pendingCols = append(pendingCols, c)
}
}
e, err := expression.ParseSimpleExprCastWithTableInfo(sessCtx, col.GeneratedExprString, t.Meta(), &col.FieldType)
if err != nil {
return nil, errors.Trace(err)
}
tpExpr.GenExpr = e
decodeColMap[col.ID] = decoder.Column{
Col: col,
GenExpr: e,
}
} else {
decodeColMap[col.ID] = decoder.Column{
Col: col,
}
}
decodeColMap[col.ID] = tpExpr
}
return decodeColMap, nil
}
Expand Down
40 changes: 25 additions & 15 deletions util/admin/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -589,27 +589,37 @@ func CompareTableRecord(sessCtx sessionctx.Context, txn kv.Transaction, t table.
}

func makeRowDecoder(t table.Table, decodeCol []*table.Column, genExpr map[model.TableColumnID]expression.Expression) *decoder.RowDecoder {
cols := t.Cols()
tblInfo := t.Meta()
decodeColsMap := make(map[int64]decoder.Column, len(decodeCol))
for _, v := range decodeCol {
col := cols[v.Offset]
tpExpr := decoder.Column{
Col: col,
}
decodeColCpy := make([]*table.Column, len(decodeCol))
copy(decodeColCpy, decodeCol)
decodeColsMap := buildFullDecodeColMap(decodeColCpy, t, genExpr)

decoder.SubstituteGenColsInDecodeColMap(decodeColsMap)
decoder.RemoveUnusedVirtualCols(decodeColsMap, decodeCol)
return decoder.NewRowDecoder(t, decodeColsMap)
}

func buildFullDecodeColMap(pendingCols []*table.Column, t table.Table, genExpr map[model.TableColumnID]expression.Expression) map[int64]decoder.Column {
decodeColMap := make(map[int64]decoder.Column, len(pendingCols))
for i := 0; i < len(pendingCols); i++ {
col := pendingCols[i]
if col.IsGenerated() && !col.GeneratedStored {
for _, c := range cols {
// Find depended columns and put them into pendingCols.
for _, c := range t.Cols() {
if _, ok := col.Dependences[c.Name.L]; ok {
decodeColsMap[c.ID] = decoder.Column{
Col: c,
}
pendingCols = append(pendingCols, c)
}
}
tpExpr.GenExpr = genExpr[model.TableColumnID{TableID: tblInfo.ID, ColumnID: col.ID}]
decodeColMap[col.ID] = decoder.Column{
Col: col,
GenExpr: genExpr[model.TableColumnID{TableID: t.Meta().ID, ColumnID: col.ID}],
}
} else {
decodeColMap[col.ID] = decoder.Column{
Col: col,
}
}
decodeColsMap[col.ID] = tpExpr
}
return decoder.NewRowDecoder(t, decodeColsMap)
return decodeColMap
}

// genExprs use to calculate generated column value.
Expand Down
71 changes: 71 additions & 0 deletions util/rowDecoder/decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
package decoder

import (
"github.com/pingcap/parser/ast"
tangenta marked this conversation as resolved.
Show resolved Hide resolved
"sort"
"time"

"github.com/pingcap/parser/mysql"
Expand Down Expand Up @@ -134,3 +136,72 @@ func (rd *RowDecoder) DecodeAndEvalRowWithMap(ctx sessionctx.Context, handle int
}
return row, nil
}

// SubstituteGenColsInDecodeColMap substitutes generated columns in every expression in decodeColMap
tangenta marked this conversation as resolved.
Show resolved Hide resolved
// with non-generated one by looking up decodeColMap.
func SubstituteGenColsInDecodeColMap(decodeColMap map[int64]Column) {
// Sort columns by id in ascending order.
var orderedCols []int64
for colID := range decodeColMap {
orderedCols = append(orderedCols, colID)
}
sort.Slice(orderedCols, func(i, j int) bool { return orderedCols[i] < orderedCols[j] })

for _, colID := range orderedCols {
decCol := decodeColMap[colID]
if decCol.GenExpr != nil {
decodeColMap[colID] = Column{
Col: decCol.Col,
GenExpr: substituteGeneratedColumn(decCol.GenExpr, decodeColMap),
}
} else {
decodeColMap[colID] = Column{
Col: decCol.Col,
}
}
}
}

// substituteGeneratedColumn substitutes generated columns in an expression with non-generated one by looking up decodeColMap.
func substituteGeneratedColumn(expr expression.Expression, decodeColMap map[int64]Column) expression.Expression {
switch v := expr.(type) {
case *expression.Column:
if c, ok := decodeColMap[v.ID]; ok && c.GenExpr != nil {
return c.GenExpr
}
return v
case *expression.ScalarFunction:
if v.FuncName.L == ast.Cast {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we handle CAST in this special way? I don't get the point from expression.ColumnSubstitute, neither from here.

Copy link
Contributor Author

@tangenta tangenta Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I am not sure. The removal of them does NOT fail the integration test.

newFunc := v.Clone().(*expression.ScalarFunction)
newFunc.GetArgs()[0] = substituteGeneratedColumn(newFunc.GetArgs()[0], decodeColMap)
return newFunc
}
newArgs := make([]expression.Expression, 0, len(v.GetArgs()))
for _, arg := range v.GetArgs() {
newArgs = append(newArgs, substituteGeneratedColumn(arg, decodeColMap))
}
return expression.NewFunctionInternal(v.GetCtx(), v.FuncName.L, v.RetType, newArgs...)
}
return expr
}

// RemoveUnusedVirtualCols removes all virtual columns in decodeColMap that cannot found in indexedCols.
func RemoveUnusedVirtualCols(decodeColMap map[int64]Column, indexedCols []*table.Column) {
for colID, decCol := range decodeColMap {
col := decCol.Col
if !col.IsGenerated() || col.GeneratedStored {
continue
}

found := false
for _, v := range indexedCols {
if v.Offset == col.Offset {
found = true
tangenta marked this conversation as resolved.
Show resolved Hide resolved
}
}

if !found {
delete(decodeColMap, colID)
}
}
}