expanding ALTER support

[jmhodges]

This is a significant, but not complete expansion of the ALTER support in
sqlparser.

It adds support for these ALTER commands:

• ADD COLUMN
• DROP COLUMN
• ADD INDEX
• DROP INDEX
• DROP FOREIGN KEY
• DROP PRIMARY KEY
• ADD PARTITION
• DROP PARTITION
• ADD CHECK (as a no-op; it's parsed but never executed by mysql servers)

The main addition with this API is an additional field on DDL that is a slice
of newly created AlterSpecs. An AlterSpec represents one of the commands
that can occur in the same ALTER statement with other commands. This
differentiates them from the ALTER statement parse states already in sql.y
which are concerned with ALTER commands that must be the sole commands in the
statement.

The ADD PARTITION and DROP PARTITION states are the exception and are new
singleton commands that sql.y now supports.

This patch also includes some incidental updates to go.mod because of tooling
issues (described in #5755) and are duplicated in #5766

Updates #5705

Signed-off-by: Jeff Hodges jeff@somethingsimilar.com

[jmhodges]

I'm very interested in getting design feedback here! The new field in DDL felt a bit odd, but required for backwards compatibility. And perhaps the ADD/DROP PARTITION stuff doesn't belong as an AlterSpec.

Also, the recent rewrite of ast.go and creation of rewriter.go happened while this patch was being developed, and I'd like to be sure I'm doing things correctly in that new context.

The next important ALTER commands to be handled are probably CHANGE COLUMN, RENAME INDEX/KEY, and the character set commands. Those shouldn't be too invasive. I chose not to include them because I didn't want slam everything into one big patch but also wanted enough diversity in needs that the design could be proven out in this patch.

[jmhodges]

looks like an unrelated flaky test? Lmk if it's real!

[systay]

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?

[jmhodges]

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)",
	}, {

[sougou]

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.

[systay]

Yeah, that looks like a flaky test. I restarted it. 🙏

This looks really good in general. Thank you for taking the time to do this.

The one concern I have, which is not specific to this PR but more in general of your additions to the parser, is that Vitess might start accepting, but ignoring, whole or parts of queries. It would be nice to have a way of marking AST objects in some way as unsupported, and fail early in the pipeline with a clear message. Not sure how that would work yet. If you have ideas, do share.

[cmoog]

It would be nice to have a way of marking AST objects in some way as unsupported and fail early in the pipeline with a clear message. Not sure how that would work yet. If you have ideas, do share.

To throw a potential solution out there... we could add struct tags to unsupported fields like so

DDL struct {
        ...
	PartitionSpec *PartitionSpec
	VindexCols    []ColIdent
	Ignore        string       `vitess:"unsupported"`
	AlterSpecs    []*AlterSpec `vitess:"unsupported"`
}

Then we'd check if `unsupported" feilds were set after parsing – which would trigger an error. Although this would certainly involve some reflection at runtime that would come at a cost.

[jmhodges]

The desire for those unsupported checks would perhaps make more sense in a higher level package? The sqlparser could be just a sql language parser (since that's complicated enough), and handling semantics seem like they'd be better done above it.

[systay]

Sorry for letting this linger, @jmhodges. Seems like a conflict has snuck in. When it's in this file, I always just remove the file and recreate it using make parser. If you push the fix, I'll merge it in, and we can add the higher level checking in a separate PR.

[sougou]

Note that the drop partition construct still needs fixing.

[systay]

DROP PARTITION needs to be fixed

[sougou]

@jmhodges this PR has fallen behind. Do you want to rework it?

[sougou]

Closing this due to inactivity.

[jmhodges]

The day job is taking up my brain. Won't be able to continue working on this for a while.

[sougou]

No problem. Feel free to resurrect this when you get the time.