fix alter add index on virtual column bug #7575

crazycs520 · 2018-08-31T17:36:45Z

What problem does this PR solve?

create table test ( b json , c int as (JSON_EXTRACT(b,'$.d')));
insert into test set b='{"d": 100}';
alter table test add index idxd(c);
admin check table test;

When alter add index on a virtual generated column, we shoud work out the val by colGenExpr.
The old TiDB will use test virtual generated column default value to fill back to index record when alter add index.

What is changed and how it works?

If index.column is a virtual generated column, get the generatedExpr, and also, we need to decode the column of virtual column dependent on.
after decode from raw data, we have to use generatedExpr to eval the virtual column data.

Check List

Tests

Unit test
Increased code complexity

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to update the tidb-ansible repository
Need to be included in the release note

crazycs520 · 2018-09-03T03:29:57Z

@winkyao @zimulala @ciscoxll PTAL

winoros · 2018-09-03T09:05:36Z

ddl/index.go

 	closed      bool
 	priority    int

 	// The following attributes are used to reduce memory allocation.
 	defaultVals        []types.Datum
 	idxRecords         []*indexRecord
 	rowMap             map[int64]types.Datum
+	rowSlice           []types.Datum


colSlice instead of row?

colSlice is just a buffer, main purpose is getting row for expression eval.

I think rowBuffer is a more clear name.

winoros · 2018-09-03T09:07:08Z

ddl/index.go

+				w.rowSlice[col.Offset] = ri
+			}
+		}
+		w.row = chunk.MutRowFromDatums(w.rowSlice).ToRow()


Since rowSlice is only used here, and the check are both if len(colExprMap) != 0. So let it be a local variable instead of one in struct.

put it in the struct is used to reduce memory allocations. @winoros

winkyao · 2018-09-04T02:14:56Z

@crazycs520 Please pick the proper items in Related changes, in the pr description.

winkyao · 2018-09-04T03:21:59Z

ddl/index.go

@@ -843,14 +890,28 @@ func (w *addIndexWorker) run(d *ddlCtx) {
 	log.Infof("[ddl-reorg] worker[%v] exit", w.id)
 }

