Skip to content

expanding ALTER support #5768

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.

Sign up for GitHub

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

Closed
jmhodges wants to merge 1 commit into from
Closed

expanding ALTER support #5768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 87 additions & 6 deletions go/vt/sqlparser/ast.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,16 @@ type (

// AutoIncSpec is set for AddAutoIncStr.
AutoIncSpec *AutoIncSpec

// Ignore is set to the IGNORE option on the ALTER or RENAME statement. It
// will end with a trailing space.
Ignore string

// AlterSpecs includes the specifics of an ALTER statement that involves
// adding, dropping, or renaming columns or changes a table's
// configuration. ALTERs that rename a table are still handled as a RENAME
// Action with From and ToTables set.
AlterSpecs []*AlterSpec
}

// ParenSelect is a parenthesized SELECT statement.
Expand Down Expand Up @@ -212,6 +222,30 @@ type (
// It should be used only as an indicator. It does not contain
// the full AST for the statement.
OtherAdmin struct{}

// AlterSpec represents a specific command in an ALTER statement. An ALTER
// statement can have some kinds of commands multiple times or with other
// commands. Those are the kinds of commands that an AlterSpec represents.
AlterSpec struct {
// The kind of operation the ALTER command is.
Action AlterAction
// ColumnsAdded is set when the Action is AlterAddColumn.
ColumnsAdded []*ColumnDefinition
// ColumnDropped is set when the Action is AlterDropColumn.
ColumnDropped ColIdent
// IndexAdded is set when the Action is AlterAddIndexOrKey.
IndexAdded *IndexDefinition
// IndexDropped is set when the Action is AlterDropIndexOrKey. It's the name of the index or key dropped by this ALTER command.
IndexDropped *ColIdent
// PartitionAdded is set when the Action is AlterAddPartition.
PartitionAdded *PartitionDefinition
// PartitionsDropped is the name of the partition is dropped. It's set when
// AlterDropPartition is set.
PartitionsDropped Partitions

// ForeignKeyDropped is set when the Action is AlterDropForeignKey.
ForeignKeyDropped *ColIdent
}
)

