Skip to content

Support ALTER TABLE DROP PRIMARY KEY#682

Merged
alamb merged 3 commits into
apache:mainfrom
ding-young:alter-table/drop-pk
Nov 2, 2022
Merged

Support ALTER TABLE DROP PRIMARY KEY#682
alamb merged 3 commits into
apache:mainfrom
ding-young:alter-table/drop-pk

Conversation

@ding-young

@ding-young ding-young commented Oct 21, 2022

Copy link
Copy Markdown
Contributor

Issue

resolves #591

@coveralls

coveralls commented Oct 21, 2022 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 3364446229

  • 7 of 7 (100.0%) changed or added relevant lines in 3 files are covered.
  • 411 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.03%) to 85.974%
Files with Coverage Reduction New Missed Lines %
src/ast/ddl.rs 1 82.29%
src/ast/value.rs 7 87.5%
src/ast/data_type.rs 13 88.62%
tests/sqlparser_common.rs 54 96.85%
src/parser.rs 336 83.72%
Totals Coverage Status
Change from base Build 3292360592: 0.03%
Covered Lines: 10923
Relevant Lines: 12705
💛 - Coveralls

AugustoFKL
AugustoFKL suggested changes Oct 29, 2022
View reviewed changes
Comment thread src/parser.rs Outdated
name,
cascade,
}
} else if self.parse_keyword(Keyword::PRIMARY) {

@AugustoFKL AugustoFKL Oct 29, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ding-young ALTER TABLE tb DROP PRIMARY isn't allowed and, therefore, shouldn't be acceptable.

The let _ in the next line is permissive over this.

Also, I think this is a MySQL-specific statement. Could you please add the validation to accept only MySQL and GenericDialect, please?

@alamb alamb changed the title Parse alter table drop primary key Support ALTER TABLE DROP PRIMARY KEY Oct 31, 2022
alamb
alamb reviewed Oct 31, 2022
View reviewed changes

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ding-young do you have time to implement @AugustoFKL 's suggestion? Otherwise this PR looks good to me

@ding-young

ding-young commented Oct 31, 2022

Copy link
Copy Markdown
Contributor Author

@ding-young do you have time to implement @AugustoFKL 's suggestion? Otherwise this PR looks good to me

Yes, I'll work on it soon.

@ding-young

ding-young commented Oct 31, 2022

Copy link
Copy Markdown
Contributor Author

@AugustoFKL Thank you for feedback. I changed the code 😃

AugustoFKL
AugustoFKL approved these changes Nov 1, 2022
View reviewed changes

@AugustoFKL AugustoFKL left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@alamb ready for approval :)

@alamb

alamb commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

Thanks @ding-young and @AugustoFKL !

@alamb alamb merged commit 27c3ec8 into apache:main Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

+1 more reviewer

@AugustoFKL AugustoFKL AugustoFKL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MySQL "ALTER ... DROP PRIMARY KEY" is not parsed correctly

4 participants

@ding-young @coveralls @alamb @AugustoFKL