iffyio commented on code in PR #1614:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1903247158
##########
src/ast/mod.rs:
##########
@@ -3341,16 +3341,13 @@ pub enum Statement {
value: Option<Value>,
is_eq: bool,
},
- /// ```sql
- /// LOCK TABLES <table_name> [READ [LOCAL] | [LOW_PRIORITY] WRITE]
- /// ```
- /// Note: this is a MySQL-specific statement. See
<https://dev.mysql.com/doc/refman/8.0/en/lock-tables.html>
- LockTables { tables: Vec<LockTable> },
+ /// See [`LockTables`].
+ LockTables(LockTables),
/// ```sql
/// UNLOCK TABLES
/// ```
/// Note: this is a MySQL-specific statement. See
<https://dev.mysql.com/doc/refman/8.0/en/lock-tables.html>
- UnlockTables,
+ UnlockTables(bool),
Review Comment:
```suggestion
UnlockTables(UnlockTables),
```
maybe we use a dedicated struct here as well? Its not clear what the bool
property implies otherwise
##########
src/ast/mod.rs:
##########
@@ -7278,16 +7279,126 @@ impl fmt::Display for SearchModifier {
}
}
+/// A `LOCK TABLE ..` statement. MySQL and Postgres variants are supported.
+///
+/// The MySQL and Postgres syntax variants are significant enough that they
+/// are explicitly represented as enum variants. In order to support additional
+/// databases in the future, this enum is marked as `#[non_exhaustive]`.
+///
+/// In MySQL, when multiple tables are mentioned in the statement the lock mode
+/// can vary per table.
+///
+/// In contrast, Postgres only allows specifying a single mode which is applied
+/// to all mentioned tables.
+///
+/// MySQL: see <https://dev.mysql.com/doc/refman/8.0/en/lock-tables.html>
+///
+/// ```sql
+/// LOCK [TABLE | TABLES] name [[AS] alias] locktype [,name [[AS] alias]
locktype]
+/// ````
+///
+/// Where *locktype* is:
+/// ```sql
+/// READ [LOCAL] | [LOW_PRIORITY] WRITE
+/// ```
+///
+/// Postgres: See <https://www.postgresql.org/docs/current/sql-lock.html>
+///
+/// ```sql
+/// LOCK [ TABLE ] [ ONLY ] name [, ...] [ IN lockmode MODE ] [ NOWAIT ]
+/// ```
+/// Where *lockmode* is one of:
+///
+/// ```sql
+/// ACCESS SHARE | ROW SHARE | ROW EXCLUSIVE | SHARE UPDATE EXCLUSIVE
+/// | SHARE | SHARE ROW EXCLUSIVE | EXCLUSIVE | ACCESS EXCLUSIVE
+/// ``````
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
-pub struct LockTable {
- pub table: Ident,
+#[non_exhaustive]
+pub enum LockTables {
Review Comment:
I think we want to avoid variants that are specific to dialects, those tend
to make it more difficult to reuse the parser code and ast representations
across dialects. Representation wise, I think both variants can be merged into
a struct with something like the following?
```rust
struct TableLock {
pub table: ObjectName,
pub alias: Option<Ident>,
pub lock_type: Option<LockTableType>,
}
struct LockTable {
pluralized_table_keyword: Option<bool>, // If None, then no table keyword
was provided
locks: Vec<TableLock>,
lock_mode: Option<LockTableType>,
only: bool,
no_wait: bool
}
```
similarly, the parser uses the same impl to create the struct
##########
src/ast/mod.rs:
##########
@@ -7299,17 +7410,34 @@ impl fmt::Display for LockTable {
if let Some(alias) = alias {
write!(f, "AS {alias} ")?;
}
- write!(f, "{lock_type}")?;
+ if let Some(lock_type) = lock_type {
+ write!(f, "{lock_type}")?;
+ }
Ok(())
}
}
+/// Table lock types.
+///
+/// `Read` & `Write` are MySQL-specfic.
+///
+/// AccessShare, RowShare, RowExclusive, ShareUpdateExclusive, Share,
+/// ShareRowExclusive, Exclusive, AccessExclusive are Postgres-specific.
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+#[non_exhaustive]
Review Comment:
```suggestion
```
I think we tend to not use this attribute, I think there are pros/cons with
using it but better to keep with the existing convention in this PR
--
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]
