-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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, parser: support 'ALTER TABLE RENAME KEY TO' syntax #6475
Changes from 1 commit
8ffbb10
cc4f73d
de15d28
f1e3621
fe3423e
259f141
61bc9b6
6249e70
5ff091e
647f31f
492ac8d
d7b784d
f5b2da6
b5e794c
696798f
bb28681
ccffedf
9b4c344
70d8304
afa6bbf
6237a56
1ba167b
d5a175f
319ed7a
f43be2c
5843e9e
e7e576d
c30b65d
ceca5ac
22e0ee0
fedcf82
582a02f
27b1ca5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -991,6 +991,8 @@ func (d *ddl) AlterTable(ctx sessionctx.Context, ident ast.Ident, specs []*ast.A | |
err = d.RenameTable(ctx, ident, newIdent) | ||
case ast.AlterTableDropPrimaryKey: | ||
err = ErrUnsupportedModifyPrimaryKey.GenByArgs("drop") | ||
case ast.AlterTableRenameIndex: | ||
err = d.RenameIndex(ctx, ident, spec) | ||
case ast.AlterTableOption: | ||
for _, opt := range spec.Options { | ||
switch opt.Tp { | ||
|
@@ -1559,6 +1561,42 @@ func (d *ddl) AlterTableComment(ctx sessionctx.Context, ident ast.Ident, spec *a | |
return errors.Trace(err) | ||
} | ||
|
||
// RenameIndex updates the table comment information. | ||
func (d *ddl) RenameIndex(ctx sessionctx.Context, ident ast.Ident, spec *ast.AlterTableSpec) error { | ||
is := d.infoHandle.Get() | ||
schema, ok := is.SchemaByName(ident.Schema) | ||
if !ok { | ||
return infoschema.ErrDatabaseNotExists.GenByArgs(ident.Schema) | ||
} | ||
|
||
tb, err := is.TableByName(ident.Schema, ident.Name) | ||
if err != nil { | ||
return errors.Trace(infoschema.ErrTableNotExists.GenByArgs(ident.Schema, ident.Name)) | ||
} | ||
|
||
if fromIdx := findIndexByName(spec.FromKey.L, tb.Meta().Indices); fromIdx == nil { | ||
return errors.Trace(infoschema.ErrKeyNotExists.GenByArgs(spec.FromKey.O, tb.Meta().Name)) | ||
} | ||
if spec.FromKey.O == spec.ToKey.O { | ||
return nil | ||
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. What's MySQL's behavior ? 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. It's the MySQL's behavior. You can see the test case in line 2660. |
||
} | ||
if toIdx := findIndexByName(spec.ToKey.L, tb.Meta().Indices); toIdx != nil && spec.FromKey.L != spec.ToKey.L { | ||
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.
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. TiDB is case-insensitive. 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. @tiancaiamao @zimulala although index check is case-insensitive (index
Take an example in MySQL, pay attention to KEY a: mysql> create table t1 (pk int primary key, i int, j int, key a(i));
Query OK, 0 rows affected (0.03 sec)
mysql> show create table t1;
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
| Table | Create Table
|
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
| t1 | CREATE TABLE `t1` (
`pk` int(11) NOT NULL,
`i` int(11) DEFAULT NULL,
`j` int(11) DEFAULT NULL,
PRIMARY KEY (`pk`),
KEY `a` (`i`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
1 row in set (0.00 sec)
mysql> alter table t1 rename key a to A;
Query OK, 0 rows affected (0.02 sec)
Records: 0 Duplicates: 0 Warnings: 0
mysql> show create table t1;
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
| Table | Create Table
|
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
| t1 | CREATE TABLE `t1` (
`pk` int(11) NOT NULL,
`i` int(11) DEFAULT NULL,
`j` int(11) DEFAULT NULL,
PRIMARY KEY (`pk`),
KEY `A` (`i`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+-------+------------------------------------------------------------------------------------------------------------------------------------
-----------------------------------------------+
1 row in set (0.00 sec)
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. So, add comment to the code. 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. ok |
||
return errors.Trace(infoschema.ErrKeyNameDuplicate.GenByArgs(toIdx.Name.O)) | ||
} | ||
|
||
job := &model.Job{ | ||
SchemaID: schema.ID, | ||
TableID: tb.Meta().ID, | ||
Type: model.ActionRenameIndex, | ||
BinlogInfo: &model.HistoryInfo{}, | ||
Args: []interface{}{spec.FromKey, spec.ToKey}, | ||
} | ||
|
||
err = d.doDDLJob(ctx, job) | ||
err = d.callHookOnChanged(err) | ||
return errors.Trace(err) | ||
} | ||
|
||
// DropTable will proceed even if some table in the list does not exists. | ||
func (d *ddl) DropTable(ctx sessionctx.Context, ti ast.Ident) (err error) { | ||
is := d.getInformationSchema() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -356,6 +356,31 @@ func (d *ddl) onModifyTableComment(t *meta.Meta, job *model.Job) (ver int64, _ e | |
return ver, nil | ||
} | ||
|
||
func (d *ddl) onRenameIndex(t *meta.Meta, job *model.Job) (ver int64, _ error) { | ||
var from, to model.CIStr | ||
if err := job.DecodeArgs(&from, &to); err != nil { | ||
job.State = model.JobStateCancelled | ||
return ver, errors.Trace(err) | ||
} | ||
|
||
tblInfo, err := getTableInfo(t, job, job.SchemaID) | ||
if err != nil { | ||
return ver, errors.Trace(err) | ||
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. Can you add 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. OK |
||
} | ||
|
||
idx := findIndexByName(from.L, tblInfo.Indices) | ||
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. I'm afraid we have to double check here if we don't take @coocood 's suggestion. When the code runs to here, those two request would both success! 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. yes, double check is needed here. |
||
if idx == nil { | ||
return ver, errors.Trace(infoschema.ErrKeyNotExists.GenByArgs(from, tblInfo.Name)) | ||
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. Ditto. 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. OK |
||
} | ||
idx.Name = to | ||
ver, err = updateVersionAndTableInfo(t, job, tblInfo, true) | ||
if err != nil { | ||
return ver, errors.Trace(err) | ||
} | ||
job.FinishTableJob(model.JobStateDone, model.StatePublic, ver, tblInfo) | ||
return ver, nil | ||
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. s/nil/ errors.Trace(err) 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. I think return nil is ok. |
||
} | ||
|
||
func checkTableNotExists(t *meta.Meta, job *model.Job, schemaID int64, tableName string) error { | ||
// Check this table's database. | ||
tables, err := t.ListTables(schemaID) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2635,6 +2635,51 @@ func (s *testSuite) TestCheckIndex(c *C) { | |
// table data (handle, data): (1, 10), (2, 20), (4, 40) | ||
} | ||
|
||
func (s *testSuite) TestRenameIndex(c *C) { | ||
s.ctx = mock.NewContext() | ||
s.ctx.Store = s.store | ||
_, err := session.BootstrapSession(s.store) | ||
c.Assert(err, IsNil) | ||
se, err := session.CreateSession4Test(s.store) | ||
c.Assert(err, IsNil) | ||
defer se.Close() | ||
|
||
_, err = se.Execute(context.Background(), "create database test_rename_index") | ||
c.Assert(err, IsNil) | ||
_, err = se.Execute(context.Background(), "use test_rename_index") | ||
c.Assert(err, IsNil) | ||
_, err = se.Execute(context.Background(), "create table t (pk int primary key, c int default 1, c1 int default 1, unique key k1(c), key k2(c1))") | ||
c.Assert(err, IsNil) | ||
|
||
// Test rename success | ||
_, err = se.Execute(context.Background(), "alter table t rename index k1 to k3") | ||
c.Assert(err, IsNil) | ||
_, err = se.Execute(context.Background(), "admin check index t k3") | ||
c.Assert(err, IsNil) | ||
|
||
// Test rename to the same name | ||
_, err = se.Execute(context.Background(), "alter table t rename index k3 to k3") | ||
c.Assert(err, IsNil) | ||
_, err = se.Execute(context.Background(), "admin check index t k3") | ||
c.Assert(err, IsNil) | ||
|
||
// Test rename on non-exists keys | ||
_, err = se.Execute(context.Background(), "alter table t rename index x to x") | ||
c.Assert(err, NotNil) | ||
c.Assert(strings.Contains(err.Error(), "Key 'x' doesn't exist in table 't'"), IsTrue) | ||
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. You can use 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. I'll move these tests to |
||
|
||
// Test rename on already-exists keys | ||
_, err = se.Execute(context.Background(), "alter table t rename index k3 to k2") | ||
c.Assert(err, NotNil) | ||
c.Assert(strings.Contains(err.Error(), "Duplicate key name 'k2'"), IsTrue) | ||
|
||
_, err = se.Execute(context.Background(), "alter table t rename index k2 to K2") | ||
c.Assert(err, IsNil) | ||
_, err = se.Execute(context.Background(), "alter table t rename key k3 to K2") | ||
c.Assert(err, NotNil) | ||
c.Assert(strings.Contains(err.Error(), "Duplicate key name 'K2'"), IsTrue) | ||
} | ||
|
||
func setColValue(c *C, txn kv.Transaction, key kv.Key, v types.Datum) { | ||
row := []types.Datum{v, {}} | ||
colIDs := []int64{2, 3} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,10 @@ var ( | |
ErrColumnExists = terror.ClassSchema.New(codeColumnExists, "Duplicate column name '%s'") | ||
// ErrIndexExists returns for index already exists. | ||
ErrIndexExists = terror.ClassSchema.New(codeIndexExists, "Duplicate Index") | ||
// ErrKeyNameDuplicate returns for index duplicate when rename index. | ||
ErrKeyNameDuplicate = terror.ClassSchema.New(codeKeyNameDuplicate, "Duplicate key name '%s'") | ||
// ErrKeyNotExists returns for index not exists. | ||
ErrKeyNotExists = terror.ClassSchema.New(codeKeyNotExists, " Key '%s' doesn't exist in table '%s'") | ||
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. Do we need replace " Key" with "Key"? 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. yes. |
||
// ErrMultiplePriKey returns for multiple primary keys. | ||
ErrMultiplePriKey = terror.ClassSchema.New(codeMultiplePriKey, "Multiple primary key defined") | ||
// ErrTooManyKeyParts returns for too many key parts. | ||
|
@@ -279,13 +283,15 @@ const ( | |
codeForeignKeyNotExists = 1091 | ||
codeWrongFkDef = 1239 | ||
|
||
codeDatabaseExists = 1007 | ||
codeTableExists = 1050 | ||
codeBadTable = 1051 | ||
codeColumnExists = 1060 | ||
codeIndexExists = 1831 | ||
codeMultiplePriKey = 1068 | ||
codeTooManyKeyParts = 1070 | ||
codeDatabaseExists = 1007 | ||
codeTableExists = 1050 | ||
codeBadTable = 1051 | ||
codeColumnExists = 1060 | ||
codeIndexExists = 1831 | ||
codeMultiplePriKey = 1068 | ||
codeTooManyKeyParts = 1070 | ||
codeKeyNameDuplicate = 1061 | ||
codeKeyNotExists = 1176 | ||
) | ||
|
||
func init() { | ||
|
@@ -304,6 +310,8 @@ func init() { | |
codeIndexExists: mysql.ErrDupIndex, | ||
codeMultiplePriKey: mysql.ErrMultiplePriKey, | ||
codeTooManyKeyParts: mysql.ErrTooManyKeyParts, | ||
codeKeyNameDuplicate: mysql.ErrDupKeyName, | ||
codeKeyNotExists: mysql.ErrKeyDoesNotExist, | ||
} | ||
terror.ErrClassToMySQLCodes[terror.ClassSchema] = schemaMySQLErrCodes | ||
initInfoSchemaDB() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1036,6 +1036,14 @@ AlterTableSpec: | |
NewTable: $3.(*ast.TableName), | ||
} | ||
} | ||
| "RENAME" KeyOrIndex Identifier "TO" Identifier | ||
{ | ||
$$ = &ast.AlterTableSpec{ | ||
Tp: ast.AlterTableRenameIndex, | ||
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. Keep alignment. 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. ok |
||
FromKey: model.NewCIStr($3), | ||
ToKey: model.NewCIStr($5), | ||
} | ||
} | ||
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. Please add tests in parser_test.go 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. ok |
||
| LockClause | ||
{ | ||
$$ = &ast.AlterTableSpec{ | ||
|
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.
Is the comment not correct?
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.
@lamxTyler ennnnn..... Forget to change this.. Will fix.