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 DROP PROCEDURE statement #1324

Merged
merged 5 commits into from
Jul 9, 2024

Conversation

LorrensP-2158466
Copy link
Contributor

Potentially closes #1282

This introduces a new statement: DropProcedure. It has the same fields as DropFunction because it also has the same syntax.

The other way i could have implemented this was using the Drop statement and introducing a new ObjectType called ObjectType::Procedure, but because DROP PROCEDURE ... is the same (just different word) as DROP FUNCTION..., I thought this was weird to do.

I also added a test case in the same way as DropFunction.

Let me know what you guys think of this!

@coveralls
Copy link

coveralls commented Jun 25, 2024

Pull Request Test Coverage Report for Build 9666189324

Details

  • 83 of 96 (86.46%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 88.997%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser/mod.rs 13 15 86.67%
src/ast/mod.rs 8 13 61.54%
tests/sqlparser_postgres.rs 62 68 91.18%
Files with Coverage Reduction New Missed Lines %
src/ast/mod.rs 1 81.37%
Totals Coverage Status
Change from base Build 9633122504: -0.01%
Covered Lines: 26353
Relevant Lines: 29611

💛 - Coveralls

src/ast/mod.rs Outdated
@@ -5867,12 +5893,12 @@ impl fmt::Display for DropFunctionOption {
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
pub struct DropFunctionDesc {
pub struct DropFuncDesc {
Copy link
Contributor

Choose a reason for hiding this comment

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

was it a specific reason to rename DropFunctionDesc to DropFuncDesc? if not we can probably undo the change given its a breaking change and the benefit is arguable, (and we have other similarly named objects like DropFunctionOption, parse_drop_function_desc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah you're right. I playing around with some new names since DropProcedure uses it. I'll change it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

done in bb0dad8

let option = match self.parse_one_of_keywords(&[Keyword::CASCADE, Keyword::RESTRICT]) {
Some(Keyword::CASCADE) => Some(ReferentialAction::Cascade),
Some(Keyword::RESTRICT) => Some(ReferentialAction::Restrict),
_ => None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_ => None,
Some(invalid) => return parser_err!(...)
None => None,

Should this rather return an error if the action is unexpected?
seems otherwise e.g. DROP PROCEDURE testproc SET NULL will be accepted by the parser but it will silently discard the SET NULL part (we can also include a test case demonstrating the behavior)

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 they way that parse_one_of_keywords works, it will only return CASCADE or RESTRICT here (never Some(..)).

I added a test in e782bf5 and changed the code to try and make this clearer

@alamb alamb changed the title Support drop procederue statement Support DROP PROCEDURE statement Jul 8, 2024
@coveralls
Copy link

coveralls commented Jul 8, 2024

Pull Request Test Coverage Report for Build 9839024282

Details

  • 83 of 99 (83.84%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 89.155%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser/mod.rs 11 14 78.57%
src/ast/mod.rs 8 13 61.54%
tests/sqlparser_postgres.rs 64 72 88.89%
Files with Coverage Reduction New Missed Lines %
src/ast/mod.rs 1 81.39%
Totals Coverage Status
Change from base Build 9838850938: -0.02%
Covered Lines: 26668
Relevant Lines: 29912

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented Jul 8, 2024

Thank you @LorrensP-2158466 and @iffyio

@LorrensP-2158466
Copy link
Contributor Author

Thanks for doing those final changes! It's very busy at the moment, sorry.

@alamb
Copy link
Contributor

alamb commented Jul 9, 2024

Thanks for doing those final changes! It's very busy at the moment, sorry.

No problems! The best software in my opinion is a good team effort. 🚀

@alamb alamb merged commit 9f60eb1 into apache:main Jul 9, 2024
10 checks passed
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.

Add support for DROP PROCEDURE
4 participants