Support add/drop attributes for table or partition #1271

rleungx · 2021-07-08T04:10:18Z

What problem does this PR solve?

It's used for the region label feature. Closes #1288.

What is changed and how it works?

This PR is going to support

ALTER TABLE t ATTRIBUTES="attr1,attr2,...,attrn"
ALTER TABLE t PARTITION p ATTRIBUTES="attr1,attr2,...,attrn"

to change the attributes of a table or a partition.

And use the following syntax to reset all attributes for a table or a partition:

ALTER TABLE t ATTRIBUTES=default
ALTER TABLE t PARTITION p ATTRIBUTES=default

Check List

Tests

Unit test

Related changes

Need to update the documentation
Need to be included in the release note

CLAassistant · 2021-07-08T04:10:26Z

All committers have signed the CLA.

kennytm · 2021-07-08T04:35:20Z

what is the difference between this and PLACEMENT POLICY?

JmPotato · 2021-07-08T05:07:02Z

ast/ddl.go

@@ -2648,6 +2651,19 @@ func (n *AlterTableSpec) Restore(ctx *format.RestoreCtx) error {
 				return errors.Annotatef(err, "An error occurred while restore AlterTableSpec.PlacementSpecs[%d]", i)
 			}
 		}
+	case AlterTableAlterPartitionAttributes:
+		if len(n.PartitionNames) != 1 {
+			return errors.Errorf("Maybe partition options are combined.")


This error message indicates the number of partion names is over 1, what about zero?

rleungx · 2021-07-08T05:30:27Z

what is the difference between this and PLACEMENT POLICY?

We are going to add some attributes to a table/partition to solve some specified problems. e.g., notmerge attribute means don't merge a table. It's different from the placement rule which mainly focuses on data placement.

tangenta · 2021-07-14T08:21:36Z

@rleungx We may also need to consider the removal syntax for attributes. Anyway, it would be better if there is a detailed design document.

rleungx · 2021-07-27T09:51:45Z

PTAL @djshow832 @xhebox, thanks.

rleungx · 2021-07-27T09:53:18Z

@rleungx We may also need to consider the removal syntax for attributes. Anyway, it would be better if there is a detailed design document.

How about using alter table t attributes=default for removing all attributes?

xhebox · 2021-07-27T10:20:29Z

parser.y

-			$$ = &ast.AlterTableSpec{
-				Tp:        ast.AlterTablePartition,
-				Partition: $1.(*ast.PartitionOptions),
+			if alterTableSpec, ok := $1.(*ast.AlterTableSpec); ok {


I suggest adding a new term ATTRIBUTES : "ATTRIBUTES" "=" stringLit | "DEFAULT", which will return the attributes spec, to reuse and write less code.

And the change here is not elegant for me. It is strange if PartitionOpt did not return PartitionOptions.

ti-chi-bot · 2021-07-28T05:15:46Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

djshow832
xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

nolouch

lgtm

ti-chi-bot · 2021-07-28T05:27:20Z

@nolouch: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

nolouch · 2021-07-28T05:42:05Z

parser_test.go

+		{"ALTER TABLE t ATTRIBUTES='str1,str2'", true, "ALTER TABLE `t` ATTRIBUTES='str1,str2'"},
+		{"ALTER TABLE t ATTRIBUTES=DEFAULT", true, "ALTER TABLE `t` ATTRIBUTES=DEFAULT"},
+		{"ALTER TABLE t ATTRIBUTES=default", true, "ALTER TABLE `t` ATTRIBUTES=DEFAULT"},
+		{"ALTER TABLE t ATTRIBUTES=DeFaUlT", true, "ALTER TABLE `t` ATTRIBUTES=DEFAULT"},


BTW, Default meaning empty or may have some default attributes？

It will reset the attributes. And since we don't have the default attributes for now, it can be regarded as empty.

Signed-off-by: Ryan Leung <rleungx@gmail.com>

djshow832

Rest LGTM

djshow832 · 2021-07-28T07:02:18Z

parser.y

+	"ATTRIBUTES" "=" "DEFAULT"
+	{
+		$$ = &ast.AttributesSpec{Default: true}
+	}
+|	"ATTRIBUTES" "=" stringLit
+	{
+		$$ = &ast.AttributesSpec{Default: false, Attributes: $3}
+	}
+


As I recall, = is optional in all table options in MySQL.

djshow832 · 2021-07-28T07:04:00Z

parser_test.go

@@ -2783,6 +2783,20 @@ func (s *testParserSuite) TestDDL(c *C) {
 		{"ALTER TABLE t ALTER PLACEMENT POLICY CONSTRAINTS='str' ROLE=leader REPLICAS=1", true, "ALTER TABLE `t` ALTER PLACEMENT POLICY CONSTRAINTS='str' ROLE=LEADER REPLICAS=1"},
 		{"ALTER TABLE t ADD PLACEMENT POLICY CONSTRAINTS='str1' ROLE=leader REPLICAS=1, ADD PLACEMENT POLICY CONSTRAINTS='str2' ROLE=leader REPLICAS=1", true, "ALTER TABLE `t` ADD PLACEMENT POLICY CONSTRAINTS='str1' ROLE=LEADER REPLICAS=1, ADD PLACEMENT POLICY CONSTRAINTS='str2' ROLE=LEADER REPLICAS=1"},

+		// alter attributes
+		{"ALTER TABLE t ATTRIBUTES='str'", true, "ALTER TABLE `t` ATTRIBUTES='str'"},
+		{"ALTER TABLE t ATTRIBUTES='str1,str2'", true, "ALTER TABLE `t` ATTRIBUTES='str1,str2'"},


How about double quotation marks, like "str1, str2"?

Signed-off-by: Ryan Leung <rleungx@gmail.com>

djshow832 · 2021-07-28T07:16:10Z

/merge

ti-chi-bot · 2021-07-28T07:16:12Z

This pull request has been accepted and is ready to merge.

Commit hash: 8938372

morgo self-requested a review July 8, 2021 05:04

JmPotato reviewed Jul 8, 2021

View reviewed changes

morgo removed their request for review July 8, 2021 21:26

rleungx force-pushed the support-region-label branch from 143e065 to 6ebf0e8 Compare July 13, 2021 05:57

ti-chi-bot added the needs-rebase label Jul 16, 2021

rleungx mentioned this pull request Jul 23, 2021

ddl: support alter table attributes to add a label rule pingcap/tidb#26511

Merged

6 tasks

rleungx force-pushed the support-region-label branch from a8bb32c to 3332b2d Compare July 27, 2021 09:08

ti-chi-bot removed the needs-rebase label Jul 27, 2021

rleungx force-pushed the support-region-label branch from 3332b2d to ea7714f Compare July 27, 2021 09:49

rleungx requested review from xhebox and djshow832 July 27, 2021 09:51

rleungx force-pushed the support-region-label branch from ea7714f to 4d99cd9 Compare July 27, 2021 09:56

xhebox reviewed Jul 27, 2021

View reviewed changes

xhebox approved these changes Jul 28, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 LGT1 label Jul 28, 2021

nolouch approved these changes Jul 28, 2021

View reviewed changes

nolouch reviewed Jul 28, 2021

View reviewed changes

rleungx added 5 commits July 28, 2021 14:54

support alter table add/drop attributes

64549e5

Signed-off-by: Ryan Leung <rleungx@gmail.com>

support alter table alter partition add/drop attributes

5964aff

Signed-off-by: Ryan Leung <rleungx@gmail.com>

change syntax

59c7e19

Signed-off-by: Ryan Leung <rleungx@gmail.com>

add two more actions

2de97fd

Signed-off-by: Ryan Leung <rleungx@gmail.com>

support default keyword for attribute

c27f548

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx added 3 commits July 28, 2021 14:55

tiny fix

f0f6828

Signed-off-by: Ryan Leung <rleungx@gmail.com>

address comments

17535d6

Signed-off-by: Ryan Leung <rleungx@gmail.com>

resolve the conflict

ad6dc6b

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the support-region-label branch from c731350 to ad6dc6b Compare July 28, 2021 06:59

djshow832 reviewed Jul 28, 2021

View reviewed changes

address comments

8938372

Signed-off-by: Ryan Leung <rleungx@gmail.com>

djshow832 approved these changes Jul 28, 2021

View reviewed changes

ti-chi-bot added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Jul 28, 2021

ti-chi-bot added the status/can-merge label Jul 28, 2021

ti-chi-bot merged commit 915a010 into pingcap:master Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support add/drop attributes for table or partition #1271

Support add/drop attributes for table or partition #1271

rleungx commented Jul 8, 2021 •

edited

Loading

CLAassistant commented Jul 8, 2021 •

edited

Loading

kennytm commented Jul 8, 2021

JmPotato Jul 8, 2021

rleungx commented Jul 8, 2021

tangenta commented Jul 14, 2021

rleungx commented Jul 27, 2021

rleungx commented Jul 27, 2021

xhebox Jul 27, 2021 •

edited

Loading

ti-chi-bot commented Jul 28, 2021 •

edited

Loading

nolouch left a comment

ti-chi-bot commented Jul 28, 2021

nolouch Jul 28, 2021

rleungx Jul 28, 2021 •

edited

Loading

djshow832 left a comment

djshow832 Jul 28, 2021

djshow832 Jul 28, 2021

djshow832 commented Jul 28, 2021

ti-chi-bot commented Jul 28, 2021

Support add/drop attributes for table or partition #1271

Support add/drop attributes for table or partition #1271

Conversation

rleungx commented Jul 8, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

CLAassistant commented Jul 8, 2021 • edited Loading

kennytm commented Jul 8, 2021

JmPotato Jul 8, 2021

Choose a reason for hiding this comment

rleungx commented Jul 8, 2021

tangenta commented Jul 14, 2021

rleungx commented Jul 27, 2021

rleungx commented Jul 27, 2021

xhebox Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

ti-chi-bot commented Jul 28, 2021 • edited Loading

nolouch left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jul 28, 2021

nolouch Jul 28, 2021

Choose a reason for hiding this comment

rleungx Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

djshow832 left a comment

Choose a reason for hiding this comment

djshow832 Jul 28, 2021

Choose a reason for hiding this comment

djshow832 Jul 28, 2021

Choose a reason for hiding this comment

djshow832 commented Jul 28, 2021

ti-chi-bot commented Jul 28, 2021

rleungx commented Jul 8, 2021 •

edited

Loading

CLAassistant commented Jul 8, 2021 •

edited

Loading

xhebox Jul 27, 2021 •

edited

Loading

ti-chi-bot commented Jul 28, 2021 •

edited

Loading

rleungx Jul 28, 2021 •

edited

Loading