Skip to content

sql: add a syntax hint for ALTER PARTITION #40709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

solongordon
Copy link
Contributor

If a user tries to modify multiple index partitions via ALTER PARTITION
... OF TABLE, they now receive a hint to use ALTER PARTITION ... OF
INDEX instead.

I also improved the help docs for \h ALTER PARTITION to specify how to
use the wildcard syntax.

Fixes #40387

Release note: None

@solongordon solongordon requested review from knz and rohany September 12, 2019 15:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor Author

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rohany)


pkg/sql/parser/sql.y, line 1512 at r1 (raw file):

    err := errors.New("index wildcard unsupported in ALTER PARTITION ... OF TABLE")
    err = errors.WithHint(err, "try ALTER PARTITION <partition> OF INDEX <tablename>@*")
    return setErr(sqllex, err)

@knz Is this a reasonable way to return a syntax hint? I couldn't find other examples of this in sql.y.

@solongordon
Copy link
Contributor Author

Release justification: Low risk improvements to documentation and error messaging.

Copy link
Contributor

@rohany rohany left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz, @rohany, and @solongordon)


pkg/sql/parser/sql.y, line 1173 at r1 (raw file):

//
// Commands:
//   -- Alter a single partition of a table or its indexes.

its unclear to me from the comments what the difference between this and alter partition of index is

@@ -1499,6 +1505,12 @@ alter_zone_partition_stmt:
s.AllIndexes = true
$$.val = s
}
| ALTER PARTITION partition_name OF TABLE table_name '@' '*' error
Copy link
Contributor

Choose a reason for hiding this comment

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

You can (and should) test the hint reporting in parse_test.go, in TestParseError

@knz
Copy link
Contributor

knz commented Sep 12, 2019

Is this a reasonable way to return a syntax hint?

yes 💯

If a user tries to modify multiple index partitions via ALTER PARTITION
... OF TABLE, they now receive a hint to use ALTER PARTITION ... OF
INDEX instead.

I also improved the help docs for `\h ALTER PARTITION` to specify how to
use the wildcard syntax.

Fixes cockroachdb#40387

Release note: None
@solongordon solongordon force-pushed the partition-wildcard-better-error branch from b2c438f to 838cfa1 Compare September 12, 2019 16:33
Copy link
Contributor Author

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rohany)


pkg/sql/parser/sql.y, line 1173 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

its unclear to me from the comments what the difference between this and alter partition of index is

I changed around the wording a bit. Let me know if that helps.


pkg/sql/parser/sql.y, line 1508 at r1 (raw file):

Previously, knz (kena) wrote…

You can (and should) test the hint reporting in parse_test.go, in TestParseError

Done.

@rohany
Copy link
Contributor

rohany commented Sep 12, 2019

lgtm

@solongordon
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Sep 12, 2019
40709: sql: add a syntax hint for ALTER PARTITION r=solongordon a=solongordon

If a user tries to modify multiple index partitions via ALTER PARTITION
... OF TABLE, they now receive a hint to use ALTER PARTITION ... OF
INDEX instead.

I also improved the help docs for `\h ALTER PARTITION` to specify how to
use the wildcard syntax.

Fixes #40387

Release note: None

Co-authored-by: Solon Gordon <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 12, 2019

Build succeeded

@craig craig bot merged commit 838cfa1 into cockroachdb:master Sep 12, 2019
@solongordon solongordon deleted the partition-wildcard-better-error branch September 13, 2019 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: new partiton @* doesn't work with table/poor error message
4 participants