From f88140297810cd13e95d8a020e06157fde9bd563 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Wed, 2 Aug 2023 17:00:59 -0700 Subject: [PATCH 01/16] Working prototype of adding a new auto inc column --- enginetest/memory_engine_test.go | 50 ++++-- enginetest/queries/alter_table_queries.go | 23 +++ sql/analyzer/validate_create_table.go | 44 +++++- sql/parse/parse.go | 182 ++++++++++++---------- sql/plan/alter_table.go | 14 +- 5 files changed, 204 insertions(+), 109 deletions(-) diff --git a/enginetest/memory_engine_test.go b/enginetest/memory_engine_test.go index a8e674c9ae..b91e323384 100644 --- a/enginetest/memory_engine_test.go +++ b/enginetest/memory_engine_test.go @@ -270,30 +270,48 @@ func TestSingleQueryPrepared(t *testing.T) { // Convenience test for debugging a single query. Unskip and set to the desired query. func TestSingleScript(t *testing.T) { - t.Skip() + // t.Skip() var scripts = []queries.ScriptTest{ + // { + // Name: "ALTER TABLE MODIFY column with KEY", + // SetUpScript: []string{ + // "CREATE table test (pk int primary key, mk int, index (mk))", + // "ALTER TABLE `test` MODIFY column mk int auto_increment", + // }, + // Assertions: []queries.ScriptTestAssertion{ + // { + // Query: "show create table test", + // Expected: []sql.Row{{"test", + // "CREATE TABLE `test` (\n" + + // " `pk` int NOT NULL,\n" + + // " `mk` int AUTO_INCREMENT,\n" + + // " PRIMARY KEY (`pk`),\n" + + // " KEY `mk` (`mk`)\n" + + // ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + // }, + // }, + // }, { - Name: "renaming views with RENAME TABLE ... TO .. statement", + Name: "add column auto_increment", SetUpScript: []string{ - "create table t1 (id int primary key, v1 int);", - "create view v1 as select * from t1;", + "CREATE TABLE t1 (i bigint primary key, s varchar(20))", + "INSERT INTO t1 VALUES (1, 'a'), (2, 'b'), (3, 'c')", }, Assertions: []queries.ScriptTestAssertion{ { - Query: "rename table v1 to view1", - Expected: []sql.Row{{types.OkResult{RowsAffected: 0}}}, - }, - { - Query: "show tables;", - Expected: []sql.Row{{"myview"}, {"t1"}, {"view1"}}, - }, - { - Query: "rename table view1 to newViewName, t1 to newTableName", - Expected: []sql.Row{{types.OkResult{RowsAffected: 0}}}, + Query: "alter table t1 add column j int auto_increment unique", + Expected: []sql.Row{{types.NewOkResult(0)}}, }, { - Query: "show tables;", - Expected: []sql.Row{{"myview"}, {"newTableName"}, {"newViewName"}}, + Query: "show create table t1", + Expected: []sql.Row{{"t1", + "CREATE TABLE `t1` (\n" + + " `i` bigint NOT NULL,\n" + + " `s` varchar(20),\n" + + " `j` int AUTO_INCREMENT,\n" + + " PRIMARY KEY (`i`),\n" + + " UNIQUE KEY `j` (`j`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, }, }, diff --git a/enginetest/queries/alter_table_queries.go b/enginetest/queries/alter_table_queries.go index e37e595e5b..a3d8fb7314 100755 --- a/enginetest/queries/alter_table_queries.go +++ b/enginetest/queries/alter_table_queries.go @@ -355,4 +355,27 @@ var AlterTableScripts = []ScriptTest{ }, }, }, + { + Name: "add column auto_increment", + SetUpScript: []string{ + "CREATE TABLE t1 (i bigint primary key, s varchar(20))", + "INSERT INTO t1 VALUES (1, 'a'), (2, 'b'), (3, 'c')", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "alter table t1 add column j int auto_increment", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "show create table t1", + Expected: []sql.Row{{"t1", "CREATE TABLE `t1` (\n" + + " `i` bigint NOT NULL,\n" + + " `s` varchar(20),\n" + + " `j` int,\n" + + " PRIMARY KEY (`i`),\n" + + " UNIQUE KEY `j` (`j`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + }, + }, + }, } diff --git a/sql/analyzer/validate_create_table.go b/sql/analyzer/validate_create_table.go index db6d42cb8e..b97565d49f 100644 --- a/sql/analyzer/validate_create_table.go +++ b/sql/analyzer/validate_create_table.go @@ -101,7 +101,7 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S sch = sch.Copy() // Make a copy of the original schema to deal with any references to the original table. initialSch := sch - + // Need a TransformUp here because multiple of these statement types can be nested under a Block node. // It doesn't look it, but this is actually an iterative loop over all the independent clauses in an ALTER statement return transform.Node(n, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) { @@ -111,6 +111,7 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S if err != nil { return nil, transform.SameTree, err } + sch, err = validateModifyColumn(ctx, initialSch, sch, n.(*plan.ModifyColumn), keyedColumns) if err != nil { return nil, transform.SameTree, err @@ -127,12 +128,11 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S } return n, transform.NewTree, nil case *plan.AddColumn: - // TODO: can't `alter table add column j int unique auto_increment` as it ignores unique - // TODO: when above works, need to make sure unique index exists first then do what we did for modify n, err := nn.WithTargetSchema(sch.Copy()) if err != nil { return nil, transform.SameTree, err } + sch, err = validateAddColumn(initialSch, sch, n.(*plan.AddColumn), keyedColumns) if err != nil { return nil, transform.SameTree, err @@ -147,6 +147,8 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S if err != nil { return nil, transform.SameTree, err } + delete(keyedColumns, nn.Column) + return n, transform.NewTree, nil case *plan.AlterIndex: n, err := nn.WithTargetSchema(sch.Copy()) @@ -157,6 +159,8 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S if err != nil { return nil, transform.SameTree, err } + + keyedColumns = updateKeyedColumns(keyedColumns, nn) return n, transform.NewTree, nil case *plan.AlterPK: n, err := nn.WithTargetSchema(sch.Copy()) @@ -193,6 +197,28 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S }) } +func addKeyColumns(cols map[string]bool, columns map[string]bool) { + for col := range columns { + cols[col] = true + } +} + +// updateKeyedColumns updates the keyedColumns map based on the action of the AlterIndex node +func updateKeyedColumns(keyedColumns map[string]bool, n *plan.AlterIndex) map[string]bool { + switch n.Action { + case plan.IndexAction_Create: + for _, col := range n.Columns { + keyedColumns[col.Name] = true + } + case plan.IndexAction_Drop: + for _, col := range n.Columns { + delete(keyedColumns, col.Name) + } + } + + return keyedColumns +} + // validateRenameColumn checks that a DDL RenameColumn node can be safely executed (e.g. no collision with other // column names, doesn't invalidate any table check constraints). // @@ -266,9 +292,15 @@ func validateModifyColumn(ctx *sql.Context, initialSch sql.Schema, schema sql.Sc // Look for the old column and throw an error if it's not there. The column cannot have been renamed in the same // statement. This matches the MySQL behavior. - if !schema.Contains(mc.Column(), nameable.Name()) || - !initialSch.Contains(mc.Column(), nameable.Name()) { - return nil, sql.ErrTableColumnNotFound.New(nameable.Name(), mc.Column()) + if mc.IsPartOfAddColumn() { + if !schema.Contains(mc.Column(), nameable.Name()) { + return nil, sql.ErrTableColumnNotFound.New(nameable.Name(), mc.Column()) + } + } else { + if !schema.Contains(mc.Column(), nameable.Name()) || + !initialSch.Contains(mc.Column(), nameable.Name()) { + return nil, sql.ErrTableColumnNotFound.New(nameable.Name(), mc.Column()) + } } newSch := replaceInSchema(schema, mc.NewColumn(), nameable.Name()) diff --git a/sql/parse/parse.go b/sql/parse/parse.go index b1415e00f5..397fc98eb9 100644 --- a/sql/parse/parse.go +++ b/sql/parse/parse.go @@ -19,7 +19,6 @@ import ( goerrors "errors" "fmt" "regexp" - "sort" "strconv" "strings" "time" @@ -1332,76 +1331,76 @@ func convertAlterTable(ctx *sql.Context, query string, c *sqlparser.AlterTable) return statements[0], nil } - // TODO: add correct precedence for ADD/DROP PRIMARY KEY and (maybe) FOREIGN KEY - // certain alter statements need to happen before others - sort.Slice(statements, func(i, j int) bool { - switch ii := statements[i].(type) { - case *plan.RenameColumn: - switch statements[j].(type) { - case *plan.DropColumn, - *plan.ModifyColumn, - *plan.AddColumn, - *plan.DropConstraint, - *plan.DropCheck, - *plan.CreateCheck, - *plan.AlterIndex: - return true - } - case *plan.DropColumn: - switch statements[j].(type) { - case *plan.ModifyColumn, - *plan.AddColumn, - *plan.DropConstraint, - *plan.DropCheck, - *plan.CreateCheck, - *plan.AlterIndex: - return true - } - case *plan.ModifyColumn: - switch statements[j].(type) { - case *plan.AddColumn, - *plan.DropConstraint, - *plan.DropCheck, - *plan.CreateCheck, - *plan.AlterIndex: - return true - } - case *plan.AddColumn: - switch statements[j].(type) { - case *plan.DropConstraint, - *plan.DropCheck, - *plan.CreateCheck, - *plan.AlterIndex: - return true - } - case *plan.DropConstraint: - switch statements[j].(type) { - case *plan.DropCheck, - *plan.CreateCheck, - *plan.AlterIndex: - return true - } - case *plan.DropCheck: - switch statements[j].(type) { - case *plan.CreateCheck, - *plan.AlterIndex: - return true - } - case *plan.CreateCheck: - switch statements[j].(type) { - case *plan.AlterIndex: - return true - } - // AlterIndex precedence is Rename, Drop, then Create - // So statement[i] < statement[j] = statement[i].action > statement[j].action - case *plan.AlterIndex: - switch jj := statements[j].(type) { - case *plan.AlterIndex: - return ii.Action > jj.Action - } - } - return false - }) + // // TODO: add correct precedence for ADD/DROP PRIMARY KEY and (maybe) FOREIGN KEY + // // certain alter statements need to happen before others + // sort.Slice(statements, func(i, j int) bool { + // switch ii := statements[i].(type) { + // case *plan.RenameColumn: + // switch statements[j].(type) { + // case *plan.DropColumn, + // *plan.ModifyColumn, + // *plan.AddColumn, + // *plan.DropConstraint, + // *plan.DropCheck, + // *plan.CreateCheck, + // *plan.AlterIndex: + // return true + // } + // case *plan.DropColumn: + // switch statements[j].(type) { + // case *plan.ModifyColumn, + // *plan.AddColumn, + // *plan.DropConstraint, + // *plan.DropCheck, + // *plan.CreateCheck, + // *plan.AlterIndex: + // return true + // } + // case *plan.ModifyColumn: + // switch statements[j].(type) { + // case *plan.AddColumn, + // *plan.DropConstraint, + // *plan.DropCheck, + // *plan.CreateCheck, + // *plan.AlterIndex: + // return true + // } + // case *plan.AddColumn: + // switch statements[j].(type) { + // case *plan.DropConstraint, + // *plan.DropCheck, + // *plan.CreateCheck, + // *plan.AlterIndex: + // return true + // } + // case *plan.DropConstraint: + // switch statements[j].(type) { + // case *plan.DropCheck, + // *plan.CreateCheck, + // *plan.AlterIndex: + // return true + // } + // case *plan.DropCheck: + // switch statements[j].(type) { + // case *plan.CreateCheck, + // *plan.AlterIndex: + // return true + // } + // case *plan.CreateCheck: + // switch statements[j].(type) { + // case *plan.AlterIndex: + // return true + // } + // // AlterIndex precedence is Rename, Drop, then Create + // // So statement[i] < statement[j] = statement[i].action > statement[j].action + // case *plan.AlterIndex: + // switch jj := statements[j].(type) { + // case *plan.AlterIndex: + // return ii.Action > jj.Action + // } + // } + // return false + // }) return plan.NewBlock(statements), nil } @@ -2093,16 +2092,9 @@ func convertRenameTable(ctx *sql.Context, ddl *sqlparser.DDL, alterTbl bool) (sq return plan.NewRenameTable(sql.UnresolvedDatabase(""), fromTables, toTables, alterTbl), nil } - -func isUniqueColumn(tableSpec *sqlparser.TableSpec, columnName string) (bool, error) { - for _, column := range tableSpec.Columns { - if column.Name.String() == columnName { +func isUniqueColumn(column *sqlparser.ColumnDefinition) bool { return column.Type.KeyOpt == colKeyUnique || - column.Type.KeyOpt == colKeyUniqueKey, nil - } - } - return false, fmt.Errorf("unknown column name %s", columnName) - + column.Type.KeyOpt == colKeyUniqueKey } func newColumnAction(ctx *sql.Context, ddl *sqlparser.DDL) (sql.Node, error) { @@ -2112,7 +2104,12 @@ func newColumnAction(ctx *sql.Context, ddl *sqlparser.DDL) (sql.Node, error) { if err != nil { return nil, err } - return plan.NewAddColumn(sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), tableNameToUnresolvedTable(ddl.Table), sch.Schema[0], columnOrderToColumnOrder(ddl.ColumnOrder)), nil + + // auto_increment will be added in an additional pass, so scrub it for now + column := sch.Schema[0] + column.AutoIncrement = false + + return plan.NewAddColumn(sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), tableNameToUnresolvedTable(ddl.Table), column, columnOrderToColumnOrder(ddl.ColumnOrder)), nil case sqlparser.DropStr: return plan.NewDropColumn(sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), tableNameToUnresolvedTable(ddl.Table), ddl.Column.String()), nil case sqlparser.RenameStr: @@ -2146,10 +2143,7 @@ func convertAlterTableClause(ctx *sql.Context, query string, ddl *sqlparser.DDL) } column := ddl.TableSpec.Columns[0] - isUnique, err := isUniqueColumn(ddl.TableSpec, column.Name.String()) - if err != nil { - return nil, fmt.Errorf("on table %s, %w", ddl.Table.String(), err) - } + isUnique := isUniqueColumn(column) if isUnique { createIndex := plan.NewAlterCreateIndex( @@ -2163,6 +2157,24 @@ func convertAlterTableClause(ctx *sql.Context, query string, ddl *sqlparser.DDL) ) ddlNodes = append(ddlNodes, createIndex) } + + if column.Type.Autoincrement { + sch, _, err := TableSpecToSchema(ctx, ddl.TableSpec, true) + if err != nil { + return nil, err + } + + modifyAction := plan.NewModifyColumn( + sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), + tableNameToUnresolvedTable(ddl.Table), + column.Name.String(), + sch.Schema[0], + columnOrderToColumnOrder(ddl.ColumnOrder), + ) + + modifyAction.MarkAddColumn() + ddlNodes = append(ddlNodes, modifyAction) + } } } diff --git a/sql/plan/alter_table.go b/sql/plan/alter_table.go index 4df9b02725..ad7c5ffb12 100644 --- a/sql/plan/alter_table.go +++ b/sql/plan/alter_table.go @@ -693,8 +693,9 @@ func (*RenameColumn) CollationCoercibility(ctx *sql.Context) (collation sql.Coll type ModifyColumn struct { ddlNode Table sql.Node - columnName string - column *sql.Column + columnName string + partOfAddColumn bool + column *sql.Column order *sql.ColumnOrder targetSchema sql.Schema } @@ -727,6 +728,15 @@ func NewModifyColumn(database sql.Database, table *UnresolvedTable, columnName s } } +func (m *ModifyColumn) MarkAddColumn() { + m.partOfAddColumn = true +} + +// IsPartOfAddColumn returns if this node is part of an AddColumn operation, as oposed to a stand-alone modify operation. +func (m *ModifyColumn) IsPartOfAddColumn() bool { + return m.partOfAddColumn +} + func (m *ModifyColumn) WithDatabase(db sql.Database) (sql.Node, error) { nm := *m nm.Db = db From dff6d0450131f0ab352714a8dbc91da61a52773d Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Wed, 2 Aug 2023 17:38:00 -0700 Subject: [PATCH 02/16] Rewrote engine test as scripts --- enginetest/enginetests.go | 80 ---------------- enginetest/memory_engine_test.go | 12 ++- enginetest/queries/alter_table_queries.go | 106 ++++++++++++++++++++++ sql/analyzer/validate_create_table.go | 2 +- 4 files changed, 118 insertions(+), 82 deletions(-) diff --git a/enginetest/enginetests.go b/enginetest/enginetests.go index adbb282c7a..3bdfa13fc7 100644 --- a/enginetest/enginetests.go +++ b/enginetest/enginetests.go @@ -5415,86 +5415,6 @@ func TestAddDropPks(t *testing.T, harness Harness) { }) } -func TestAddAutoIncrementColumn(t *testing.T, harness Harness) { - harness.Setup([]setup.SetupScript{{ - "create database mydb", - "use mydb", - }}) - e := mustNewEngine(t, harness) - defer e.Close() - ctx := NewContext(harness) - - t.Run("Add primary key column with auto increment", func(t *testing.T) { - ctx.SetCurrentDatabase("mydb") - RunQuery(t, e, harness, "CREATE TABLE t1 (i int, j int);") - RunQuery(t, e, harness, "insert into t1 values (1,1), (2,2), (3,3)") - AssertErr( - t, e, harness, - "alter table t1 add column pk int primary key;", - sql.ErrPrimaryKeyViolation, - ) - - TestQueryWithContext( - t, ctx, e, harness, - "alter table t1 add column pk int primary key auto_increment;", - []sql.Row{{types.NewOkResult(0)}}, - nil, nil, - ) - - TestQueryWithContext( - t, ctx, e, harness, - "select pk from t1;", - []sql.Row{ - {1}, - {2}, - {3}, - }, - nil, nil, - ) - - TestQueryWithContext( - t, ctx, e, harness, - "show create table t1;", - []sql.Row{ - {"t1", "CREATE TABLE `t1` (\n `i` int,\n `j` int,\n `pk` int NOT NULL AUTO_INCREMENT,\n PRIMARY KEY (`pk`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, - }, - nil, nil, - ) - }) - - t.Run("Add primary key column with auto increment first", func(t *testing.T) { - ctx.SetCurrentDatabase("mydb") - RunQuery(t, e, harness, "CREATE TABLE t2 (i int, j int);") - RunQuery(t, e, harness, "insert into t2 values (1,1), (2,2), (3,3)") - TestQueryWithContext( - t, ctx, e, harness, - "alter table t2 add column pk int primary key auto_increment first;", - []sql.Row{{types.NewOkResult(0)}}, - nil, nil, - ) - - TestQueryWithContext( - t, ctx, e, harness, - "select pk from t2;", - []sql.Row{ - {1}, - {2}, - {3}, - }, - nil, nil, - ) - - TestQueryWithContext( - t, ctx, e, harness, - "show create table t2;", - []sql.Row{ - {"t2", "CREATE TABLE `t2` (\n `pk` int NOT NULL AUTO_INCREMENT,\n `i` int,\n `j` int,\n PRIMARY KEY (`pk`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, - }, - nil, nil, - ) - }) -} - func TestNullRanges(t *testing.T, harness Harness) { harness.Setup(setup.NullsSetup...) for _, tt := range queries.NullRangeTests { diff --git a/enginetest/memory_engine_test.go b/enginetest/memory_engine_test.go index b91e323384..0b4aa95eb8 100644 --- a/enginetest/memory_engine_test.go +++ b/enginetest/memory_engine_test.go @@ -313,6 +313,14 @@ func TestSingleScript(t *testing.T) { " UNIQUE KEY `j` (`j`)\n" + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, + { + Query: "select * from t1 order by i", + Expected: []sql.Row{ + {1, "a", 1}, + {2, "b", 2}, + {3, "c", 3}, + }, + }, }, }, } @@ -1325,7 +1333,9 @@ func TestAddDropPks_Exp(t *testing.T) { func TestAddAutoIncrementColumn(t *testing.T) { t.Skip("in memory tables don't implement sql.RewritableTable yet") - enginetest.TestAddAutoIncrementColumn(t, enginetest.NewDefaultMemoryHarness()) + for _, script := range queries.AlterTableAddAutoIncrementScripts { + enginetest.TestScript(t, enginetest.NewDefaultMemoryHarness(), script) + } } func TestNullRanges(t *testing.T) { diff --git a/enginetest/queries/alter_table_queries.go b/enginetest/queries/alter_table_queries.go index a3d8fb7314..ca3eba282d 100755 --- a/enginetest/queries/alter_table_queries.go +++ b/enginetest/queries/alter_table_queries.go @@ -379,3 +379,109 @@ var AlterTableScripts = []ScriptTest{ }, }, } + +var AlterTableAddAutoIncrementScripts = []ScriptTest{ + { + Name: "Add primary key column with auto increment", + SetUpScript: []string{ + "CREATE TABLE t1 (i int, j int);", + "insert into t1 values (1,1), (2,2), (3,3)", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "alter table t1 add column pk int primary key", + ExpectedErr: sql.ErrPrimaryKeyViolation, + }, + { + Query: "alter table t1 add column pk int primary key auto_increment;", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "show create table t1", + Expected: []sql.Row{}, + }, + { + Query: "select * from t1 order by pk", + Expected: []sql.Row{ + {1, 1, 1}, + {2, 2, 2}, + {3, 3, 3}, + }, + }, + }, + }, + { + Name: "Add primary key column with auto increment, first", + SetUpScript: []string{ + "CREATE TABLE t1 (i int, j int);", + "insert into t1 values (1,1), (2,2), (3,3)", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "alter table t1 add column pk int primary key", + ExpectedErr: sql.ErrPrimaryKeyViolation, + }, + { + Query: "alter table t1 add column pk int primary key auto_increment;", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "show create table t1", + Expected: []sql.Row{}, + }, + { + Query: "select * from t1 order by pk", + Expected: []sql.Row{ + {1, 1, 1}, + {2, 2, 2}, + {3, 3, 3}, + }, + }, + }, + }, + { + Name: "add column auto_increment, non primary key", + SetUpScript: []string{ + "CREATE TABLE t1 (i bigint primary key, s varchar(20))", + "INSERT INTO t1 VALUES (1, 'a'), (2, 'b'), (3, 'c')", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "alter table t1 add column j int auto_increment unique", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "show create table t1", + Expected: []sql.Row{{"t1", + "CREATE TABLE `t1` (\n" + + " `i` bigint NOT NULL,\n" + + " `s` varchar(20),\n" + + " `j` int AUTO_INCREMENT,\n" + + " PRIMARY KEY (`i`),\n" + + " UNIQUE KEY `j` (`j`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + }, + { + Query: "select * from t1 order by i", + Expected: []sql.Row{ + {1, "a", 1}, + {2, "b", 2}, + {3, "c", 3}, + }, + }, + }, + }, + { + Name: "add column auto_increment, non key", + SetUpScript: []string{ + "CREATE TABLE t1 (i bigint primary key, s varchar(20))", + "INSERT INTO t1 VALUES (1, 'a'), (2, 'b'), (3, 'c')", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "alter table t1 add column j int auto_increment", + ExpectedErr: sql.ErrInvalidAutoIncCols, + }, + }, + }, +} \ No newline at end of file diff --git a/sql/analyzer/validate_create_table.go b/sql/analyzer/validate_create_table.go index b97565d49f..f7bc64a070 100644 --- a/sql/analyzer/validate_create_table.go +++ b/sql/analyzer/validate_create_table.go @@ -59,7 +59,7 @@ func validateCreateTable(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan. func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope, sel RuleSelector) (sql.Node, transform.TreeIdentity, error) { var sch sql.Schema var indexes []string - var keyedColumns map[string]bool + keyedColumns := make(map[string]bool) var err error transform.Inspect(n, func(n sql.Node) bool { switch n := n.(type) { From 3a5aeaa7c4840adf3dcbf12fcc1a96a81a6a752c Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Wed, 2 Aug 2023 17:48:27 -0700 Subject: [PATCH 03/16] Moved some scripts around --- enginetest/queries/alter_table_queries.go | 60 ++++++++++++++++++----- enginetest/queries/script_queries.go | 57 --------------------- 2 files changed, 47 insertions(+), 70 deletions(-) diff --git a/enginetest/queries/alter_table_queries.go b/enginetest/queries/alter_table_queries.go index ca3eba282d..109f1d3655 100755 --- a/enginetest/queries/alter_table_queries.go +++ b/enginetest/queries/alter_table_queries.go @@ -356,25 +356,59 @@ var AlterTableScripts = []ScriptTest{ }, }, { - Name: "add column auto_increment", + Name: "multi-alter ddl column statements", SetUpScript: []string{ - "CREATE TABLE t1 (i bigint primary key, s varchar(20))", - "INSERT INTO t1 VALUES (1, 'a'), (2, 'b'), (3, 'c')", + "create table tbl_i (i int primary key)", + "create table tbl_ij (i int primary key, j int)", }, Assertions: []ScriptTestAssertion{ { - Query: "alter table t1 add column j int auto_increment", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Query: "alter table tbl_i add column j int, drop column j", + ExpectedErr: sql.ErrTableColumnNotFound, }, { - Query: "show create table t1", - Expected: []sql.Row{{"t1", "CREATE TABLE `t1` (\n" + - " `i` bigint NOT NULL,\n" + - " `s` varchar(20),\n" + - " `j` int,\n" + - " PRIMARY KEY (`i`),\n" + - " UNIQUE KEY `j` (`j`)\n" + - ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + Query: "alter table tbl_i add column j int, rename column j to k;", + ExpectedErr: sql.ErrTableColumnNotFound, + }, + { + Query: "alter table tbl_i add column j int, modify column j varchar(10)", + ExpectedErr: sql.ErrTableColumnNotFound, + }, + { + Query: "alter table tbl_ij add index (j), drop column j;", + ExpectedErr: sql.ErrKeyColumnDoesNotExist, + }, + { + Query: "alter table tbl_ij drop column j, rename column j to k;", + ExpectedErr: sql.ErrTableColumnNotFound, + }, + { + Query: "alter table tbl_ij drop column k, rename column j to k;", + ExpectedErr: sql.ErrTableColumnNotFound, + }, + { + Query: "alter table tbl_i add index(j), add column j int;", + Expected: []sql.Row{ + {types.NewOkResult(0)}, + }, + }, + { + Query: "show create table tbl_i", + Expected: []sql.Row{ + {"tbl_i", "CREATE TABLE `tbl_i` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, + }, + }, + { + Query: "alter table tbl_ij add index (j), drop column j, add column j int;", + Expected: []sql.Row{ + {types.NewOkResult(0)}, + }, + }, + { + Query: "show create table tbl_ij", + Expected: []sql.Row{ + {"tbl_ij", "CREATE TABLE `tbl_ij` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, + }, }, }, }, diff --git a/enginetest/queries/script_queries.go b/enginetest/queries/script_queries.go index e73873fa9d..39cfd723f7 100644 --- a/enginetest/queries/script_queries.go +++ b/enginetest/queries/script_queries.go @@ -3100,63 +3100,6 @@ var ScriptTests = []ScriptTest{ }, }, }, - { - Name: "multi-alter ddl column statements", - SetUpScript: []string{ - "create table tbl_i (i int primary key)", - "create table tbl_ij (i int primary key, j int)", - }, - Assertions: []ScriptTestAssertion{ - { - Query: "alter table tbl_i add column j int, drop column j", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_i add column j int, rename column j to k;", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_i add column j int, modify column j varchar(10)", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_ij add index (j), drop column j;", - ExpectedErr: sql.ErrKeyColumnDoesNotExist, - }, - { - Query: "alter table tbl_ij drop column j, rename column j to k;", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_ij drop column k, rename column j to k;", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_i add index(j), add column j int;", - Expected: []sql.Row{ - {types.NewOkResult(0)}, - }, - }, - { - Query: "show create table tbl_i", - Expected: []sql.Row{ - {"tbl_i", "CREATE TABLE `tbl_i` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, - }, - }, - { - Query: "alter table tbl_ij add index (j), drop column j, add column j int;", - Expected: []sql.Row{ - {types.NewOkResult(0)}, - }, - }, - { - Query: "show create table tbl_ij", - Expected: []sql.Row{ - {"tbl_ij", "CREATE TABLE `tbl_ij` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, - }, - }, - }, - }, { Name: "Keyless Table with Unique Index", SetUpScript: []string{ From 05c4e11ede52cc1176117db4d1ca981feeffcbf9 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 11:01:30 -0700 Subject: [PATCH 04/16] Getting rid of alter table statement sorting --- enginetest/memory_engine_test.go | 61 +++++++++++++------ enginetest/queries/alter_table_queries.go | 28 +-------- sql/parse/parse.go | 73 +---------------------- 3 files changed, 46 insertions(+), 116 deletions(-) diff --git a/enginetest/memory_engine_test.go b/enginetest/memory_engine_test.go index 0b4aa95eb8..5207fe0070 100644 --- a/enginetest/memory_engine_test.go +++ b/enginetest/memory_engine_test.go @@ -292,33 +292,58 @@ func TestSingleScript(t *testing.T) { // }, // }, { - Name: "add column auto_increment", + Name: "multi-alter ddl column statements", SetUpScript: []string{ - "CREATE TABLE t1 (i bigint primary key, s varchar(20))", - "INSERT INTO t1 VALUES (1, 'a'), (2, 'b'), (3, 'c')", + "create table tbl_i (i int primary key)", + "create table tbl_ij (i int primary key, j int)", }, Assertions: []queries.ScriptTestAssertion{ { - Query: "alter table t1 add column j int auto_increment unique", - Expected: []sql.Row{{types.NewOkResult(0)}}, + Query: "alter table tbl_i add column j int, drop column j", + ExpectedErr: sql.ErrTableColumnNotFound, }, { - Query: "show create table t1", - Expected: []sql.Row{{"t1", - "CREATE TABLE `t1` (\n" + - " `i` bigint NOT NULL,\n" + - " `s` varchar(20),\n" + - " `j` int AUTO_INCREMENT,\n" + - " PRIMARY KEY (`i`),\n" + - " UNIQUE KEY `j` (`j`)\n" + - ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + Query: "alter table tbl_i add column j int, rename column j to k;", + ExpectedErr: sql.ErrTableColumnNotFound, }, { - Query: "select * from t1 order by i", + Query: "alter table tbl_i add column j int, modify column j varchar(10)", + ExpectedErr: sql.ErrTableColumnNotFound, + }, + { + Query: "alter table tbl_ij add index (j), drop column j;", + ExpectedErr: sql.ErrKeyColumnDoesNotExist, + }, + { + Query: "alter table tbl_ij drop column j, rename column j to k;", + ExpectedErr: sql.ErrTableColumnNotFound, + }, + { + Query: "alter table tbl_ij drop column k, rename column j to k;", + ExpectedErr: sql.ErrTableColumnNotFound, + }, + { + Query: "alter table tbl_i add index(j), add column j int;", + Expected: []sql.Row{ + {types.NewOkResult(0)}, + }, + }, + { + Query: "show create table tbl_i", + Expected: []sql.Row{ + {"tbl_i", "CREATE TABLE `tbl_i` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, + }, + }, + { + Query: "alter table tbl_ij add index (j), drop column j, add column j int;", + Expected: []sql.Row{ + {types.NewOkResult(0)}, + }, + }, + { + Query: "show create table tbl_ij", Expected: []sql.Row{ - {1, "a", 1}, - {2, "b", 2}, - {3, "c", 3}, + {"tbl_ij", "CREATE TABLE `tbl_ij` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, }, }, }, diff --git a/enginetest/queries/alter_table_queries.go b/enginetest/queries/alter_table_queries.go index 109f1d3655..d308005f72 100755 --- a/enginetest/queries/alter_table_queries.go +++ b/enginetest/queries/alter_table_queries.go @@ -356,7 +356,7 @@ var AlterTableScripts = []ScriptTest{ }, }, { - Name: "multi-alter ddl column statements", + Name: "multi-alter ddl column errors", SetUpScript: []string{ "create table tbl_i (i int primary key)", "create table tbl_ij (i int primary key, j int)", @@ -374,10 +374,6 @@ var AlterTableScripts = []ScriptTest{ Query: "alter table tbl_i add column j int, modify column j varchar(10)", ExpectedErr: sql.ErrTableColumnNotFound, }, - { - Query: "alter table tbl_ij add index (j), drop column j;", - ExpectedErr: sql.ErrKeyColumnDoesNotExist, - }, { Query: "alter table tbl_ij drop column j, rename column j to k;", ExpectedErr: sql.ErrTableColumnNotFound, @@ -388,27 +384,7 @@ var AlterTableScripts = []ScriptTest{ }, { Query: "alter table tbl_i add index(j), add column j int;", - Expected: []sql.Row{ - {types.NewOkResult(0)}, - }, - }, - { - Query: "show create table tbl_i", - Expected: []sql.Row{ - {"tbl_i", "CREATE TABLE `tbl_i` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, - }, - }, - { - Query: "alter table tbl_ij add index (j), drop column j, add column j int;", - Expected: []sql.Row{ - {types.NewOkResult(0)}, - }, - }, - { - Query: "show create table tbl_ij", - Expected: []sql.Row{ - {"tbl_ij", "CREATE TABLE `tbl_ij` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, - }, + ExpectedErr: sql.ErrKeyColumnDoesNotExist, }, }, }, diff --git a/sql/parse/parse.go b/sql/parse/parse.go index 397fc98eb9..dbcdcd5cde 100644 --- a/sql/parse/parse.go +++ b/sql/parse/parse.go @@ -1330,78 +1330,7 @@ func convertAlterTable(ctx *sql.Context, query string, c *sqlparser.AlterTable) if len(statements) == 1 { return statements[0], nil } - - // // TODO: add correct precedence for ADD/DROP PRIMARY KEY and (maybe) FOREIGN KEY - // // certain alter statements need to happen before others - // sort.Slice(statements, func(i, j int) bool { - // switch ii := statements[i].(type) { - // case *plan.RenameColumn: - // switch statements[j].(type) { - // case *plan.DropColumn, - // *plan.ModifyColumn, - // *plan.AddColumn, - // *plan.DropConstraint, - // *plan.DropCheck, - // *plan.CreateCheck, - // *plan.AlterIndex: - // return true - // } - // case *plan.DropColumn: - // switch statements[j].(type) { - // case *plan.ModifyColumn, - // *plan.AddColumn, - // *plan.DropConstraint, - // *plan.DropCheck, - // *plan.CreateCheck, - // *plan.AlterIndex: - // return true - // } - // case *plan.ModifyColumn: - // switch statements[j].(type) { - // case *plan.AddColumn, - // *plan.DropConstraint, - // *plan.DropCheck, - // *plan.CreateCheck, - // *plan.AlterIndex: - // return true - // } - // case *plan.AddColumn: - // switch statements[j].(type) { - // case *plan.DropConstraint, - // *plan.DropCheck, - // *plan.CreateCheck, - // *plan.AlterIndex: - // return true - // } - // case *plan.DropConstraint: - // switch statements[j].(type) { - // case *plan.DropCheck, - // *plan.CreateCheck, - // *plan.AlterIndex: - // return true - // } - // case *plan.DropCheck: - // switch statements[j].(type) { - // case *plan.CreateCheck, - // *plan.AlterIndex: - // return true - // } - // case *plan.CreateCheck: - // switch statements[j].(type) { - // case *plan.AlterIndex: - // return true - // } - // // AlterIndex precedence is Rename, Drop, then Create - // // So statement[i] < statement[j] = statement[i].action > statement[j].action - // case *plan.AlterIndex: - // switch jj := statements[j].(type) { - // case *plan.AlterIndex: - // return ii.Action > jj.Action - // } - // } - // return false - // }) - + return plan.NewBlock(statements), nil } From a2057616d68492c54ca8a771077c5c991dcfa151 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 13:17:09 -0700 Subject: [PATCH 05/16] Better tests, still experimenting --- enginetest/queries/alter_table_queries.go | 34 +++++++++++---------- sql/analyzer/validate_create_table.go | 6 ---- sql/parse/parse.go | 36 +++++++++++------------ 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/enginetest/queries/alter_table_queries.go b/enginetest/queries/alter_table_queries.go index d308005f72..1778fe3815 100755 --- a/enginetest/queries/alter_table_queries.go +++ b/enginetest/queries/alter_table_queries.go @@ -398,24 +398,24 @@ var AlterTableAddAutoIncrementScripts = []ScriptTest{ "insert into t1 values (1,1), (2,2), (3,3)", }, Assertions: []ScriptTestAssertion{ - { - Query: "alter table t1 add column pk int primary key", - ExpectedErr: sql.ErrPrimaryKeyViolation, - }, { Query: "alter table t1 add column pk int primary key auto_increment;", Expected: []sql.Row{{types.NewOkResult(0)}}, }, { Query: "show create table t1", - Expected: []sql.Row{}, + Expected: []sql.Row{{"t1", + "CREATE TABLE `t1` (\n" + + " `i` int,\n" + + " `j` int,\n" + + " `pk` int NOT NULL AUTO_INCREMENT,\n" + + " PRIMARY KEY (`pk`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, { - Query: "select * from t1 order by pk", + Query: "select pk from t1 order by pk", Expected: []sql.Row{ - {1, 1, 1}, - {2, 2, 2}, - {3, 3, 3}, + {1}, {2}, {3}, }, }, }, @@ -432,19 +432,23 @@ var AlterTableAddAutoIncrementScripts = []ScriptTest{ ExpectedErr: sql.ErrPrimaryKeyViolation, }, { - Query: "alter table t1 add column pk int primary key auto_increment;", + Query: "alter table t1 add column pk int primary key auto_increment first", Expected: []sql.Row{{types.NewOkResult(0)}}, }, { Query: "show create table t1", - Expected: []sql.Row{}, + Expected: []sql.Row{{"t1", + "CREATE TABLE `t1` (\n" + + " `pk` int NOT NULL AUTO_INCREMENT,\n" + + " `i` int,\n" + + " `j` int,\n" + + " PRIMARY KEY (`pk`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, { - Query: "select * from t1 order by pk", + Query: "select pk from t1 order by pk", Expected: []sql.Row{ - {1, 1, 1}, - {2, 2, 2}, - {3, 3, 3}, + {1}, {2}, {3}, }, }, }, diff --git a/sql/analyzer/validate_create_table.go b/sql/analyzer/validate_create_table.go index f7bc64a070..041369fac9 100644 --- a/sql/analyzer/validate_create_table.go +++ b/sql/analyzer/validate_create_table.go @@ -197,12 +197,6 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S }) } -func addKeyColumns(cols map[string]bool, columns map[string]bool) { - for col := range columns { - cols[col] = true - } -} - // updateKeyedColumns updates the keyedColumns map based on the action of the AlterIndex node func updateKeyedColumns(keyedColumns map[string]bool, n *plan.AlterIndex) map[string]bool { switch n.Action { diff --git a/sql/parse/parse.go b/sql/parse/parse.go index dbcdcd5cde..06c6c62f35 100644 --- a/sql/parse/parse.go +++ b/sql/parse/parse.go @@ -2036,7 +2036,7 @@ func newColumnAction(ctx *sql.Context, ddl *sqlparser.DDL) (sql.Node, error) { // auto_increment will be added in an additional pass, so scrub it for now column := sch.Schema[0] - column.AutoIncrement = false + // column.AutoIncrement = false return plan.NewAddColumn(sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), tableNameToUnresolvedTable(ddl.Table), column, columnOrderToColumnOrder(ddl.ColumnOrder)), nil case sqlparser.DropStr: @@ -2087,23 +2087,23 @@ func convertAlterTableClause(ctx *sql.Context, query string, ddl *sqlparser.DDL) ddlNodes = append(ddlNodes, createIndex) } - if column.Type.Autoincrement { - sch, _, err := TableSpecToSchema(ctx, ddl.TableSpec, true) - if err != nil { - return nil, err - } - - modifyAction := plan.NewModifyColumn( - sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), - tableNameToUnresolvedTable(ddl.Table), - column.Name.String(), - sch.Schema[0], - columnOrderToColumnOrder(ddl.ColumnOrder), - ) - - modifyAction.MarkAddColumn() - ddlNodes = append(ddlNodes, modifyAction) - } + // if column.Type.Autoincrement { + // sch, _, err := TableSpecToSchema(ctx, ddl.TableSpec, true) + // if err != nil { + // return nil, err + // } + // + // modifyAction := plan.NewModifyColumn( + // sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), + // tableNameToUnresolvedTable(ddl.Table), + // column.Name.String(), + // sch.Schema[0], + // columnOrderToColumnOrder(ddl.ColumnOrder), + // ) + // + // modifyAction.MarkAddColumn() + // ddlNodes = append(ddlNodes, modifyAction) + // } } } From e2c6eb70dd12686e2836164f254b93db5c22b8b6 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 14:00:25 -0700 Subject: [PATCH 06/16] Made adding an auto increment column rewrite the table --- sql/analyzer/validate_create_table.go | 28 ++++++++++++++++++++++----- sql/rowexec/ddl_iters.go | 2 +- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/sql/analyzer/validate_create_table.go b/sql/analyzer/validate_create_table.go index 041369fac9..b24d96ba03 100644 --- a/sql/analyzer/validate_create_table.go +++ b/sql/analyzer/validate_create_table.go @@ -48,7 +48,7 @@ func validateCreateTable(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan. } } - err = validateAutoIncrement(ct.CreateSchema.Schema, keyedColumns) + err = validateAutoIncrementModify(ct.CreateSchema.Schema, keyedColumns) if err != nil { return nil, transform.SameTree, err } @@ -271,8 +271,7 @@ func validateAddColumn(initialSch sql.Schema, schema sql.Schema, ac *plan.AddCol newSch = append(newSch, ac.Column().Copy()) } - // TODO: more validation possible to do here - err := validateAutoIncrement(newSch, keyedColumns) + err := validateAutoIncrementAdd(newSch) if err != nil { return nil, err } @@ -299,7 +298,7 @@ func validateModifyColumn(ctx *sql.Context, initialSch sql.Schema, schema sql.Sc newSch := replaceInSchema(schema, mc.NewColumn(), nameable.Name()) - err := validateAutoIncrement(newSch, keyedColumns) + err := validateAutoIncrementModify(newSch, keyedColumns) if err != nil { return nil, err } @@ -623,7 +622,8 @@ func removeInSchema(sch sql.Schema, colName, tableName string) sql.Schema { return schCopy } -func validateAutoIncrement(schema sql.Schema, keyedColumns map[string]bool) error { +// TODO: make this work for CREATE TABLE statements where there's a non-pk auto increment column +func validateAutoIncrementModify(schema sql.Schema, keyedColumns map[string]bool) error { seen := false for _, col := range schema { if col.AutoIncrement { @@ -646,6 +646,24 @@ func validateAutoIncrement(schema sql.Schema, keyedColumns map[string]bool) erro return nil } +func validateAutoIncrementAdd(schema sql.Schema) error { + seen := false + for _, col := range schema { + if col.AutoIncrement { + if col.Default != nil { + // AUTO_INCREMENT col cannot have default + return sql.ErrInvalidAutoIncCols.New() + } + if seen { + // there can be at most one AUTO_INCREMENT col + return sql.ErrInvalidAutoIncCols.New() + } + seen = true + } + } + return nil +} + const textIndexPrefix = 1000 // validateIndexes prevents creating tables with blob/text primary keys and indexes without a specified length diff --git a/sql/rowexec/ddl_iters.go b/sql/rowexec/ddl_iters.go index 391a5d42b1..1be7f588a6 100644 --- a/sql/rowexec/ddl_iters.go +++ b/sql/rowexec/ddl_iters.go @@ -1333,7 +1333,7 @@ func (i *addColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTable) oldPkSchema, newPkSchema := sql.SchemaToPrimaryKeySchema(rwt, rwt.Schema()), sql.SchemaToPrimaryKeySchema(rwt, newSch) rewriteRequired := false - if i.a.Column().Default != nil || !i.a.Column().Nullable { + if i.a.Column().Default != nil || !i.a.Column().Nullable || i.a.Column().AutoIncrement { rewriteRequired = true } From 612d37bb1973ec6ccbfc2cd58712c96ceac95f93 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 14:31:24 -0700 Subject: [PATCH 07/16] Better validation for adding an auto-inc column --- sql/analyzer/validate_create_table.go | 54 ++++++++++++++++++--------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/sql/analyzer/validate_create_table.go b/sql/analyzer/validate_create_table.go index b24d96ba03..ae1defd657 100644 --- a/sql/analyzer/validate_create_table.go +++ b/sql/analyzer/validate_create_table.go @@ -102,9 +102,11 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S sch = sch.Copy() // Make a copy of the original schema to deal with any references to the original table. initialSch := sch + addedColumn := false + // Need a TransformUp here because multiple of these statement types can be nested under a Block node. // It doesn't look it, but this is actually an iterative loop over all the independent clauses in an ALTER statement - return transform.Node(n, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) { + n, same, err := transform.Node(n, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) { switch nn := n.(type) { case *plan.ModifyColumn: n, err := nn.WithTargetSchema(sch.Copy()) @@ -133,10 +135,12 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S return nil, transform.SameTree, err } - sch, err = validateAddColumn(initialSch, sch, n.(*plan.AddColumn), keyedColumns) + sch, err = validateAddColumn(initialSch, sch, n.(*plan.AddColumn)) if err != nil { return nil, transform.SameTree, err } + + addedColumn = true return n, transform.NewTree, nil case *plan.DropColumn: n, err := nn.WithTargetSchema(sch.Copy()) @@ -195,6 +199,21 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S } return n, transform.SameTree, nil }) + + if err != nil { + return nil, transform.SameTree, err + } + + // We can't evaluate auto-increment until the end of the analysis, since we break adding a new auto-increment unique + // column into two steps: first add the column, then create the index. If there was no index created, that's an error. + if addedColumn { + err = validateAutoIncrementAdd(ctx, sch, keyedColumns) + if err != nil { + return nil, false, err + } + } + + return n, same, nil } // updateKeyedColumns updates the keyedColumns map based on the action of the AlterIndex node @@ -242,7 +261,7 @@ func validateRenameColumn(initialSch, sch sql.Schema, rc *plan.RenameColumn) (sq return renameInSchema(sch, rc.ColumnName, rc.NewColumnName, nameable.Name()), nil } -func validateAddColumn(initialSch sql.Schema, schema sql.Schema, ac *plan.AddColumn, keyedColumns map[string]bool) (sql.Schema, error) { +func validateAddColumn(initialSch sql.Schema, schema sql.Schema, ac *plan.AddColumn) (sql.Schema, error) { table := ac.Table nameable := table.(sql.Nameable) @@ -271,11 +290,6 @@ func validateAddColumn(initialSch sql.Schema, schema sql.Schema, ac *plan.AddCol newSch = append(newSch, ac.Column().Copy()) } - err := validateAutoIncrementAdd(newSch) - if err != nil { - return nil, err - } - return newSch, nil } @@ -646,19 +660,25 @@ func validateAutoIncrementModify(schema sql.Schema, keyedColumns map[string]bool return nil } -func validateAutoIncrementAdd(schema sql.Schema) error { +func validateAutoIncrementAdd(ctx *sql.Context, schema sql.Schema, keyColumns map[string]bool) error { seen := false for _, col := range schema { if col.AutoIncrement { - if col.Default != nil { - // AUTO_INCREMENT col cannot have default - return sql.ErrInvalidAutoIncCols.New() - } - if seen { - // there can be at most one AUTO_INCREMENT col - return sql.ErrInvalidAutoIncCols.New() + { + if !col.PrimaryKey && !keyColumns[col.Name] { + // AUTO_INCREMENT col must be a key + return sql.ErrInvalidAutoIncCols.New() + } + if col.Default != nil { + // AUTO_INCREMENT col cannot have default + return sql.ErrInvalidAutoIncCols.New() + } + if seen { + // there can be at most one AUTO_INCREMENT col + return sql.ErrInvalidAutoIncCols.New() + } + seen = true } - seen = true } } return nil From dd982313a893bbbeae7d677ce3a231f59cc3d044 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 14:46:25 -0700 Subject: [PATCH 08/16] Remove script test --- enginetest/memory_engine_test.go | 78 +------------------------------- 1 file changed, 1 insertion(+), 77 deletions(-) diff --git a/enginetest/memory_engine_test.go b/enginetest/memory_engine_test.go index 5207fe0070..8b28681eb2 100644 --- a/enginetest/memory_engine_test.go +++ b/enginetest/memory_engine_test.go @@ -270,84 +270,8 @@ func TestSingleQueryPrepared(t *testing.T) { // Convenience test for debugging a single query. Unskip and set to the desired query. func TestSingleScript(t *testing.T) { - // t.Skip() + t.Skip() var scripts = []queries.ScriptTest{ - // { - // Name: "ALTER TABLE MODIFY column with KEY", - // SetUpScript: []string{ - // "CREATE table test (pk int primary key, mk int, index (mk))", - // "ALTER TABLE `test` MODIFY column mk int auto_increment", - // }, - // Assertions: []queries.ScriptTestAssertion{ - // { - // Query: "show create table test", - // Expected: []sql.Row{{"test", - // "CREATE TABLE `test` (\n" + - // " `pk` int NOT NULL,\n" + - // " `mk` int AUTO_INCREMENT,\n" + - // " PRIMARY KEY (`pk`),\n" + - // " KEY `mk` (`mk`)\n" + - // ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, - // }, - // }, - // }, - { - Name: "multi-alter ddl column statements", - SetUpScript: []string{ - "create table tbl_i (i int primary key)", - "create table tbl_ij (i int primary key, j int)", - }, - Assertions: []queries.ScriptTestAssertion{ - { - Query: "alter table tbl_i add column j int, drop column j", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_i add column j int, rename column j to k;", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_i add column j int, modify column j varchar(10)", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_ij add index (j), drop column j;", - ExpectedErr: sql.ErrKeyColumnDoesNotExist, - }, - { - Query: "alter table tbl_ij drop column j, rename column j to k;", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_ij drop column k, rename column j to k;", - ExpectedErr: sql.ErrTableColumnNotFound, - }, - { - Query: "alter table tbl_i add index(j), add column j int;", - Expected: []sql.Row{ - {types.NewOkResult(0)}, - }, - }, - { - Query: "show create table tbl_i", - Expected: []sql.Row{ - {"tbl_i", "CREATE TABLE `tbl_i` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, - }, - }, - { - Query: "alter table tbl_ij add index (j), drop column j, add column j int;", - Expected: []sql.Row{ - {types.NewOkResult(0)}, - }, - }, - { - Query: "show create table tbl_ij", - Expected: []sql.Row{ - {"tbl_ij", "CREATE TABLE `tbl_ij` (\n `i` int NOT NULL,\n `j` int,\n PRIMARY KEY (`i`),\n KEY `j` (`j`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}, - }, - }, - }, - }, } for _, test := range scripts { From 240cbc925f04e2a4a18fbaa410ebbec57798a2cf Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 14:55:30 -0700 Subject: [PATCH 09/16] Fix experimental path, add comments about my crimes --- sql/parse/parse.go | 44 +++++++++--------------------------------- sql/planbuilder/ddl.go | 33 ++++++++----------------------- 2 files changed, 17 insertions(+), 60 deletions(-) diff --git a/sql/parse/parse.go b/sql/parse/parse.go index 06c6c62f35..acbfa8047e 100644 --- a/sql/parse/parse.go +++ b/sql/parse/parse.go @@ -1305,18 +1305,14 @@ func convertDDL(ctx *sql.Context, query string, c *sqlparser.DDL) (sql.Node, err } } -// convertAlterTable converts AlterTable AST nodes -// If there are multiple alter statements, they are sorted in order of their precedence and placed inside a plan.Block -// Currently, the precedence of DDL statements is: -// 1. RENAME COLUMN -// 2. DROP COLUMN -// 3. MODIFY COLUMN -// 4. ADD COLUMN -// 5. DROP CHECK/CONSTRAINT -// 7. CREATE CHECK/CONSTRAINT -// 8. RENAME INDEX -// 9. DROP INDEX -// 10. ADD INDEX +// convertAlterTable converts AlterTable AST nodes. If there is a single clause in the statement, it is returned as +// the appropriate node type. Otherwise, a plan.Block is returned with children representing all the various clauses. +// Our validation rules for what counts as a legal set of alter clauses differs from mysql's here. MySQL seems to apply +// some form of precedence rules to the clauses in an ALTER TABLE so that e.g. DROP COLUMN always happens before other +// kinds of statements. So in MySQL, statements like `ALTER TABLE t ADD KEY (a), DROP COLUMN a` fails, whereas our +// analyzer happily produces a plan that adds an index and then drops that column. We do this in part for simplicity, +// and also because we construct more than one node per clause in some cases and really want them executed in a +// particular order in that case. func convertAlterTable(ctx *sql.Context, query string, c *sqlparser.AlterTable) (sql.Node, error) { statements := make([]sql.Node, 0, len(c.Statements)) for i := 0; i < len(c.Statements); i++ { @@ -2033,12 +2029,8 @@ func newColumnAction(ctx *sql.Context, ddl *sqlparser.DDL) (sql.Node, error) { if err != nil { return nil, err } - - // auto_increment will be added in an additional pass, so scrub it for now - column := sch.Schema[0] - // column.AutoIncrement = false - return plan.NewAddColumn(sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), tableNameToUnresolvedTable(ddl.Table), column, columnOrderToColumnOrder(ddl.ColumnOrder)), nil + return plan.NewAddColumn(sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), tableNameToUnresolvedTable(ddl.Table), sch.Schema[0], columnOrderToColumnOrder(ddl.ColumnOrder)), nil case sqlparser.DropStr: return plan.NewDropColumn(sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), tableNameToUnresolvedTable(ddl.Table), ddl.Column.String()), nil case sqlparser.RenameStr: @@ -2086,24 +2078,6 @@ func convertAlterTableClause(ctx *sql.Context, query string, ddl *sqlparser.DDL) ) ddlNodes = append(ddlNodes, createIndex) } - - // if column.Type.Autoincrement { - // sch, _, err := TableSpecToSchema(ctx, ddl.TableSpec, true) - // if err != nil { - // return nil, err - // } - // - // modifyAction := plan.NewModifyColumn( - // sql.UnresolvedDatabase(ddl.Table.Qualifier.String()), - // tableNameToUnresolvedTable(ddl.Table), - // column.Name.String(), - // sch.Schema[0], - // columnOrderToColumnOrder(ddl.ColumnOrder), - // ) - // - // modifyAction.MarkAddColumn() - // ddlNodes = append(ddlNodes, modifyAction) - // } } } diff --git a/sql/planbuilder/ddl.go b/sql/planbuilder/ddl.go index 0af0aa3001..f1bf75f2e8 100644 --- a/sql/planbuilder/ddl.go +++ b/sql/planbuilder/ddl.go @@ -2,7 +2,6 @@ package planbuilder import ( "fmt" - "sort" "strconv" "strings" @@ -33,6 +32,14 @@ func (b *Builder) resolveDb(name string) sql.Database { return database } +// buildAlterTable converts AlterTable AST nodes. If there is a single clause in the statement, it is returned as +// the appropriate node type. Otherwise, a plan.Block is returned with children representing all the various clauses. +// Our validation rules for what counts as a legal set of alter clauses differs from mysql's here. MySQL seems to apply +// some form of precedence rules to the clauses in an ALTER TABLE so that e.g. DROP COLUMN always happens before other +// kinds of statements. So in MySQL, statements like `ALTER TABLE t ADD KEY (a), DROP COLUMN a` fails, whereas our +// analyzer happily produces a plan that adds an index and then drops that column. We do this in part for simplicity, +// and also because we construct more than one node per clause in some cases and really want them executed in a +// particular order in that case. func (b *Builder) buildAlterTable(inScope *scope, query string, c *ast.AlterTable) (outScope *scope) { b.multiDDL = true defer func() { @@ -53,30 +60,6 @@ func (b *Builder) buildAlterTable(inScope *scope, query string, c *ast.AlterTabl return outScope } - // certain alter statements need to happen before others - sort.Slice(statements, func(i, j int) bool { - switch statements[i].(type) { - case *plan.RenameColumn: - switch statements[j].(type) { - case *plan.DropColumn, - *plan.AddColumn, - *plan.AlterIndex: - return true - } - case *plan.DropColumn: - switch statements[j].(type) { - case *plan.AddColumn, - *plan.AlterIndex: - return true - } - case *plan.AddColumn: - switch statements[j].(type) { - case *plan.AlterIndex: - return true - } - } - return false - }) outScope = inScope.push() outScope.node = plan.NewBlock(statements) return From 3d5341cebd929515d01178c75f6efcb979518def Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 15:03:00 -0700 Subject: [PATCH 10/16] Another alter table test --- enginetest/queries/alter_table_queries.go | 35 +++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/enginetest/queries/alter_table_queries.go b/enginetest/queries/alter_table_queries.go index 1778fe3815..b14f2cd2d0 100755 --- a/enginetest/queries/alter_table_queries.go +++ b/enginetest/queries/alter_table_queries.go @@ -388,6 +388,41 @@ var AlterTableScripts = []ScriptTest{ }, }, }, + { + Name: "Add column and make unique in separate clauses", + SetUpScript: []string{ + "create table t (c1 int primary key, c2 int, c3 int)", + "insert into t values (1, 1, 1), (2, 2, 2), (3, 3, 3)", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "alter table t add column c4 int null, add unique index uniq(c4)", + Expected: []sql.Row{ + {types.NewOkResult(0)}, + }, + }, + { + Query: "show create table t", + Expected: []sql.Row{sql.Row{"t", + "CREATE TABLE `t` (\n" + + " `c1` int NOT NULL,\n" + + " `c2` int,\n" + + " `c3` int,\n" + + " `c4` int,\n" + + " PRIMARY KEY (`c1`),\n" + + " UNIQUE KEY `uniq` (`c4`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + }, + { + Query: "select * from t", + Expected: []sql.Row{ + {1, 1, 1, nil}, + {2, 2, 2, nil}, + {3, 3, 3, nil}, + }, + }, + }, + }, } var AlterTableAddAutoIncrementScripts = []ScriptTest{ From 92d495967dd34fb26286ce9daa3b719457ac3dd9 Mon Sep 17 00:00:00 2001 From: zachmu Date: Thu, 3 Aug 2023 22:18:06 +0000 Subject: [PATCH 11/16] [ga-format-pr] Run ./format_repo.sh to fix formatting --- enginetest/memory_engine_test.go | 5 +-- enginetest/queries/alter_table_queries.go | 54 +++++++++++------------ sql/analyzer/validate_create_table.go | 22 ++++----- sql/parse/parse.go | 10 ++--- sql/plan/alter_table.go | 6 +-- sql/planbuilder/ddl.go | 4 +- 6 files changed, 50 insertions(+), 51 deletions(-) diff --git a/enginetest/memory_engine_test.go b/enginetest/memory_engine_test.go index 8b28681eb2..d0528c1a4e 100644 --- a/enginetest/memory_engine_test.go +++ b/enginetest/memory_engine_test.go @@ -271,8 +271,7 @@ func TestSingleQueryPrepared(t *testing.T) { // Convenience test for debugging a single query. Unskip and set to the desired query. func TestSingleScript(t *testing.T) { t.Skip() - var scripts = []queries.ScriptTest{ - } + var scripts = []queries.ScriptTest{} for _, test := range scripts { harness := enginetest.NewMemoryHarness("", 1, testNumPartitions, true, nil) @@ -1283,7 +1282,7 @@ func TestAddDropPks_Exp(t *testing.T) { func TestAddAutoIncrementColumn(t *testing.T) { t.Skip("in memory tables don't implement sql.RewritableTable yet") for _, script := range queries.AlterTableAddAutoIncrementScripts { - enginetest.TestScript(t, enginetest.NewDefaultMemoryHarness(), script) + enginetest.TestScript(t, enginetest.NewDefaultMemoryHarness(), script) } } diff --git a/enginetest/queries/alter_table_queries.go b/enginetest/queries/alter_table_queries.go index b14f2cd2d0..f9c7886dc5 100755 --- a/enginetest/queries/alter_table_queries.go +++ b/enginetest/queries/alter_table_queries.go @@ -383,7 +383,7 @@ var AlterTableScripts = []ScriptTest{ ExpectedErr: sql.ErrTableColumnNotFound, }, { - Query: "alter table tbl_i add index(j), add column j int;", + Query: "alter table tbl_i add index(j), add column j int;", ExpectedErr: sql.ErrKeyColumnDoesNotExist, }, }, @@ -405,13 +405,13 @@ var AlterTableScripts = []ScriptTest{ Query: "show create table t", Expected: []sql.Row{sql.Row{"t", "CREATE TABLE `t` (\n" + - " `c1` int NOT NULL,\n" + - " `c2` int,\n" + - " `c3` int,\n" + - " `c4` int,\n" + - " PRIMARY KEY (`c1`),\n" + - " UNIQUE KEY `uniq` (`c4`)\n" + - ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + " `c1` int NOT NULL,\n" + + " `c2` int,\n" + + " `c3` int,\n" + + " `c4` int,\n" + + " PRIMARY KEY (`c1`),\n" + + " UNIQUE KEY `uniq` (`c4`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, { Query: "select * from t", @@ -441,11 +441,11 @@ var AlterTableAddAutoIncrementScripts = []ScriptTest{ Query: "show create table t1", Expected: []sql.Row{{"t1", "CREATE TABLE `t1` (\n" + - " `i` int,\n" + - " `j` int,\n" + - " `pk` int NOT NULL AUTO_INCREMENT,\n" + - " PRIMARY KEY (`pk`)\n" + - ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + " `i` int,\n" + + " `j` int,\n" + + " `pk` int NOT NULL AUTO_INCREMENT,\n" + + " PRIMARY KEY (`pk`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, { Query: "select pk from t1 order by pk", @@ -463,7 +463,7 @@ var AlterTableAddAutoIncrementScripts = []ScriptTest{ }, Assertions: []ScriptTestAssertion{ { - Query: "alter table t1 add column pk int primary key", + Query: "alter table t1 add column pk int primary key", ExpectedErr: sql.ErrPrimaryKeyViolation, }, { @@ -474,11 +474,11 @@ var AlterTableAddAutoIncrementScripts = []ScriptTest{ Query: "show create table t1", Expected: []sql.Row{{"t1", "CREATE TABLE `t1` (\n" + - " `pk` int NOT NULL AUTO_INCREMENT,\n" + - " `i` int,\n" + - " `j` int,\n" + - " PRIMARY KEY (`pk`)\n" + - ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + " `pk` int NOT NULL AUTO_INCREMENT,\n" + + " `i` int,\n" + + " `j` int,\n" + + " PRIMARY KEY (`pk`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, { Query: "select pk from t1 order by pk", @@ -503,12 +503,12 @@ var AlterTableAddAutoIncrementScripts = []ScriptTest{ Query: "show create table t1", Expected: []sql.Row{{"t1", "CREATE TABLE `t1` (\n" + - " `i` bigint NOT NULL,\n" + - " `s` varchar(20),\n" + - " `j` int AUTO_INCREMENT,\n" + - " PRIMARY KEY (`i`),\n" + - " UNIQUE KEY `j` (`j`)\n" + - ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + " `i` bigint NOT NULL,\n" + + " `s` varchar(20),\n" + + " `j` int AUTO_INCREMENT,\n" + + " PRIMARY KEY (`i`),\n" + + " UNIQUE KEY `j` (`j`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, { Query: "select * from t1 order by i", @@ -528,9 +528,9 @@ var AlterTableAddAutoIncrementScripts = []ScriptTest{ }, Assertions: []ScriptTestAssertion{ { - Query: "alter table t1 add column j int auto_increment", + Query: "alter table t1 add column j int auto_increment", ExpectedErr: sql.ErrInvalidAutoIncCols, }, }, }, -} \ No newline at end of file +} diff --git a/sql/analyzer/validate_create_table.go b/sql/analyzer/validate_create_table.go index ae1defd657..aa8ca501f1 100644 --- a/sql/analyzer/validate_create_table.go +++ b/sql/analyzer/validate_create_table.go @@ -101,9 +101,9 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S sch = sch.Copy() // Make a copy of the original schema to deal with any references to the original table. initialSch := sch - + addedColumn := false - + // Need a TransformUp here because multiple of these statement types can be nested under a Block node. // It doesn't look it, but this is actually an iterative loop over all the independent clauses in an ALTER statement n, same, err := transform.Node(n, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) { @@ -113,7 +113,7 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S if err != nil { return nil, transform.SameTree, err } - + sch, err = validateModifyColumn(ctx, initialSch, sch, n.(*plan.ModifyColumn), keyedColumns) if err != nil { return nil, transform.SameTree, err @@ -134,12 +134,12 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S if err != nil { return nil, transform.SameTree, err } - + sch, err = validateAddColumn(initialSch, sch, n.(*plan.AddColumn)) if err != nil { return nil, transform.SameTree, err } - + addedColumn = true return n, transform.NewTree, nil case *plan.DropColumn: @@ -152,7 +152,7 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S return nil, transform.SameTree, err } delete(keyedColumns, nn.Column) - + return n, transform.NewTree, nil case *plan.AlterIndex: n, err := nn.WithTargetSchema(sch.Copy()) @@ -163,7 +163,7 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S if err != nil { return nil, transform.SameTree, err } - + keyedColumns = updateKeyedColumns(keyedColumns, nn) return n, transform.NewTree, nil case *plan.AlterPK: @@ -203,7 +203,7 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S if err != nil { return nil, transform.SameTree, err } - + // We can't evaluate auto-increment until the end of the analysis, since we break adding a new auto-increment unique // column into two steps: first add the column, then create the index. If there was no index created, that's an error. if addedColumn { @@ -212,7 +212,7 @@ func resolveAlterColumn(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.S return nil, false, err } } - + return n, same, nil } @@ -228,7 +228,7 @@ func updateKeyedColumns(keyedColumns map[string]bool, n *plan.AlterIndex) map[st delete(keyedColumns, col.Name) } } - + return keyedColumns } @@ -305,7 +305,7 @@ func validateModifyColumn(ctx *sql.Context, initialSch sql.Schema, schema sql.Sc } } else { if !schema.Contains(mc.Column(), nameable.Name()) || - !initialSch.Contains(mc.Column(), nameable.Name()) { + !initialSch.Contains(mc.Column(), nameable.Name()) { return nil, sql.ErrTableColumnNotFound.New(nameable.Name(), mc.Column()) } } diff --git a/sql/parse/parse.go b/sql/parse/parse.go index acbfa8047e..31c1ffcba1 100644 --- a/sql/parse/parse.go +++ b/sql/parse/parse.go @@ -1305,12 +1305,12 @@ func convertDDL(ctx *sql.Context, query string, c *sqlparser.DDL) (sql.Node, err } } -// convertAlterTable converts AlterTable AST nodes. If there is a single clause in the statement, it is returned as +// convertAlterTable converts AlterTable AST nodes. If there is a single clause in the statement, it is returned as // the appropriate node type. Otherwise, a plan.Block is returned with children representing all the various clauses. // Our validation rules for what counts as a legal set of alter clauses differs from mysql's here. MySQL seems to apply // some form of precedence rules to the clauses in an ALTER TABLE so that e.g. DROP COLUMN always happens before other // kinds of statements. So in MySQL, statements like `ALTER TABLE t ADD KEY (a), DROP COLUMN a` fails, whereas our -// analyzer happily produces a plan that adds an index and then drops that column. We do this in part for simplicity, +// analyzer happily produces a plan that adds an index and then drops that column. We do this in part for simplicity, // and also because we construct more than one node per clause in some cases and really want them executed in a // particular order in that case. func convertAlterTable(ctx *sql.Context, query string, c *sqlparser.AlterTable) (sql.Node, error) { @@ -1326,7 +1326,7 @@ func convertAlterTable(ctx *sql.Context, query string, c *sqlparser.AlterTable) if len(statements) == 1 { return statements[0], nil } - + return plan.NewBlock(statements), nil } @@ -2018,8 +2018,8 @@ func convertRenameTable(ctx *sql.Context, ddl *sqlparser.DDL, alterTbl bool) (sq return plan.NewRenameTable(sql.UnresolvedDatabase(""), fromTables, toTables, alterTbl), nil } func isUniqueColumn(column *sqlparser.ColumnDefinition) bool { - return column.Type.KeyOpt == colKeyUnique || - column.Type.KeyOpt == colKeyUniqueKey + return column.Type.KeyOpt == colKeyUnique || + column.Type.KeyOpt == colKeyUniqueKey } func newColumnAction(ctx *sql.Context, ddl *sqlparser.DDL) (sql.Node, error) { diff --git a/sql/plan/alter_table.go b/sql/plan/alter_table.go index ad7c5ffb12..8e8ccc887e 100644 --- a/sql/plan/alter_table.go +++ b/sql/plan/alter_table.go @@ -692,12 +692,12 @@ func (*RenameColumn) CollationCoercibility(ctx *sql.Context) (collation sql.Coll type ModifyColumn struct { ddlNode - Table sql.Node + Table sql.Node columnName string partOfAddColumn bool column *sql.Column - order *sql.ColumnOrder - targetSchema sql.Schema + order *sql.ColumnOrder + targetSchema sql.Schema } var _ sql.Node = (*ModifyColumn)(nil) diff --git a/sql/planbuilder/ddl.go b/sql/planbuilder/ddl.go index f1bf75f2e8..02a6e9f853 100644 --- a/sql/planbuilder/ddl.go +++ b/sql/planbuilder/ddl.go @@ -32,12 +32,12 @@ func (b *Builder) resolveDb(name string) sql.Database { return database } -// buildAlterTable converts AlterTable AST nodes. If there is a single clause in the statement, it is returned as +// buildAlterTable converts AlterTable AST nodes. If there is a single clause in the statement, it is returned as // the appropriate node type. Otherwise, a plan.Block is returned with children representing all the various clauses. // Our validation rules for what counts as a legal set of alter clauses differs from mysql's here. MySQL seems to apply // some form of precedence rules to the clauses in an ALTER TABLE so that e.g. DROP COLUMN always happens before other // kinds of statements. So in MySQL, statements like `ALTER TABLE t ADD KEY (a), DROP COLUMN a` fails, whereas our -// analyzer happily produces a plan that adds an index and then drops that column. We do this in part for simplicity, +// analyzer happily produces a plan that adds an index and then drops that column. We do this in part for simplicity, // and also because we construct more than one node per clause in some cases and really want them executed in a // particular order in that case. func (b *Builder) buildAlterTable(inScope *scope, query string, c *ast.AlterTable) (outScope *scope) { From 0d7012378339d83e4cf99ed83f50d06aa2cf12a6 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 15:31:57 -0700 Subject: [PATCH 12/16] Fixed parse test --- sql/parse/parse_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/sql/parse/parse_test.go b/sql/parse/parse_test.go index da0abbb93c..151e5b64c4 100644 --- a/sql/parse/parse_test.go +++ b/sql/parse/parse_test.go @@ -2879,15 +2879,15 @@ CREATE TABLE t2 { input: `alter table t add index (i), drop index i, add check (i = 0), drop check chk, drop constraint c, add column i int, modify column i text, drop column i, rename column i to j`, plan: plan.NewBlock([]sql.Node{ - plan.NewRenameColumn(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "i", "j"), - plan.NewDropColumn(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "i"), - plan.NewModifyColumn(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "i", &sql.Column{Name: "i", Type: types.CreateText(sql.Collation_Unspecified), Nullable: true, Source: "t"}, nil), - plan.NewAddColumn(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), &sql.Column{Name: "i", Type: types.Int32, Nullable: true, Source: "t"}, nil), - plan.NewDropConstraint(plan.NewUnresolvedTable("t", ""), "c"), - plan.NewAlterDropCheck(plan.NewUnresolvedTable("t", ""), "chk"), - plan.NewAlterAddCheck(plan.NewUnresolvedTable("t", ""), &sql.CheckConstraint{Name: "", Expr: expression.NewEquals(expression.NewUnresolvedColumn("i"), expression.NewLiteral(int8(0), types.Int8)), Enforced: true}), - plan.NewAlterDropIndex(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "i"), plan.NewAlterCreateIndex(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "", sql.IndexUsing_BTree, sql.IndexConstraint_None, []sql.IndexColumn{{Name: "i", Length: 0}}, ""), + plan.NewAlterDropIndex(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "i"), + plan.NewAlterAddCheck(plan.NewUnresolvedTable("t", ""), &sql.CheckConstraint{Name: "", Expr: expression.NewEquals(expression.NewUnresolvedColumn("i"), expression.NewLiteral(int8(0), types.Int8)), Enforced: true}), + plan.NewAlterDropCheck(plan.NewUnresolvedTable("t", ""), "chk"), + plan.NewDropConstraint(plan.NewUnresolvedTable("t", ""), "c"), + plan.NewAddColumn(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), &sql.Column{Name: "i", Type: types.Int32, Nullable: true, Source: "t"}, nil), + plan.NewModifyColumn(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "i", &sql.Column{Name: "i", Type: types.CreateText(sql.Collation_Unspecified), Nullable: true, Source: "t"}, nil), + plan.NewDropColumn(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "i"), + plan.NewRenameColumn(sql.UnresolvedDatabase(""), plan.NewUnresolvedTable("t", ""), "i", "j"), }), }, { From 95963fe782ffb167e565948aa49456fcadd6132d Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 3 Aug 2023 17:11:49 -0700 Subject: [PATCH 13/16] Tests for create table --- enginetest/enginetests.go | 4 ++ enginetest/queries/create_table_queries.go | 83 ++++++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/enginetest/enginetests.go b/enginetest/enginetests.go index 3bdfa13fc7..635a6bed49 100644 --- a/enginetest/enginetests.go +++ b/enginetest/enginetests.go @@ -2060,6 +2060,10 @@ func TestCreateTable(t *testing.T, harness Harness) { TestScriptPrepared(t, harness, script) } + for _, script := range queries.CreateTableAutoIncrementTests { + TestScript(t, harness, script) + } + harness.Setup(setup.MydbData, setup.MytableData) e := mustNewEngine(t, harness) defer e.Close() diff --git a/enginetest/queries/create_table_queries.go b/enginetest/queries/create_table_queries.go index b7e206554f..06211b5aa5 100644 --- a/enginetest/queries/create_table_queries.go +++ b/enginetest/queries/create_table_queries.go @@ -248,6 +248,89 @@ var CreateTableScriptTests = []ScriptTest{ }, } +var CreateTableAutoIncrementTests = []ScriptTest{ + { + Name: "create table with non primary auto_increment column", + SetUpScript: []string{}, + Assertions: []ScriptTestAssertion{ + { + Query: "create table t1 (a int auto_increment unique, b int, primary key(b))", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "insert into t1 (b) values (1), (2)", + Expected: []sql.Row{ + { + types.OkResult{ + RowsAffected: 2, + InsertID: 1, + }, + }, + }, + }, + { + Query: "show create table t1", + Expected: []sql.Row{{"t1", + "CREATE TABLE `t1` (\n" + + " `a` int AUTO_INCREMENT,\n" + + " `b` int NOT NULL,\n" + + " PRIMARY KEY (`b`),\n" + + " UNIQUE KEY `a` (`a`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + }, + { + Query: "select * from t1 order by b", + Expected: []sql.Row{{1, 1}, {2, 2}}, + }, + }, + }, + { + Name: "create table with non primary auto_increment column, separate unique key", + SetUpScript: []string{}, + Assertions: []ScriptTestAssertion{ + { + Query: "create table t1 (a int auto_increment, b int, primary key(b), unique key(a))", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "insert into t1 (b) values (1), (2)", + Expected: []sql.Row{ + { + types.OkResult{ + RowsAffected: 2, + InsertID: 1, + }, + }, + }, + }, + { + Query: "show create table t1", + Expected: []sql.Row{{"t1", + "CREATE TABLE `t1` (\n" + + " `a` int AUTO_INCREMENT,\n" + + " `b` int NOT NULL,\n" + + " PRIMARY KEY (`b`),\n" + + " UNIQUE KEY `a` (`a`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + }, + { + Query: "select * from t1 order by b", + Expected: []sql.Row{{1, 1}, {2, 2}}, + }, + }, + }, + { + Name: "create table with non primary auto_increment column, missing unique key", + SetUpScript: []string{}, + Assertions: []ScriptTestAssertion{ + { + Query: "create table t1 (a int auto_increment, b int, primary key(b))", + ExpectedErr: sql.ErrInvalidAutoIncCols, + }, + }, + }, +} + var BrokenCreateTableQueries = []WriteQueryTest{ { WriteQuery: `create table t1 (b blob, primary key(b(1)))`, From e22a44ffdef095e282b5f71791dd295e439c3762 Mon Sep 17 00:00:00 2001 From: zachmu Date: Fri, 4 Aug 2023 00:14:00 +0000 Subject: [PATCH 14/16] [ga-format-pr] Run ./format_repo.sh to fix formatting --- enginetest/enginetests.go | 2 +- enginetest/queries/create_table_queries.go | 44 +++++++++++----------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/enginetest/enginetests.go b/enginetest/enginetests.go index 635a6bed49..ab10c975bb 100644 --- a/enginetest/enginetests.go +++ b/enginetest/enginetests.go @@ -2063,7 +2063,7 @@ func TestCreateTable(t *testing.T, harness Harness) { for _, script := range queries.CreateTableAutoIncrementTests { TestScript(t, harness, script) } - + harness.Setup(setup.MydbData, setup.MytableData) e := mustNewEngine(t, harness) defer e.Close() diff --git a/enginetest/queries/create_table_queries.go b/enginetest/queries/create_table_queries.go index 06211b5aa5..75a4862076 100644 --- a/enginetest/queries/create_table_queries.go +++ b/enginetest/queries/create_table_queries.go @@ -250,16 +250,16 @@ var CreateTableScriptTests = []ScriptTest{ var CreateTableAutoIncrementTests = []ScriptTest{ { - Name: "create table with non primary auto_increment column", + Name: "create table with non primary auto_increment column", SetUpScript: []string{}, Assertions: []ScriptTestAssertion{ { - Query: "create table t1 (a int auto_increment unique, b int, primary key(b))", + Query: "create table t1 (a int auto_increment unique, b int, primary key(b))", Expected: []sql.Row{{types.NewOkResult(0)}}, }, { - Query: "insert into t1 (b) values (1), (2)", - Expected: []sql.Row{ + Query: "insert into t1 (b) values (1), (2)", + Expected: []sql.Row{ { types.OkResult{ RowsAffected: 2, @@ -272,29 +272,29 @@ var CreateTableAutoIncrementTests = []ScriptTest{ Query: "show create table t1", Expected: []sql.Row{{"t1", "CREATE TABLE `t1` (\n" + - " `a` int AUTO_INCREMENT,\n" + - " `b` int NOT NULL,\n" + - " PRIMARY KEY (`b`),\n" + - " UNIQUE KEY `a` (`a`)\n" + - ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + " `a` int AUTO_INCREMENT,\n" + + " `b` int NOT NULL,\n" + + " PRIMARY KEY (`b`),\n" + + " UNIQUE KEY `a` (`a`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, { - Query: "select * from t1 order by b", - Expected: []sql.Row{{1, 1}, {2, 2}}, + Query: "select * from t1 order by b", + Expected: []sql.Row{{1, 1}, {2, 2}}, }, }, }, { - Name: "create table with non primary auto_increment column, separate unique key", + Name: "create table with non primary auto_increment column, separate unique key", SetUpScript: []string{}, Assertions: []ScriptTestAssertion{ { - Query: "create table t1 (a int auto_increment, b int, primary key(b), unique key(a))", + Query: "create table t1 (a int auto_increment, b int, primary key(b), unique key(a))", Expected: []sql.Row{{types.NewOkResult(0)}}, }, { - Query: "insert into t1 (b) values (1), (2)", - Expected: []sql.Row{ + Query: "insert into t1 (b) values (1), (2)", + Expected: []sql.Row{ { types.OkResult{ RowsAffected: 2, @@ -307,15 +307,15 @@ var CreateTableAutoIncrementTests = []ScriptTest{ Query: "show create table t1", Expected: []sql.Row{{"t1", "CREATE TABLE `t1` (\n" + - " `a` int AUTO_INCREMENT,\n" + - " `b` int NOT NULL,\n" + - " PRIMARY KEY (`b`),\n" + - " UNIQUE KEY `a` (`a`)\n" + - ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + " `a` int AUTO_INCREMENT,\n" + + " `b` int NOT NULL,\n" + + " PRIMARY KEY (`b`),\n" + + " UNIQUE KEY `a` (`a`)\n" + + ") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, }, { - Query: "select * from t1 order by b", - Expected: []sql.Row{{1, 1}, {2, 2}}, + Query: "select * from t1 order by b", + Expected: []sql.Row{{1, 1}, {2, 2}}, }, }, }, From 710fa7e476172b57cfea800e8b45796e49cbf078 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Fri, 4 Aug 2023 12:00:24 -0700 Subject: [PATCH 15/16] Removed dead code --- sql/analyzer/validate_create_table.go | 10 ++-------- sql/plan/alter_table.go | 10 ---------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/sql/analyzer/validate_create_table.go b/sql/analyzer/validate_create_table.go index aa8ca501f1..1a06bee13e 100644 --- a/sql/analyzer/validate_create_table.go +++ b/sql/analyzer/validate_create_table.go @@ -299,15 +299,9 @@ func validateModifyColumn(ctx *sql.Context, initialSch sql.Schema, schema sql.Sc // Look for the old column and throw an error if it's not there. The column cannot have been renamed in the same // statement. This matches the MySQL behavior. - if mc.IsPartOfAddColumn() { - if !schema.Contains(mc.Column(), nameable.Name()) { - return nil, sql.ErrTableColumnNotFound.New(nameable.Name(), mc.Column()) - } - } else { - if !schema.Contains(mc.Column(), nameable.Name()) || + if !schema.Contains(mc.Column(), nameable.Name()) || !initialSch.Contains(mc.Column(), nameable.Name()) { - return nil, sql.ErrTableColumnNotFound.New(nameable.Name(), mc.Column()) - } + return nil, sql.ErrTableColumnNotFound.New(nameable.Name(), mc.Column()) } newSch := replaceInSchema(schema, mc.NewColumn(), nameable.Name()) diff --git a/sql/plan/alter_table.go b/sql/plan/alter_table.go index 8e8ccc887e..87949d12e7 100644 --- a/sql/plan/alter_table.go +++ b/sql/plan/alter_table.go @@ -694,7 +694,6 @@ type ModifyColumn struct { ddlNode Table sql.Node columnName string - partOfAddColumn bool column *sql.Column order *sql.ColumnOrder targetSchema sql.Schema @@ -728,15 +727,6 @@ func NewModifyColumn(database sql.Database, table *UnresolvedTable, columnName s } } -func (m *ModifyColumn) MarkAddColumn() { - m.partOfAddColumn = true -} - -// IsPartOfAddColumn returns if this node is part of an AddColumn operation, as oposed to a stand-alone modify operation. -func (m *ModifyColumn) IsPartOfAddColumn() bool { - return m.partOfAddColumn -} - func (m *ModifyColumn) WithDatabase(db sql.Database) (sql.Node, error) { nm := *m nm.Db = db From a9216ed35ff08098cbe3e021f4e1769a96164c9e Mon Sep 17 00:00:00 2001 From: zachmu Date: Fri, 4 Aug 2023 19:06:38 +0000 Subject: [PATCH 16/16] [ga-format-pr] Run ./format_repo.sh to fix formatting --- sql/analyzer/validate_create_table.go | 2 +- sql/plan/alter_table.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sql/analyzer/validate_create_table.go b/sql/analyzer/validate_create_table.go index 1a06bee13e..eae43545c2 100644 --- a/sql/analyzer/validate_create_table.go +++ b/sql/analyzer/validate_create_table.go @@ -300,7 +300,7 @@ func validateModifyColumn(ctx *sql.Context, initialSch sql.Schema, schema sql.Sc // Look for the old column and throw an error if it's not there. The column cannot have been renamed in the same // statement. This matches the MySQL behavior. if !schema.Contains(mc.Column(), nameable.Name()) || - !initialSch.Contains(mc.Column(), nameable.Name()) { + !initialSch.Contains(mc.Column(), nameable.Name()) { return nil, sql.ErrTableColumnNotFound.New(nameable.Name(), mc.Column()) } diff --git a/sql/plan/alter_table.go b/sql/plan/alter_table.go index 87949d12e7..4df9b02725 100644 --- a/sql/plan/alter_table.go +++ b/sql/plan/alter_table.go @@ -692,11 +692,11 @@ func (*RenameColumn) CollationCoercibility(ctx *sql.Context) (collation sql.Coll type ModifyColumn struct { ddlNode - Table sql.Node - columnName string - column *sql.Column - order *sql.ColumnOrder - targetSchema sql.Schema + Table sql.Node + columnName string + column *sql.Column + order *sql.ColumnOrder + targetSchema sql.Schema } var _ sql.Node = (*ModifyColumn)(nil)