-func makeupIndexColFieldMap(t table.Table, indexInfo *model.IndexInfo) map[int64]*types.FieldType {
+func makeupIndexColFieldMapAndColExpr(sessCtx sessionctx.Context, t table.Table, indexInfo *model.IndexInfo) (map[int64]*types.FieldType, map[int64]expression.Expression, error) {


You can add a struct like

type SomeStruct struct { tp *types.FieldType genExpr expression.Expression }

then return map like map[int64]*SomeStruct to avoid using two map.

But I respectively need 2 map, colFieldMap use to decode row, and colExprMap use to expression to eval.

You can achieve decoding row and eval an expression by using just one map.

…ex-on-vir-col-bug

crazycs520 · 2018-09-11T05:12:11Z

@winkyao PTAL

crazycs520 · 2018-09-11T11:09:32Z

/run-all-tests

crazycs520 · 2018-09-11T11:35:03Z

/run-common-test

crazycs520 · 2018-09-11T12:03:03Z

/run-all-tests

…ex-on-vir-col-bug

crazycs520 · 2018-09-12T05:32:09Z

/run-all-tests

ciscoxll · 2018-09-12T08:38:47Z

@crazycs520 Please fix conflicts.

ciscoxll

reset LGTM

…ex-on-vir-col-bug

winkyao · 2018-09-28T07:49:34Z

executor/executor_test.go

@@ -273,6 +273,12 @@ func (s *testSuite) TestAdmin(c *C) {
 	tk.MustExec("ALTER TABLE t1 ADD INDEX idx3 (c4);")
 	tk.MustExec("admin check table t1;")

+	// For add index on virtual column
+	tk.MustExec("drop table if exists t1;")


Please add more test cases.

@winkyao done.

…ex-on-vir-col-bug

crazycs520 · 2018-09-28T11:07:30Z

/run-all-tests

winkyao

LGTM

zhexuany

LGTM

only 2 LGTM

zimulala · 2018-09-30T02:43:01Z

util/rowDecoder/decoder.go

+package decoder
+
+import (
+	"github.com/pingcap/tidb/mysql"


Put it to line 20.

done. @zimulala PTAL

…ex-on-vir-col-bug

zimulala · 2018-09-30T08:07:53Z

executor/executor_test.go

+	tk.MustExec("alter table t1 add index idx_g(g);")
+	tk.MustExec("alter table t1 add index idx_h(h);")
+	tk.MustExec("alter table t1 add index idx_j(j);")
+	tk.MustExec("alter table t1 add index idx_i(i);")


Are we going to add some indexes for multiple columns?

done. @zimulala PTAL

crazycs520 · 2018-09-30T15:37:17Z

/run-all-tests

…ex-on-vir-col-bug

zimulala

LGTM

zimulala · 2018-10-10T04:15:48Z

/run-all-tests

crazycs520 · 2018-10-10T04:22:21Z

/run-all-tests

fix alter add index on virtual column bug

0112b14

crazycs520 added component/DDL-need-LGT3 type/bugfix This PR fixes a bug. labels Sep 3, 2018

winoros reviewed Sep 3, 2018

View reviewed changes

winkyao reviewed Sep 4, 2018

View reviewed changes

address comment

e19e34c

crazycs520 added the status/WIP label Sep 7, 2018

crazycs520 added 6 commits September 7, 2018 10:54

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

177a35d

…ex-on-vir-col-bug

fix column generate expr no cast to target field type bug

0e1cd5a

refactor code

7ef6521

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

880bd06

…ex-on-vir-col-bug

add row decoder

2c3be20

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

7f5b7cc

…ex-on-vir-col-bug

crazycs520 force-pushed the add-index-on-vir-col-bug branch from 50d4890 to 121a7f0 Compare September 11, 2018 04:41

refine code

bc00f25

crazycs520 force-pushed the add-index-on-vir-col-bug branch from 121a7f0 to bc00f25 Compare September 11, 2018 04:46

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

9645b21

…ex-on-vir-col-bug

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

9c624ff

…ex-on-vir-col-bug

ciscoxll reviewed Sep 12, 2018

View reviewed changes

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

3ac1fb2

…ex-on-vir-col-bug

crazycs520 force-pushed the add-index-on-vir-col-bug branch from 4fa73c1 to 3ac1fb2 Compare September 13, 2018 08:57

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

4e44d06

…ex-on-vir-col-bug

winkyao reviewed Sep 28, 2018

View reviewed changes

crazycs520 added 2 commits September 28, 2018 17:50

add more test and fix timestamp bugs

c2a3ecf

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

a9d653b

…ex-on-vir-col-bug

winkyao reviewed Sep 29, 2018

View reviewed changes

zhexuany previously approved these changes Sep 29, 2018

View reviewed changes

zimulala reviewed Sep 30, 2018

View reviewed changes

crazycs520 added 2 commits September 30, 2018 11:59

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

9c36df4

…ex-on-vir-col-bug

address comment

2818aab

crazycs520 force-pushed the add-index-on-vir-col-bug branch from 10f2197 to 2818aab Compare September 30, 2018 04:05

zimulala reviewed Sep 30, 2018

View reviewed changes

fix add index on multi generated column bugs and add test

012ea29

crazycs520 force-pushed the add-index-on-vir-col-bug branch from 3a80405 to 012ea29 Compare September 30, 2018 11:33

crazycs520 added the status/LGT2 Indicates that a PR has LGTM 2. label Oct 10, 2018

Merge branch 'master' of https://github.com/pingcap/tidb into add-ind…

2916122

…ex-on-vir-col-bug

zimulala approved these changes Oct 10, 2018

View reviewed changes

zimulala added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Oct 10, 2018

Merge branch 'master' into add-index-on-vir-col-bug

1da045a

crazycs520 merged commit fd71236 into pingcap:master Oct 10, 2018

crazycs520 added a commit to crazycs520/tidb that referenced this pull request Dec 11, 2018

ddl: fix alter add index on virtual column bug (pingcap#7575)

9bec31f

crazycs520 mentioned this pull request Dec 11, 2018

ddl: fix alter add index on virtual column bug (#7575) #8655

Merged

zz-jason pushed a commit that referenced this pull request Dec 12, 2018

ddl: fix alter add index on virtual column bug (#7575) (#8655)

86b5ce5

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix alter add index on virtual column bug #7575

fix alter add index on virtual column bug #7575

crazycs520 commented Aug 31, 2018 •

edited

Loading

crazycs520 commented Sep 3, 2018

winoros Sep 3, 2018

crazycs520 Sep 4, 2018

winoros Sep 4, 2018

winoros Sep 3, 2018

winkyao Sep 4, 2018

winkyao commented Sep 4, 2018

winkyao Sep 4, 2018 •

edited

Loading

crazycs520 Sep 4, 2018

winkyao Sep 4, 2018 •

edited

Loading

crazycs520 commented Sep 11, 2018

crazycs520 commented Sep 11, 2018

crazycs520 commented Sep 11, 2018

crazycs520 commented Sep 11, 2018

crazycs520 commented Sep 12, 2018

ciscoxll commented Sep 12, 2018 •

edited

Loading

ciscoxll left a comment

winkyao Sep 28, 2018

crazycs520 Sep 28, 2018

crazycs520 commented Sep 28, 2018

winkyao left a comment

zhexuany left a comment

zimulala Sep 30, 2018

crazycs520 Sep 30, 2018

zimulala Sep 30, 2018

crazycs520 Sep 30, 2018

crazycs520 commented Sep 30, 2018

zimulala left a comment

zimulala commented Oct 10, 2018

crazycs520 commented Oct 10, 2018

fix alter add index on virtual column bug #7575

fix alter add index on virtual column bug #7575

Conversation

crazycs520 commented Aug 31, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

crazycs520 commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao commented Sep 4, 2018

winkyao Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

crazycs520 commented Sep 11, 2018

crazycs520 commented Sep 11, 2018

crazycs520 commented Sep 11, 2018

crazycs520 commented Sep 11, 2018

crazycs520 commented Sep 12, 2018

ciscoxll commented Sep 12, 2018 • edited Loading

ciscoxll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 commented Sep 28, 2018

winkyao left a comment

Choose a reason for hiding this comment

zhexuany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 commented Sep 30, 2018

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Oct 10, 2018

crazycs520 commented Oct 10, 2018

crazycs520 commented Aug 31, 2018 •

edited

Loading

winkyao Sep 4, 2018 •

edited

Loading

winkyao Sep 4, 2018 •

edited

Loading

ciscoxll commented Sep 12, 2018 •

edited

Loading