iffyio commented on code in PR #2273:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2273#discussion_r2904448633
##########
src/ast/mod.rs:
##########
@@ -4624,6 +4624,19 @@ pub enum Statement {
is_eq: bool,
},
/// ```sql
+ /// LOCK [ TABLE ] [ ONLY ] name [ * ] [, ...] [ IN lockmode MODE ] [
NOWAIT ]
+ /// ```
+ ///
+ /// Note: this is a PostgreSQL-specific statement. See
<https://www.postgresql.org/docs/current/sql-lock.html>
+ Lock {
Review Comment:
instead of using an anonymous struct we can introduce a `LockTable` struct
(also I think the statement should be `LockTable` instead of `Lock`?
##########
src/ast/mod.rs:
##########
@@ -4624,6 +4624,19 @@ pub enum Statement {
is_eq: bool,
},
/// ```sql
+ /// LOCK [ TABLE ] [ ONLY ] name [ * ] [, ...] [ IN lockmode MODE ] [
NOWAIT ]
+ /// ```
+ ///
+ /// Note: this is a PostgreSQL-specific statement. See
<https://www.postgresql.org/docs/current/sql-lock.html>
Review Comment:
```suggestion
/// See <https://www.postgresql.org/docs/current/sql-lock.html>
```
##########
tests/sqlparser_postgres.rs:
##########
@@ -8703,3 +8703,39 @@ fn parse_pg_analyze() {
_ => panic!("Expected Analyze, got: {stmt:?}"),
}
}
+
+#[test]
+fn parse_lock_table() {
+ pg_and_generic().one_statement_parses_to(
+ "LOCK public.widgets IN EXCLUSIVE MODE",
+ "LOCK TABLE public.widgets IN EXCLUSIVE MODE",
+ );
+ pg_and_generic().one_statement_parses_to(
Review Comment:
can we ensure that the tests have coverage for all lock table modes?
##########
tests/sqlparser_postgres.rs:
##########
@@ -8703,3 +8703,39 @@ fn parse_pg_analyze() {
_ => panic!("Expected Analyze, got: {stmt:?}"),
}
}
+
+#[test]
+fn parse_lock_table() {
+ pg_and_generic().one_statement_parses_to(
+ "LOCK public.widgets IN EXCLUSIVE MODE",
+ "LOCK TABLE public.widgets IN EXCLUSIVE MODE",
+ );
+ pg_and_generic().one_statement_parses_to(
+ "LOCK TABLE ONLY public.widgets, analytics.events * IN SHARE ROW
EXCLUSIVE MODE NOWAIT",
+ "LOCK TABLE ONLY public.widgets, analytics.events * IN SHARE ROW
EXCLUSIVE MODE NOWAIT",
+ );
+ pg_and_generic().one_statement_parses_to(
+ "LOCK TABLE public.widgets NOWAIT",
+ "LOCK TABLE public.widgets NOWAIT",
+ );
Review Comment:
where applicable can we use `verified_stmt` instead?
##########
src/parser/mod.rs:
##########
@@ -697,6 +697,9 @@ impl<'a> Parser<'a> {
// `INSTALL` is duckdb specific
https://duckdb.org/docs/extensions/overview
Keyword::INSTALL if self.dialect.supports_install() =>
self.parse_install(),
Keyword::LOAD => self.parse_load(),
+ Keyword::LOCK if dialect_of!(self is PostgreSqlDialect |
GenericDialect) => {
Review Comment:
```suggestion
Keyword::LOCK => {
```
I think we can skip the dialect check and let the parser always accept the
statement
##########
src/ast/mod.rs:
##########
@@ -4624,6 +4624,19 @@ pub enum Statement {
is_eq: bool,
},
/// ```sql
+ /// LOCK [ TABLE ] [ ONLY ] name [ * ] [, ...] [ IN lockmode MODE ] [
NOWAIT ]
+ /// ```
+ ///
+ /// Note: this is a PostgreSQL-specific statement. See
<https://www.postgresql.org/docs/current/sql-lock.html>
+ Lock {
+ /// List of tables to lock.
+ tables: Vec<LockTableTarget>,
+ /// Optional lock mode. PostgreSQL defaults to `ACCESS EXCLUSIVE` when
omitted.
Review Comment:
```suggestion
/// Lock mode.
```
we can avoid describing postgres semantics
##########
src/parser/mod.rs:
##########
@@ -697,6 +697,9 @@ impl<'a> Parser<'a> {
// `INSTALL` is duckdb specific
https://duckdb.org/docs/extensions/overview
Keyword::INSTALL if self.dialect.supports_install() =>
self.parse_install(),
Keyword::LOAD => self.parse_load(),
+ Keyword::LOCK if dialect_of!(self is PostgreSqlDialect |
GenericDialect) => {
+ self.parse_lock_table_statement()
Review Comment:
See the
[CASE](https://github.com/mjbshaw/datafusion-sqlparser-rs/blob/69f04201df97c01238d8c5997358c590888db89d/src/parser/mod.rs#L608)
statement for an example we can follow. We can rewind the token befor calling
the parse_lock function so that the function is standalone. Then the function
should return a struct instead of the `Statement`
##########
tests/sqlparser_postgres.rs:
##########
@@ -8703,3 +8703,39 @@ fn parse_pg_analyze() {
_ => panic!("Expected Analyze, got: {stmt:?}"),
}
}
+
+#[test]
+fn parse_lock_table() {
+ pg_and_generic().one_statement_parses_to(
+ "LOCK public.widgets IN EXCLUSIVE MODE",
+ "LOCK TABLE public.widgets IN EXCLUSIVE MODE",
+ );
+ pg_and_generic().one_statement_parses_to(
+ "LOCK TABLE ONLY public.widgets, analytics.events * IN SHARE ROW
EXCLUSIVE MODE NOWAIT",
+ "LOCK TABLE ONLY public.widgets, analytics.events * IN SHARE ROW
EXCLUSIVE MODE NOWAIT",
+ );
+ pg_and_generic().one_statement_parses_to(
+ "LOCK TABLE public.widgets NOWAIT",
+ "LOCK TABLE public.widgets NOWAIT",
+ );
+}
+
+#[test]
+fn parse_lock_table_ast() {
Review Comment:
can we merge this test case into the test function above?
##########
src/parser/mod.rs:
##########
@@ -18385,6 +18388,80 @@ impl<'a> Parser<'a> {
})
}
+ /// Parse a PostgreSQL `LOCK TABLE` statement.
+ pub fn parse_lock_table_statement(&mut self) -> Result<Statement,
ParserError> {
+ if self.peek_keyword(Keyword::TABLES) {
+ return self.expected_ref("TABLE or a table name",
self.peek_token_ref());
+ }
+
+ let _ = self.parse_keyword(Keyword::TABLE);
+ let tables =
self.parse_comma_separated(Parser::parse_lock_table_target)?;
+ let lock_mode = if self.parse_keyword(Keyword::IN) {
+ let lock_mode = self.parse_lock_table_mode()?;
+ self.expect_keyword(Keyword::MODE)?;
+ Some(lock_mode)
+ } else {
+ None
+ };
+ let nowait = self.parse_keyword(Keyword::NOWAIT);
+
+ Ok(Statement::Lock {
+ tables,
+ lock_mode,
+ nowait,
+ })
+ }
+
+ fn parse_lock_table_target(&mut self) -> Result<LockTableTarget,
ParserError> {
+ let only = self.parse_keyword(Keyword::ONLY);
+ let name = self.parse_object_name(false)?;
+ let has_asterisk = self.consume_token(&Token::Mul);
Review Comment:
we seem to be lacking a test case where `has_asterisk` is false?
##########
src/parser/mod.rs:
##########
@@ -18385,6 +18388,80 @@ impl<'a> Parser<'a> {
})
}
+ /// Parse a PostgreSQL `LOCK TABLE` statement.
+ pub fn parse_lock_table_statement(&mut self) -> Result<Statement,
ParserError> {
+ if self.peek_keyword(Keyword::TABLES) {
+ return self.expected_ref("TABLE or a table name",
self.peek_token_ref());
+ }
+
+ let _ = self.parse_keyword(Keyword::TABLE);
+ let tables =
self.parse_comma_separated(Parser::parse_lock_table_target)?;
+ let lock_mode = if self.parse_keyword(Keyword::IN) {
+ let lock_mode = self.parse_lock_table_mode()?;
+ self.expect_keyword(Keyword::MODE)?;
+ Some(lock_mode)
+ } else {
+ None
+ };
+ let nowait = self.parse_keyword(Keyword::NOWAIT);
+
+ Ok(Statement::Lock {
+ tables,
+ lock_mode,
+ nowait,
+ })
+ }
+
+ fn parse_lock_table_target(&mut self) -> Result<LockTableTarget,
ParserError> {
+ let only = self.parse_keyword(Keyword::ONLY);
+ let name = self.parse_object_name(false)?;
+ let has_asterisk = self.consume_token(&Token::Mul);
+
+ Ok(LockTableTarget {
+ name,
+ only,
+ has_asterisk,
+ })
+ }
+
+ fn parse_lock_table_mode(&mut self) -> Result<LockTableMode, ParserError> {
+ if self.parse_keyword(Keyword::ACCESS) {
Review Comment:
I think a simpler/clearer way to write this function would be something like
the following?
```rust
if self.parse_keywords(ACCESS, SHARE) { ... }
else if self.parse_keywords(ACESS, EXCLUSIVE) { ... }
...
```
that way the accounted for cases are visible, and we avoid spreading out the
error cases
##########
src/parser/mod.rs:
##########
@@ -18385,6 +18388,80 @@ impl<'a> Parser<'a> {
})
}
+ /// Parse a PostgreSQL `LOCK TABLE` statement.
+ pub fn parse_lock_table_statement(&mut self) -> Result<Statement,
ParserError> {
+ if self.peek_keyword(Keyword::TABLES) {
+ return self.expected_ref("TABLE or a table name",
self.peek_token_ref());
+ }
+
+ let _ = self.parse_keyword(Keyword::TABLE);
+ let tables =
self.parse_comma_separated(Parser::parse_lock_table_target)?;
+ let lock_mode = if self.parse_keyword(Keyword::IN) {
+ let lock_mode = self.parse_lock_table_mode()?;
+ self.expect_keyword(Keyword::MODE)?;
+ Some(lock_mode)
+ } else {
+ None
+ };
+ let nowait = self.parse_keyword(Keyword::NOWAIT);
+
+ Ok(Statement::Lock {
+ tables,
+ lock_mode,
+ nowait,
+ })
+ }
+
+ fn parse_lock_table_target(&mut self) -> Result<LockTableTarget,
ParserError> {
+ let only = self.parse_keyword(Keyword::ONLY);
+ let name = self.parse_object_name(false)?;
+ let has_asterisk = self.consume_token(&Token::Mul);
+
+ Ok(LockTableTarget {
+ name,
+ only,
+ has_asterisk,
+ })
+ }
+
+ fn parse_lock_table_mode(&mut self) -> Result<LockTableMode, ParserError> {
+ if self.parse_keyword(Keyword::ACCESS) {
+ return match self.expect_one_of_keywords(&[Keyword::SHARE,
Keyword::EXCLUSIVE])? {
+ Keyword::SHARE => Ok(LockTableMode::AccessShare),
+ Keyword::EXCLUSIVE => Ok(LockTableMode::AccessExclusive),
+ unexpected_keyword => Err(ParserError::ParserError(format!(
Review Comment:
can this use one of the error helper methods? (similar for the other places)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
