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

Add Postgres-specific statements PREPARE, EXECUTE and DEALLOCATE #243

Merged
merged 4 commits into from
Jul 28, 2020
Merged

Add Postgres-specific statements PREPARE, EXECUTE and DEALLOCATE #243

merged 4 commits into from
Jul 28, 2020

Conversation

silathdiir
Copy link
Contributor

@coveralls
Copy link

coveralls commented Jul 27, 2020

Pull Request Test Coverage Report for Build 185417591

  • 93 of 96 (96.88%) changed or added relevant lines in 3 files are covered.
  • 146 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.1%) to 91.76%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/mod.rs 13 16 81.25%
Files with Coverage Reduction New Missed Lines %
tests/sqlparser_common.rs 25 96.81%
src/ast/mod.rs 41 81.09%
src/parser.rs 80 88.88%
Totals Coverage Status
Change from base Build 175941714: 0.1%
Covered Lines: 4354
Relevant Lines: 4745

💛 - Coveralls

Copy link
Contributor

@nickolay nickolay left a comment

Choose a reason for hiding this comment

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

Thanks! Please see my comments inline...

src/ast/mod.rs Outdated
/// ASSERT <condition> [AS <message>]
Assert {
condition: Expr,
message: Option<Expr>,
},
/// DEALLOCATE [ PREPARE ] { name | ALL }
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be enclosed in backticks (as well as the ASSERT comment above -- sorry about missing that!), so that cargo doc output is properly formatted. Same below.

src/ast/mod.rs Outdated
Comment on lines 563 to 564
name: Option<ObjectName>,
all: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to handle ALL separately, from the parser viewpoint, it's as good an object name, as any. Can name be schema-qualified? The docs are not clear on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I tried to check and test again. It is not schema-qualified. I will remove all and fix type of name to an Ident.

src/ast/mod.rs Outdated
Deallocate {
name: Option<ObjectName>,
all: bool,
prepare: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be meaningless syntax. It's fine to keep it, but maybe note that PostgreSQL docs say it's ignored?

Copy link
Contributor Author

@silathdiir silathdiir Jul 28, 2020

Choose a reason for hiding this comment

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

Yes. The syntax is meaningless. But I think if we should output (format) the useless keyword PREPARE as the SQL passed into. What's your opinion?

src/ast/mod.rs Outdated
Comment on lines 849 to 853
Statement::Deallocate { name, all, prepare } => {
f.write_str("DEALLOCATE ")?;
if *prepare {
f.write_str("PREPARE ")?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we're inconsistent about that, but I believe using the same style as in the Statement::CreateTable branch will yield smaller code (especially we figure out the ALL vs object name question above).

src/ast/mod.rs Outdated
if !data_types.is_empty() {
write!(f, "({}) ", display_comma_separated(data_types))?;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I know we're inconsistent about this, but it seems that we usually don't add blank lines in situations like this.

@@ -20,7 +20,7 @@ impl Dialect for PostgreSqlDialect {
// See https://www.postgresql.org/docs/11/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
// We don't yet support identifiers beginning with "letters with
// diacritical marks and non-Latin letters"
(ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || ch == '_'
(ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || ch == '_' || ch == '$'
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't match the documentation linked just above this line (Postgres seems to have $1 and $$comments$$, but I don't see how $foo is supposed to be parsed), so I'd like to split this separately. You should be able to keep it in a dialect of your own for now.

src/parser.rs Outdated
let name = self.parse_object_name()?;

let mut parameters = vec![];
if self.expect_token(&Token::LParen).is_ok() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have confusingly named consume_token for that. Same below.

@@ -423,6 +423,139 @@ fn parse_show() {
)
}

#[test]
fn parse_deallocate() {
let stmt = pg().verified_stmt("DEALLOCATE a");
Copy link
Contributor

Choose a reason for hiding this comment

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

Use pg_and_generic() please.

@silathdiir silathdiir requested a review from nickolay July 28, 2020 08:29
@nickolay nickolay merged commit 8020b2e into apache:main Jul 28, 2020
@nickolay
Copy link
Contributor

Excellent, thank you!

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.

PREPARE, EXECUTE and DEALLOCATE are not top-level statements
3 participants