Skip to content

Commit

Permalink
*: fix a bug causes indexed virtual generated column return wrong val…
Browse files Browse the repository at this point in the history
…ue and refine admin check table (#18408)
  • Loading branch information
wjhuang2016 authored Jul 24, 2020
1 parent a57e95b commit baf6c99
Show file tree
Hide file tree
Showing 23 changed files with 217 additions and 275 deletions.
2 changes: 1 addition & 1 deletion ddl/db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3830,7 +3830,7 @@ func testAddIndexForGeneratedColumn(tk *testkit.TestKit, s *testSerialDBSuite, c
tk.MustExec("insert into t values()")
tk.MustExec("ALTER TABLE t ADD COLUMN y1 year as (y + 2)")
_, err := tk.Exec("ALTER TABLE t ADD INDEX idx_y(y1)")
c.Assert(err.Error(), Equals, "[ddl:8202]Cannot decode index value, because cannot convert datum from unsigned bigint to type year.")
c.Assert(err, IsNil)

t := s.testGetTable(c, "t")
for _, idx := range t.Indices() {
Expand Down
14 changes: 5 additions & 9 deletions ddl/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -1209,19 +1209,15 @@ func makeupDecodeColMap(sessCtx sessionctx.Context, t table.Table, indexInfo *mo
indexedCols[i] = cols[v.Offset]
}

var containsVirtualCol bool
decodeColMap, err := decoder.BuildFullDecodeColMap(indexedCols, t, func(genCol *table.Column) (expression.Expression, error) {
containsVirtualCol = true
return expression.ParseSimpleExprCastWithTableInfo(sessCtx, genCol.GeneratedExprString, t.Meta(), &genCol.FieldType)
})
dbName := model.NewCIStr(sessCtx.GetSessionVars().CurrentDB)
exprCols, _, err := expression.ColumnInfos2ColumnsAndNames(sessCtx, dbName, t.Meta().Name, t.Meta().Columns, t.Meta())
if err != nil {
return nil, err
}
mockSchema := expression.NewSchema(exprCols...)

decodeColMap := decoder.BuildFullDecodeColMap(t, mockSchema)

if containsVirtualCol {
decoder.SubstituteGenColsInDecodeColMap(decodeColMap)
decoder.RemoveUnusedVirtualCols(decodeColMap, indexedCols)
}
return decodeColMap, nil
}

Expand Down
3 changes: 2 additions & 1 deletion executor/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,11 +357,12 @@ func buildIndexLookUpChecker(b *executorBuilder, p *plannercore.PhysicalIndexLoo
for _, col := range is.Columns {
tps = append(tps, &col.FieldType)
}

if !e.isCommonHandle() {
tps = append(tps, types.NewFieldType(mysql.TypeLonglong))
}

e.checkIndexValue = &checkIndexValue{genExprs: is.GenExprs, idxColTps: tps}
e.checkIndexValue = &checkIndexValue{idxColTps: tps}

colNames := make([]string, 0, len(is.IdxCols))
for i := range is.IdxCols {
Expand Down
18 changes: 1 addition & 17 deletions executor/distsql.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,6 @@ type IndexLookUpExecutor struct {
type checkIndexValue struct {
idxColTps []*types.FieldType
idxTblCols []*table.Column
genExprs map[model.TableColumnID]expression.Expression
}

// Open implements the Executor Open interface.
Expand Down Expand Up @@ -883,7 +882,6 @@ func (w *tableWorker) compareData(ctx context.Context, task *lookupTableTask, ta
break
}

tblReaderExec := tableReader.(*TableReaderExecutor)
iter := chunk.NewIterator4Chunk(chk)
for row := iter.Begin(); row != iter.End(); row = iter.Next() {
handle, err := w.idxLookup.getHandle(row, w.handleIdx, w.idxLookup.isCommonHandle())
Expand All @@ -899,21 +897,7 @@ func (w *tableWorker) compareData(ctx context.Context, task *lookupTableTask, ta
idxRow := task.idxRows.GetRow(offset)
vals = vals[:0]
for i, col := range w.idxTblCols {
if col.IsGenerated() && !col.GeneratedStored {
expr := w.genExprs[model.TableColumnID{TableID: tblInfo.ID, ColumnID: col.ID}]
// Eval the column value
val, err := expr.Eval(row)
if err != nil {
return errors.Trace(err)
}
val, err = table.CastValue(tblReaderExec.ctx, val, col.ColumnInfo, false, false)
if err != nil {
return errors.Trace(err)
}
vals = append(vals, val)
} else {
vals = append(vals, row.GetDatum(i, &col.FieldType))
}
vals = append(vals, row.GetDatum(i, &col.FieldType))
}
vals = tablecodec.TruncateIndexValuesIfNeeded(tblInfo, w.idxLookup.index, vals)
for i, val := range vals {
Expand Down
6 changes: 2 additions & 4 deletions executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -782,23 +782,21 @@ func (e *CheckTableExec) Next(ctx context.Context, req *chunk.Chunk) error {

func (e *CheckTableExec) checkTableRecord(idxOffset int) error {
idxInfo := e.indexInfos[idxOffset]
// TODO: Fix me later, can not use genExprs in indexLookUpReader, because the schema of expression is different.
genExprs := e.srcs[idxOffset].genExprs
txn, err := e.ctx.Txn(true)
if err != nil {
return err
}
if e.table.Meta().GetPartitionInfo() == nil {
idx := tables.NewIndex(e.table.Meta().ID, e.table.Meta(), idxInfo)
return admin.CheckRecordAndIndex(e.ctx, txn, e.table, idx, genExprs)
return admin.CheckRecordAndIndex(e.ctx, txn, e.table, idx)
}

info := e.table.Meta().GetPartitionInfo()
for _, def := range info.Definitions {
pid := def.ID
partition := e.table.(table.PartitionedTable).GetPartition(pid)
idx := tables.NewIndex(def.ID, e.table.Meta(), idxInfo)
if err := admin.CheckRecordAndIndex(e.ctx, txn, partition, idx, genExprs); err != nil {
if err := admin.CheckRecordAndIndex(e.ctx, txn, partition, idx); err != nil {
return errors.Trace(err)
}
}
Expand Down
3 changes: 2 additions & 1 deletion expression/builtin_compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ func (s *testEvaluatorSuite) TestCompareFunctionWithRefine(c *C) {
{"-123456789123456789123456789.12345 < a", "1"},
{"'aaaa'=a", "eq(0, a)"},
}
cols, names := ColumnInfos2ColumnsAndNames(s.ctx, model.NewCIStr(""), tblInfo.Name, tblInfo.Columns)
cols, names, err := ColumnInfos2ColumnsAndNames(s.ctx, model.NewCIStr(""), tblInfo.Name, tblInfo.Columns, tblInfo)
c.Assert(err, IsNil)
schema := NewSchema(cols...)
for _, t := range tests {
f, err := ParseSimpleExprsWithNames(s.ctx, t.exprStr, schema, names)
Expand Down
2 changes: 2 additions & 0 deletions expression/builtin_time_vec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,7 @@ func BenchmarkVectorizedBuiltinTimeFunc(b *testing.B) {
func (s *testEvaluatorSuite) TestVecMonth(c *C) {
ctx := mock.NewContext()
ctx.GetSessionVars().SQLMode |= mysql.ModeNoZeroDate
ctx.GetSessionVars().StmtCtx.TruncateAsWarning = true
input := chunk.New([]*types.FieldType{types.NewFieldType(mysql.TypeDatetime)}, 3, 3)
input.Reset()
input.AppendTime(0, types.ZeroDate)
Expand All @@ -476,5 +477,6 @@ func (s *testEvaluatorSuite) TestVecMonth(c *C) {
c.Assert(len(ctx.GetSessionVars().StmtCtx.GetWarnings()), Equals, 2)

ctx.GetSessionVars().StmtCtx.InInsertStmt = true
ctx.GetSessionVars().StmtCtx.TruncateAsWarning = false
c.Assert(f.vecEvalInt(input, result), NotNil)
}
2 changes: 1 addition & 1 deletion expression/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ func handleInvalidTimeError(ctx sessionctx.Context, err error) error {
return err
}
sc := ctx.GetSessionVars().StmtCtx
err = sc.HandleTruncate(err)
if ctx.GetSessionVars().StrictSQLMode && (sc.InInsertStmt || sc.InUpdateStmt || sc.InDeleteStmt) {
return err
}
sc.AppendWarning(err)
return nil
}

Expand Down
51 changes: 46 additions & 5 deletions expression/expression.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/pingcap/tidb/types"
"github.com/pingcap/tidb/types/json"
"github.com/pingcap/tidb/util/chunk"
"github.com/pingcap/tidb/util/generatedexpr"
"github.com/pingcap/tidb/util/logutil"
"github.com/pingcap/tipb/go-tipb"
"go.uber.org/zap"
Expand All @@ -49,6 +50,9 @@ const (
// EvalAstExpr evaluates ast expression directly.
var EvalAstExpr func(sctx sessionctx.Context, expr ast.ExprNode) (types.Datum, error)

// RewriteAstExpr rewrites ast expression directly.
var RewriteAstExpr func(sctx sessionctx.Context, expr ast.ExprNode, schema *Schema, names types.NameSlice) (Expression, error)

// VecExpr contains all vectorized evaluation methods.
type VecExpr interface {
// Vectorized returns if this expression supports vectorized evaluation.
Expand Down Expand Up @@ -712,8 +716,11 @@ func EvaluateExprWithNull(ctx sessionctx.Context, schema *Schema, expr Expressio
}

// TableInfo2SchemaAndNames converts the TableInfo to the schema and name slice.
func TableInfo2SchemaAndNames(ctx sessionctx.Context, dbName model.CIStr, tbl *model.TableInfo) (*Schema, []*types.FieldName) {
cols, names := ColumnInfos2ColumnsAndNames(ctx, dbName, tbl.Name, tbl.Columns)
func TableInfo2SchemaAndNames(ctx sessionctx.Context, dbName model.CIStr, tbl *model.TableInfo) (*Schema, []*types.FieldName, error) {
cols, names, err := ColumnInfos2ColumnsAndNames(ctx, dbName, tbl.Name, tbl.Columns, tbl)
if err != nil {
return nil, nil, err
}
keys := make([]KeyInfo, 0, len(tbl.Indices)+1)
for _, idx := range tbl.Indices {
if !idx.Unique || idx.State != model.StatePublic {
Expand Down Expand Up @@ -752,11 +759,11 @@ func TableInfo2SchemaAndNames(ctx sessionctx.Context, dbName model.CIStr, tbl *m
}
schema := NewSchema(cols...)
schema.SetUniqueKeys(keys)
return schema, names
return schema, names, nil
}

// ColumnInfos2ColumnsAndNames converts the ColumnInfo to the *Column and NameSlice.
func ColumnInfos2ColumnsAndNames(ctx sessionctx.Context, dbName, tblName model.CIStr, colInfos []*model.ColumnInfo) ([]*Column, types.NameSlice) {
func ColumnInfos2ColumnsAndNames(ctx sessionctx.Context, dbName, tblName model.CIStr, colInfos []*model.ColumnInfo, tblInfo *model.TableInfo) ([]*Column, types.NameSlice, error) {
columns := make([]*Column, 0, len(colInfos))
names := make([]*types.FieldName, 0, len(colInfos))
for i, col := range colInfos {
Expand All @@ -780,7 +787,41 @@ func ColumnInfos2ColumnsAndNames(ctx sessionctx.Context, dbName, tblName model.C
}
columns = append(columns, newCol)
}
return columns, names
// Resolve virtual generated column.
mockSchema := NewSchema(columns...)
// Ignore redundant warning here.
save := ctx.GetSessionVars().StmtCtx.IgnoreTruncate
defer func() {
ctx.GetSessionVars().StmtCtx.IgnoreTruncate = save
}()
ctx.GetSessionVars().StmtCtx.IgnoreTruncate = true
for i, col := range colInfos {
if col.State != model.StatePublic {
continue
}
if col.IsGenerated() && !col.GeneratedStored {
expr, err := generatedexpr.ParseExpression(col.GeneratedExprString)
if err != nil {
return nil, nil, errors.Trace(err)
}
expr, err = generatedexpr.SimpleResolveName(expr, tblInfo)
if err != nil {
return nil, nil, errors.Trace(err)
}
e, err := RewriteAstExpr(ctx, expr, mockSchema, names)
if err != nil {
return nil, nil, errors.Trace(err)
}
if e != nil {
columns[i].VirtualExpr = e.Clone()
}
columns[i].VirtualExpr, err = columns[i].VirtualExpr.ResolveIndices(mockSchema)
if err != nil {
return nil, nil, errors.Trace(err)
}
}
}
return columns, names, nil
}

// NewValuesFunc creates a new values function.
Expand Down
12 changes: 12 additions & 0 deletions expression/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6763,6 +6763,18 @@ func (s *testIntegrationSerialSuite) TestIssue17233(c *C) {
tk.MustQuery("SELECT view_10.col_1 FROM view_4 JOIN view_10").Check(testkit.Rows("16", "16", "16", "16", "16", "18", "18", "18", "18", "18", "19", "19", "19", "19", "19"))
}

func (s *testIntegrationSerialSuite) TestIssue17989(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
tk.MustExec("drop table if exists t")
tk.MustExec("create table t(a int, b tinyint as(a+1), c int as(b+1));")
tk.MustExec("set sql_mode='';")
tk.MustExec("insert into t(a) values(2000);")
tk.MustExec("create index idx on t(c);")
tk.MustQuery("select c from t;").Check(testkit.Rows("128"))
tk.MustExec("admin check table t")
}

func (s *testIntegrationSerialSuite) TestIssue18638(c *C) {
collate.SetNewCollationEnabledForTest(true)
defer collate.SetNewCollationEnabledForTest(false)
Expand Down
5 changes: 4 additions & 1 deletion expression/simple_rewriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ func ParseSimpleExprCastWithTableInfo(ctx sessionctx.Context, exprStr string, ta
// RewriteSimpleExprWithTableInfo rewrites simple ast.ExprNode to expression.Expression.
func RewriteSimpleExprWithTableInfo(ctx sessionctx.Context, tbl *model.TableInfo, expr ast.ExprNode) (Expression, error) {
dbName := model.NewCIStr(ctx.GetSessionVars().CurrentDB)
columns, names := ColumnInfos2ColumnsAndNames(ctx, dbName, tbl.Name, tbl.Columns)
columns, names, err := ColumnInfos2ColumnsAndNames(ctx, dbName, tbl.Name, tbl.Columns, tbl)
if err != nil {
return nil, err
}
rewriter := &simpleRewriter{ctx: ctx, schema: NewSchema(columns...), names: names}
expr.Accept(rewriter)
if rewriter.err != nil {
Expand Down
17 changes: 15 additions & 2 deletions planner/core/expression_rewriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,30 @@ func evalAstExpr(sctx sessionctx.Context, expr ast.ExprNode) (types.Datum, error
if val, ok := expr.(*driver.ValueExpr); ok {
return val.Datum, nil
}
newExpr, err := rewriteAstExpr(sctx, expr, nil, nil)
if err != nil {
return types.Datum{}, err
}
return newExpr.Eval(chunk.Row{})
}

// rewriteAstExpr rewrites ast expression directly.
func rewriteAstExpr(sctx sessionctx.Context, expr ast.ExprNode, schema *expression.Schema, names types.NameSlice) (expression.Expression, error) {
var is infoschema.InfoSchema
if sctx.GetSessionVars().TxnCtx.InfoSchema != nil {
is = sctx.GetSessionVars().TxnCtx.InfoSchema.(infoschema.InfoSchema)
}
b := NewPlanBuilder(sctx, is, &hint.BlockHintProcessor{})
fakePlan := LogicalTableDual{}.Init(sctx, 0)
if schema != nil {
fakePlan.schema = schema
fakePlan.names = names
}
newExpr, _, err := b.rewrite(context.TODO(), expr, fakePlan, nil, true)
if err != nil {
return types.Datum{}, err
return nil, err
}
return newExpr.Eval(chunk.Row{})
return newExpr, nil
}

func (b *PlanBuilder) rewriteInsertOnDuplicateUpdate(ctx context.Context, exprNode ast.ExprNode, mockPlan LogicalPlan, insertPlan *Insert) (expression.Expression, error) {
Expand Down
1 change: 1 addition & 0 deletions planner/core/optimizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ var DefaultDisabledLogicalRulesList *atomic.Value

func init() {
expression.EvalAstExpr = evalAstExpr
expression.RewriteAstExpr = rewriteAstExpr
DefaultDisabledLogicalRulesList = new(atomic.Value)
DefaultDisabledLogicalRulesList.Store(set.NewStringSet())
}
3 changes: 2 additions & 1 deletion planner/core/partition_pruning_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ func prepareTestCtx(c *C, createTable string, partitionExpr string) *testCtx {
sctx := mock.NewContext()
tblInfo, err := ddl.BuildTableInfoFromAST(stmt.(*ast.CreateTableStmt))
c.Assert(err, IsNil)
columns, names := expression.ColumnInfos2ColumnsAndNames(sctx, model.NewCIStr("t"), tblInfo.Name, tblInfo.Columns)
columns, names, err := expression.ColumnInfos2ColumnsAndNames(sctx, model.NewCIStr("t"), tblInfo.Name, tblInfo.Columns, tblInfo)
c.Assert(err, IsNil)
schema := expression.NewSchema(columns...)

col, fn, err := makePartitionByFnCol(sctx, columns, names, partitionExpr)
Expand Down
4 changes: 0 additions & 4 deletions planner/core/physical_plans.go
Original file line number Diff line number Diff line change
Expand Up @@ -351,10 +351,6 @@ func (p *PhysicalIndexScan) Clone() (PhysicalPlan, error) {
if p.Hist != nil {
cloned.Hist = p.Hist.Copy()
}
cloned.GenExprs = make(map[model.TableColumnID]expression.Expression)
for id, expr := range p.GenExprs {
cloned.GenExprs[id] = expr.Clone()
}
return cloned, nil
}

Expand Down
Loading

0 comments on commit baf6c99

Please sign in to comment.