Skip to content
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

Jump to bottom

ddl: Support the operation of dropping multi-indexes in one statement #20457

Merged
merged 100 commits into from
Sep 2, 2021
Merged

ddl: Support the operation of dropping multi-indexes in one statement #20457

merged 100 commits into from
Sep 2, 2021

Conversation

ou-bing
Copy link
Contributor

@ou-bing ou-bing commented Oct 14, 2020 •
edited by AilinKid
Loading

What problem does this PR solve?

Problem Summary:

What is changed and how it works?

Proposal: xxx

Support add multi-columns :

ALTER TABLE tbl_name
    [alter_specification [, alter_specification] ...]

alter_specification:
| DROP [INDEX] idx_name

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Release note

  • DDL: Support the operation of dropping multi-indexes in one statement

@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Oct 14, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Oct 14, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@sre-bot
Copy link
Contributor

sre-bot commented Oct 14, 2020

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@ou-bing ou-bing changed the title Feature/dropping multi indexes ddl: Support the operation of dropping multi-indexes in one statement Oct 14, 2020
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Oct 14, 2020
@ou-bing ou-bing marked this pull request as ready for review October 16, 2020 01:52
blacktear23
blacktear23 reviewed Oct 16, 2020
View reviewed changes
ddl/ddl_api.go Outdated Show resolved Hide resolved
@ti-srebot
Copy link
Contributor

@ou-bing, please update your pull request.

1 similar comment
@ti-srebot
Copy link
Contributor

@ou-bing, please update your pull request.

@ti-srebot
Copy link
Contributor

@ou-bing PR closed due to no update for a long time. Feel free to reopen it anytime.

@ti-srebot ti-srebot closed this Aug 10, 2021
@zimulala zimulala reopened this Aug 23, 2021
@zimulala
Copy link
Contributor

zimulala commented Aug 23, 2021 •
edited
Loading

Please fix conflicts. Are there warnings related tests?

@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 23, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2021
@ou-bing
Copy link
Contributor Author

ou-bing commented Aug 26, 2021

Please fix conflicts. Are there warnings related tests?

There are some related tests in testDropIndexesIfExists.

zimulala
zimulala reviewed Aug 31, 2021
View reviewed changes
ddl/ddl_api.go Outdated
@@ -2432,6 +2441,14 @@ func (d *ddl) AlterTable(ctx context.Context, sctx sessionctx.Context, ident ast
if !sctx.GetSessionVars().EnableChangeMultiSchema {
return errRunMultiSchemaChanges
}

if isDropIndexes(validSpecs) {
if err = d.DropIndexes(sctx, ident, validSpecs); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd better put it to line 2457, and add a "ActionDropIndexes" case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified the isSameTypeMultiSpecs function and add some test cases for drop both primary key and index.

zimulala
zimulala approved these changes Sep 2, 2021
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 2, 2021
@zimulala
Copy link
Contributor

zimulala commented Sep 2, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: f96b07b

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 2, 2021
@ti-chi-bot ti-chi-bot merged commit b35c7bc into pingcap:master Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Howie59 Howie59 Howie59 left review comments

@djshow832 djshow832 djshow832 left review comments

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@AilinKid AilinKid AilinKid left review comments

@blacktear23 blacktear23 blacktear23 left review comments

@morgo morgo morgo left review comments

@zimulala zimulala zimulala approved these changes

Assignees
No one assigned
Labels
first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. require-LGT3 Indicates that the PR requires three LGTM. sig/sql-infra SIG: SQL Infra size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support the operation of dropping multi-indexes in one statement