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.

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

Support ALTER INDEX {INDEX_NAME} RENAME TO {NEW_INDEX_NAME} #767

Merged
merged 4 commits into from Dec 28, 2022

Conversation

devgony
Copy link
Contributor

@devgony devgony commented Dec 19, 2022

Goal

To resolve #714, this PR starts with ALTER INDEX .. RENAME .. to implemet ALTER INDEX base

Todo

  • add AlterIndex match arm
  • fix unreachable with Parser.expected

operation,
})
}
_ => unreachable!(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should we handle this unreachable? 🤔
Is there a way to get fit enum of keywords that i only need (TABLE, INDEX)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ok and a consequence of using expect_one_of_keywords

Perhaps a comment

Suggested change
_ => unreachable!(),
// unreachable because expect_one_of_keywords used above
_ => unreachable!(),

Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @devgony -- this looks good to me.

let if_not_exists =
self.parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]);
if self.parse_keyword(Keyword::PARTITION) {
let object_type = self.expect_one_of_keywords(&[Keyword::TABLE, Keyword::INDEX])?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

reviewing with https://github.com/sqlparser-rs/sqlparser-rs/pull/767/files?w=1 ignoring whitespace made the changes much easier for me to see

let using = if is_postgresql && self.parse_keyword(Keyword::USING) {
Some(self.parse_expr()?)
Keyword::INDEX => {
let _ = self.parse_keyword(Keyword::ONLY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ignoring the error -- it should be something like

Suggested change
let _ = self.parse_keyword(Keyword::ONLY);
self.parse_keyword(Keyword::ONLY)?;

However, it isn't clear to me why it is trying to parse an ONLY as part of an alter index statement

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this is the same in some statements above. I'll clean it up in a follow on PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed in #773

operation,
})
}
_ => unreachable!(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ok and a consequence of using expect_one_of_keywords

Perhaps a comment

Suggested change
_ => unreachable!(),
// unreachable because expect_one_of_keywords used above
_ => unreachable!(),

@coveralls
Copy link

coveralls commented Dec 28, 2022

Pull Request Test Coverage Report for Build 3794320019

  • 130 of 140 (92.86%) changed or added relevant lines in 4 files are covered.
  • 1016 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.05%) to 86.388%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/ddl.rs 3 4 75.0%
src/parser.rs 115 124 92.74%
Files with Coverage Reduction New Missed Lines %
src/lib.rs 3 10.02%
src/test_utils.rs 8 83.33%
src/tokenizer.rs 51 89.69%
tests/sqlparser_common.rs 79 97.29%
src/ast/mod.rs 249 77.62%
src/parser.rs 626 83.01%
Totals Coverage Status
Change from base Build 3720056662: 0.05%
Covered Lines: 13004
Relevant Lines: 15053

💛 - Coveralls

@alamb alamb merged commit 61c661c into sqlparser-rs:main Dec 28, 2022
@alamb
Copy link
Collaborator

alamb commented Dec 28, 2022

Thanks again @devgony

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.

Support ALTER INDEX statement
3 participants