You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "freshtonic (via GitHub)" <gi...@apache.org> on 2024/12/20 05:27:58 UTC
[PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
freshtonic opened a new pull request, #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614
See: https://www.postgresql.org/docs/current/sql-lock.html
PG's full syntax for this statement is supported:
```
LOCK [ TABLE ] [ ONLY ] name [ * ] [, ...] [ IN lockmode MODE ] [ NOWAIT ]
where lockmode is one of:
ACCESS SHARE | ROW SHARE | ROW EXCLUSIVE | SHARE UPDATE EXCLUSIVE
| SHARE | SHARE ROW EXCLUSIVE | EXCLUSIVE | ACCESS EXCLUSIVE
```
It is implemented to not intefere with the roughly equivalent (but different) syntax in MySQL, by using a new Statement variant.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1923330659
##########
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 agree with @iffyio that following the existing patterns in this crate is the best thing for this PR
If we were starting a new project having dialect specific AST structures would make sense to consider but at this point ensuring the code is consistent is more important in my opinion
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1896659962
##########
src/parser/mod.rs:
##########
@@ -9604,7 +9604,13 @@ impl<'a> Parser<'a> {
top = Some(self.parse_top()?);
}
- let projection = self.parse_projection()?;
+ let projection = if dialect_of!(self is PostgreSqlDialect | GenericDialect)
+ && self.peek_keyword(Keyword::FROM)
+ {
+ vec![]
+ } else {
+ self.parse_projection()?
+ };
Review Comment:
ah yeah we could probably use an enum to represent the variants, something like?
```rust
enum LockTableKind {
TABLE
TABLES
}
Statement::LockTable { table_kind: Option<LockTableKind> }
```
see [TableSampleKind](https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/ast/query.rs#L1009) for example
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "freshtonic (via GitHub)" <gi...@apache.org>.
freshtonic commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1903356404
##########
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:
> None of this is a hill I will die on, but handling the burden of grammar differences in the parser and AST design means consuming the AST correctly in downstream projects will be easier.
What I mean by this:
When pieces of dialect-specific syntax are mixed in the same AST struct/enum variant the consumers have to understand the dialect-specific differences in order to know which fields they can safely ignore.
At CipherStash I wrote a type-inferencer for SQL statements in order to determine if specific transformations can be performed safely. It uses `sqlparser`'s AST and there are a lot of cases where I had to spend time understanding which AST node fields or combinations of field values I can safely ignore when I'm only targeting Postgres.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "freshtonic (via GitHub)" <gi...@apache.org>.
freshtonic commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1894703311
##########
src/parser/mod.rs:
##########
@@ -9604,7 +9604,13 @@ impl<'a> Parser<'a> {
top = Some(self.parse_top()?);
}
- let projection = self.parse_projection()?;
+ let projection = if dialect_of!(self is PostgreSqlDialect | GenericDialect)
+ && self.peek_keyword(Keyword::FROM)
+ {
+ vec![]
+ } else {
+ self.parse_projection()?
+ };
Review Comment:
@iffyio oh my bad - I should have branched from the apache `main` branch instead of our fork's `main` before pushing. I'll remedy this.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "freshtonic (via GitHub)" <gi...@apache.org>.
freshtonic commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1895681055
##########
src/parser/mod.rs:
##########
@@ -9604,7 +9604,13 @@ impl<'a> Parser<'a> {
top = Some(self.parse_top()?);
}
- let projection = self.parse_projection()?;
+ let projection = if dialect_of!(self is PostgreSqlDialect | GenericDialect)
+ && self.peek_keyword(Keyword::FROM)
+ {
+ vec![]
+ } else {
+ self.parse_projection()?
+ };
Review Comment:
@iffyio I tried this and it's not straightforward without storing a value on the variant the identifies the dialect that was used to parse the AST.
The following syntax would be problematic:
```sql
LOCK customers;
```
In PG, the `TABLE` keyword is optional. In MySQL one of `TABLE` or `TABLES` is mandatory.
The `Display` impl for `Statement`, in the `LockTable { .. }` match arm could potentially generate SQL that will not be parsable by Postgres if a `TABLES` keyword is emitted.
Is there precedent for choosing how to render an AST fragment using a stored value to encode the dialect (or a proxy to the dialect) that was used to parse the AST?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #1614: Parse Postgres's LOCK TABLE statement
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1907585091
##########
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:
Yeah I agree that downstream crates targeting a single dialect would be easier to implement by essentially having dialect specific AST representations (on the other extreme there are downstream crates that would like to process the AST in a dialect agnostic manner,, we also have custom dialects in other downstream crates that need support). I think there are pros/cons to this approach vs the current one followed by the parser which puts some of the responsibility on the downstream crate. I'm thinking in any case ideally we would want to keep to the current approach for the PR while shift in approaches could be tackled as its own dedicated proposal.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "freshtonic (via GitHub)" <gi...@apache.org>.
freshtonic commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1903354362
##########
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
MySQL's & Postgres's `LOCK` statements have minimal overlap. They are similar in name only.
- MySQL allows different lock modes per table vs Postgres one lock mode applied to all tables
- MySQL's lock modes and Postgres's lock modes do not overlap at all
- MySQL has freely interchangeable table keywords, one of which MUST be present: `TABLE` or `TABLES`
- Postgres has one `TABLE` keyword but it's optional
- Postgres supports additional (optional) `ONLY` and `NOWAIT` keywords
- It is _never_ valid to mix and match the Postgres-specific syntax with MySQL-specific syntax.
I agree that explicit database-specific AST fragments make parser reuse more difficult but in the case of `LOCK` pretty much nothing is reusable.
The db-specific AST pieces do make implementing `Display` a lot less error prone.
It also makes it (almost) impossible be able to represent invalid AST (e.g. a mix of PG & MySQL) except I didn't take a hard stance on this for `LockTableType` which does mix MySQL & Postgres bits.
None of this is a hill I will die on, but handling the burden of grammar differences in the parser and AST design means _consuming_ the AST correctly in downstream projects will be easier.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#issuecomment-2604104920
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#issuecomment-2745969727
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "freshtonic (via GitHub)" <gi...@apache.org>.
freshtonic commented on PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#issuecomment-2571280859
@iffyio I've pushed another attempt at this.
Munging the Postgres and MySQL versions together into the same `Statement::LockTables { .. }` variant was painful due to requiring additional fields that could be relied upon for a correct `Display` implementation which would produce a valid statement for both MySQL and Postgres.
In my opinion, introducing a `LockTables` enum (with two variants for Postgres & MySQL and which is marked `non_exhaustive` in order to support other variations in the future) is less of a cognitive burden than inlining all variations as lots of optional fields on the same struct.
That said, it does not appear to be an idiom that us used elsewhere so I don't have much confidence this PR will be accepted but one can hope :)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "freshtonic (via GitHub)" <gi...@apache.org>.
freshtonic commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1894703918
##########
src/ast/mod.rs:
##########
@@ -3336,6 +3336,27 @@ pub enum Statement {
is_eq: bool,
},
/// ```sql
+ /// LOCK [ TABLE ] [ ONLY ] name [ * ] [, ...] [ IN lockmode MODE ] [ NOWAIT ]
+ /// ```
+ /// Where *lockmode* is one of:
+ ///
+ /// ACCESS SHARE | ROW SHARE | ROW EXCLUSIVE | SHARE UPDATE EXCLUSIVE
+ /// | SHARE | SHARE ROW EXCLUSIVE | EXCLUSIVE | ACCESS EXCLUSIVE
+ ///
+ /// Note: this is a Postgres-specific statement. See <https://www.postgresql.org/docs/current/sql-lock.html>
+ LockTablesPG {
Review Comment:
I'll try doing that. FWIW the reason I didn't to begin with is that at it seemed like the MySQL version of the `LOCK ...` statement is quite different to PG's, but I didn't think too hard about it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "freshtonic (via GitHub)" <gi...@apache.org>.
freshtonic commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1895681055
##########
src/parser/mod.rs:
##########
@@ -9604,7 +9604,13 @@ impl<'a> Parser<'a> {
top = Some(self.parse_top()?);
}
- let projection = self.parse_projection()?;
+ let projection = if dialect_of!(self is PostgreSqlDialect | GenericDialect)
+ && self.peek_keyword(Keyword::FROM)
+ {
+ vec![]
+ } else {
+ self.parse_projection()?
+ };
Review Comment:
@iffyio I tried this and it's not straightforward without storing a value on the variant the identifies the dialect that was used to parse the AST.
The following syntax would be problematic (to render, in `Display`):
```sql
LOCK customers;
```
In PG, the `TABLE` keyword is optional. In MySQL one of `TABLE` or `TABLES` is mandatory.
The `Display` impl for `Statement`, in the `LockTable { .. }` match arm could potentially generate SQL that will not be parsable by Postgres if a `TABLES` keyword is emitted.
Is there precedent for choosing how to render an AST fragment using a stored value to encode the dialect (or a proxy to the dialect) that was used to parse the AST?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Parse Postgres's LOCK TABLE statement [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1614:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1614#discussion_r1894627136
##########
src/parser/mod.rs:
##########
@@ -9604,7 +9604,13 @@ impl<'a> Parser<'a> {
top = Some(self.parse_top()?);
}
- let projection = self.parse_projection()?;
+ let projection = if dialect_of!(self is PostgreSqlDialect | GenericDialect)
+ && self.peek_keyword(Keyword::FROM)
+ {
+ vec![]
+ } else {
+ self.parse_projection()?
+ };
Review Comment:
we can probably skip these changes in this PR given it's now in https://github.com/apache/datafusion-sqlparser-rs/pull/1613?
##########
src/ast/mod.rs:
##########
@@ -3336,6 +3336,27 @@ pub enum Statement {
is_eq: bool,
},
/// ```sql
+ /// LOCK [ TABLE ] [ ONLY ] name [ * ] [, ...] [ IN lockmode MODE ] [ NOWAIT ]
+ /// ```
+ /// Where *lockmode* is one of:
+ ///
+ /// ACCESS SHARE | ROW SHARE | ROW EXCLUSIVE | SHARE UPDATE EXCLUSIVE
+ /// | SHARE | SHARE ROW EXCLUSIVE | EXCLUSIVE | ACCESS EXCLUSIVE
+ ///
+ /// Note: this is a Postgres-specific statement. See <https://www.postgresql.org/docs/current/sql-lock.html>
+ LockTablesPG {
Review Comment:
Could we take a look at merging this into the existing `Statement::LockTables` variant? I think that would be the desirable path forward in order to avoid duplicating the statement type
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org