func (*Union) iStatement() {}
Expand Down Expand Up @@ -323,11 +357,13 @@ type IndexDefinition struct {

// IndexInfo describes the name and type of an index in a CREATE TABLE statement
type IndexInfo struct {
Type string
Name ColIdent
Primary bool
Spatial bool
Unique bool
Type string
Name ColIdent
Primary bool
Spatial bool
Foreign bool
Fulltext bool
Unique bool
}

// VindexSpec defines a vindex for a CREATE VINDEX or DROP VINDEX statement
Expand Down Expand Up @@ -907,7 +943,23 @@ func (node *DDL) Format(buf *TrackedBuffer) {
buf.Myprintf(", %v to %v", node.FromTables[i], node.ToTables[i])
}
case AlterStr:
if node.PartitionSpec != nil {
if len(node.AlterSpecs) != 0 {
buf.Myprintf("%s %stable %v ", node.Action, node.Ignore, node.Table)
if len(node.AlterSpecs) == 1 {
buf.Myprintf("%v", node.AlterSpecs[0])
} else {
buf.Myprintf("(")
for i, spec := range node.AlterSpecs {
if i != 0 {
buf.Myprintf(", %v", spec)
} else {
buf.Myprintf("%v", spec)
}
}
buf.Myprintf(")")
}

} else if node.PartitionSpec != nil {
buf.Myprintf("%s table %v %v", node.Action, node.Table, node.PartitionSpec)
} else {
buf.Myprintf("%s table %v", node.Action, node.Table)
Expand Down Expand Up @@ -996,6 +1048,35 @@ func (ts *TableSpec) Format(buf *TrackedBuffer) {
buf.Myprintf("\n)%s", strings.Replace(ts.Options, ", ", ",\n ", -1))
}

// Format formats the node as a SQL command.
func (spec *AlterSpec) Format(buf *TrackedBuffer) {
switch spec.Action {
case AlterAddColumn:
buf.Myprintf("add column ")
for i, col := range spec.ColumnsAdded {
if i == 0 {
buf.Myprintf("%v", col)
} else {
buf.Myprintf(", %v", col)
}
}
case AlterDropColumn:
buf.Myprintf("drop column %v", spec.ColumnDropped)
case AlterAddIndexOrKey:
buf.Myprintf("add %v", spec.IndexAdded)
case AlterDropIndexOrKey:
buf.Myprintf("drop index %v", *spec.IndexDropped)
case AlterAddPartition:
buf.Myprintf("add partition %v", spec.PartitionAdded)
case AlterDropPartition:
buf.Myprintf("drop%v", spec.PartitionsDropped)
Copy link
Copy Markdown
Collaborator

@systay systay Jan 28, 2020

Choose a reason for hiding this comment

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

Shouldn't this be drop partition %v? Maybe we should add tests cases for the new constructs in parse_test.go to make sure that parsing and pretty printing both agree?

Copy link
Copy Markdown
Contributor Author

@jmhodges jmhodges Jan 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Annoyingly, it's not and I didn't want to change the way the Partitions struct worked. We already have test cases for drop partition:

	}, {
		input:  "alter table a drop partition p2712",
		output: "alter table a drop partition (p2712)",
	}, {

Copy link
Copy Markdown
Contributor

@sougou sougou Feb 3, 2020

Choose a reason for hiding this comment

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

This generates a syntax error in mysql:

mysql> alter table a drop partition (a,b);
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '(a,b)' at line 1
mysql> alter table a drop partition a;
ERROR 1505 (HY000): Partition management on a not partitioned table is not possible
mysql> alter table a drop partition a, b;
ERROR 1505 (HY000): Partition management on a not partitioned table is not possible

If you want to reuse an existing type, you could use Exprs which prints a comma-separated list. Otherwise, I'd recommend creating a new ColIdents type so you can define a separate formatting for that member.

Also, I assume partition names are case-insensitive. Otherwise, we should use TabletIdents.

case AlterDropForeignKey:
buf.Myprintf("drop foreign key %v", spec.ForeignKeyDropped)
case AlterDropPrimaryKey:
buf.Myprintf("drop primary key")
}
}

// Format formats the node.
func (col *ColumnDefinition) Format(buf *TrackedBuffer) {
buf.Myprintf("%v %v", col.Name, &col.Type)
Expand Down
32 changes: 32 additions & 0 deletions go/vt/sqlparser/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,35 @@ const (
TxReadOnly = "read only"
TxReadWrite = "read write"
)

// AlterAction is used to determine what kind of ALTER command is being
// represented by an AlterSpec.
type AlterAction int

const (
// AlterAddColumn is set when the ALTER command adds a column. Multiple
// columns can be added within one `ADD COLUMN` sub-command.
AlterAddColumn AlterAction = iota

// AlterDropColumn is set when the ALTER command drops a column or index.
AlterDropColumn

// AlterAddIndexOrKey is set when the ALTER command adds an index or key.
AlterAddIndexOrKey

// AlterDropIndexOrKey is set when the ALTER command drops an index or key.
AlterDropIndexOrKey

// AlterAddPartition is set when the ALTER command adds a partition.
AlterAddPartition

// AlterDropPartition is set when the ALTER command drops a partition.
AlterDropPartition

// AlterDropForeignKey is set when the ALTER command drops a foreign key constraint.
AlterDropForeignKey

// AlterDropPrimaryKey is set when the ALTER command drops the primary key
// from the table.
AlterDropPrimaryKey
)
102 changes: 47 additions & 55 deletions go/vt/sqlparser/parse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"strings"
"sync"
"testing"

"github.com/stretchr/testify/require"
)

var (
Expand Down Expand Up @@ -824,20 +826,20 @@ var (
}, {
input: "set sql_safe_updates = 1",
}, {
input: "alter ignore table a add foo",
output: "alter table a",
input: "alter ignore table a add foo bigint",
output: "alter ignore table a add column foo bigint",
}, {
input: "alter table a add foo",
output: "alter table a",
input: "alter table a add foo bigint",
output: "alter table a add column foo bigint",
}, {
input: "alter table a add spatial key foo (column1)",
output: "alter table a",
output: "alter table a add spatial key foo (column1)",
}, {
input: "alter table a add unique key foo (column1)",
output: "alter table a",
output: "alter table a add unique key foo (column1)",
}, {
input: "alter table `By` add foo",
output: "alter table `By`",
input: "alter table `By` add foo bigint",
output: "alter table `By` add column foo bigint",
}, {
input: "alter table a alter foo",
output: "alter table a",
Expand All @@ -849,7 +851,7 @@ var (
output: "alter table a",
}, {
input: "alter table a drop foo",
output: "alter table a",
output: "alter table a drop column foo",
}, {
input: "alter table a disable foo",
output: "alter table a",
Expand Down Expand Up @@ -906,61 +908,54 @@ var (
output: "alter table a",
}, {
input: "alter table a add column id int",
output: "alter table a",
output: "alter table a add column id int",
}, {
input: "alter table a add index idx (id)",
output: "alter table a",
output: "alter table a add index idx (id)",
}, {
input: "alter table a add fulltext index idx (id)",
output: "alter table a",
output: "alter table a add fulltext index idx (id)",
}, {
input: "alter table a add spatial index idx (id)",
output: "alter table a",
output: "alter table a add spatial index idx (id)",
}, {
input: "alter table a add foreign key",
output: "alter table a",
}, {
input: "alter table a add primary key",
input: "alter table a add primary key idx (id)",
output: "alter table a",
}, {
input: "alter table a add constraint",
output: "alter table a",
}, {
input: "alter table a add id",
output: "alter table a",
input: "alter table a add id int",
output: "alter table a add column id int",
}, {
input: "alter table a drop column id int",
output: "alter table a",
input: "alter table a drop column id",
output: "alter table a drop column id",
}, {
input: "alter table a drop partition p2712",
output: "alter table a",
output: "alter table a drop partition (p2712)",
}, {
input: "alter table a drop index idx (id)",
output: "alter table a",
input: "alter table a drop index idx",
output: "alter table a drop index idx",
}, {
input: "alter table a drop fulltext index idx (id)",
// ADD CHECK is parsed but ignored by mysql 5.7 and later, so we're
// skipping it.
input: "alter table a add check ch_1 (1 = 1)",
output: "alter table a",
}, {
input: "alter table a drop spatial index idx (id)",
output: "alter table a",
}, {
input: "alter table a add check ch_1",
output: "alter table a",
}, {
input: "alter table a drop check ch_1",
output: "alter table a",
}, {
input: "alter table a drop foreign key",
output: "alter table a",
input: "alter table a drop foreign key fk_idx",
output: "alter table a drop foreign key fk_idx",
}, {
input: "alter table a drop primary key",
output: "alter table a",
output: "alter table a drop primary key",
}, {
input: "alter table a drop constraint",
output: "alter table a",
}, {
input: "alter table a drop id",
output: "alter table a",
output: "alter table a drop column id",
}, {
input: "alter database d default character set = charset",
output: "alter database d",
Expand Down Expand Up @@ -1505,26 +1500,23 @@ var (
)

func TestValid(t *testing.T) {
for _, tcase := range validSQL {
if tcase.output == "" {
tcase.output = tcase.input
}
tree, err := Parse(tcase.input)
if err != nil {
t.Errorf("Parse(%q) err: %v, want nil", tcase.input, err)
continue
}
out := String(tree)
if out != tcase.output {
t.Errorf("Parse(%q) = %q, want: %q", tcase.input, out, tcase.output)
}
// This test just exercises the tree walking functionality.
// There's no way automated way to verify that a node calls
// all its children. But we can examine code coverage and
// ensure that all walkSubtree functions were called.
Walk(func(node SQLNode) (bool, error) {
return true, nil
}, tree)
for i, tcase := range validSQL {
t.Run(fmt.Sprintf("test-%d", i), func(t *testing.T) {
if tcase.output == "" {
tcase.output = tcase.input
}
tree, err := Parse(tcase.input)
require.NoError(t, err, "Parse(%q) err: %v, want nil", tcase.input, err)
out := String(tree)
require.Equal(t, out, tcase.output, "String(Parse(%q)) = %q, want: %q", tcase.input, out, tcase.output)
// This test just exercises the tree walking functionality.
// There's no way automated way to verify that a node calls
// all its children. But we can examine code coverage and
// ensure that all walkSubtree functions were called.
Walk(func(node SQLNode) (bool, error) {
return true, nil
}, tree)
})
}
}

Expand Down
Loading