iffyio commented on code in PR #1538:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1538#discussion_r1855409232
##########
src/parser/mod.rs:
##########
@@ -4693,9 +4699,59 @@ impl<'a> Parser<'a> {
if_not_exists,
temporary,
to,
+ params: mysql_create_view_params,
})
}
+ /// Parse optional algorithm, definer, and security context parameters for
[MySQL]
+ ///
+ /// [MySQL]: https://dev.mysql.com/doc/refman/9.1/en/create-view.html
+ fn parse_mysql_create_view_params(&mut self) ->
Result<Option<MySQLViewParams>, ParserError> {
+ let algorithm = if self.parse_keyword(Keyword::ALGORITHM) {
+ self.expect_token(&Token::Eq)?;
+ Some(
+ match self.expect_one_of_keywords(&[
+ Keyword::UNDEFINED,
+ Keyword::MERGE,
+ Keyword::TEMPTABLE,
+ ])? {
+ Keyword::UNDEFINED => MySQLViewAlgorithm::Undefined,
+ Keyword::MERGE => MySQLViewAlgorithm::Merge,
+ Keyword::TEMPTABLE => MySQLViewAlgorithm::TempTable,
+ _ => unreachable!(),
Review Comment:
For the unreachable branches, we can return an error instead of crashing?
That would prevent bugs in the parser from crashing users' systems
##########
src/tokenizer.rs:
##########
@@ -1202,6 +1202,9 @@ impl<'a> Tokenizer<'a> {
}
}
Some(' ') => Ok(Some(Token::AtSign)),
+ Some('\'') => Ok(Some(Token::AtSign)),
+ Some('\"') => Ok(Some(Token::AtSign)),
+ Some('`') => Ok(Some(Token::AtSign)),
Review Comment:
the changes feel unexpected, took a quick look it seems rather a bug that
mysql dialect is currently configured to accept `@` as an identifier start?
[their doc](https://dev.mysql.com/doc/refman/8.0/en/identifiers.html) seems to
suggest otherwise?
##########
src/parser/mod.rs:
##########
@@ -11023,23 +11103,43 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_grantee(&mut self) -> Result<Grantee, ParserError> {
+ let user = self.parse_identifier(false)?;
+ if dialect_of!(self is MySqlDialect | GenericDialect) &&
self.consume_token(&Token::AtSign)
Review Comment:
We can use a method on the dialect trait to gate the new functionalities as
its preferred over the
[dialect_of](https://github.com/apache/datafusion-sqlparser-rs/blob/69bb0d824b360e60c358aad1367d0d897d0c5fea/src/dialect/mod.rs#L63-L68)
macro.
here e.g. a `self.dialect.supports_user_host_grantee()` or similar method?
##########
tests/sqlparser_mysql.rs:
##########
@@ -2971,3 +2971,133 @@ fn parse_bitstring_literal() {
))]
);
}
+
+#[test]
+fn parse_grant() {
+ let sql = "GRANT ALL ON *.* TO 'jeffrey'@'%'";
+ assert_eq!(
+ mysql().verified_stmt(sql),
+ Statement::Grant {
+ privileges: Privileges::All {
+ with_privileges_keyword: false
+ },
+ objects: GrantObjects::Tables(vec![ObjectName(vec!["*".into(),
"*".into()])]),
+ grantees: vec![Grantee::UserHost {
+ user: Ident {
+ value: "jeffrey".to_owned(),
+ quote_style: Some('\'')
+ },
+ host: Ident {
+ value: "%".to_owned(),
+ quote_style: Some('\'')
+ }
+ }],
+ with_grant_option: false,
+ granted_by: None
+ }
+ )
+}
+
+#[test]
+fn parse_revoke() {
+ let sql = "REVOKE ALL ON db1.* FROM 'jeffrey'@'%'";
+ assert_eq!(
+ mysql().verified_stmt(sql),
+ Statement::Revoke {
+ privileges: Privileges::All {
+ with_privileges_keyword: false
+ },
+ objects: GrantObjects::Tables(vec![ObjectName(vec!["db1".into(),
"*".into()])]),
+ grantees: vec![Grantee::UserHost {
+ user: Ident {
+ value: "jeffrey".to_owned(),
+ quote_style: Some('\'')
+ },
+ host: Ident {
+ value: "%".to_owned(),
+ quote_style: Some('\'')
+ }
+ }],
+ granted_by: None,
+ cascade: None,
+ }
+ )
+}
+
+#[test]
+fn parse_create_view_algorithm_param() {
+ let sql = "CREATE ALGORITHM = MERGE VIEW foo AS SELECT 1";
+ let stmt = mysql().verified_stmt(sql);
+ if let Statement::CreateView {
+ params:
+ Some(MySQLViewParams {
+ algorithm,
+ definer,
+ security,
+ }),
+ ..
+ } = stmt
+ {
+ assert_eq!(algorithm, Some(MySQLViewAlgorithm::Merge));
+ assert!(definer.is_none());
+ assert!(security.is_none());
+ } else {
+ unreachable!()
+ }
+}
+
+#[test]
+fn parse_create_view_definer_param() {
+ let sql = "CREATE DEFINER = 'jeffrey'@'localhost' VIEW foo AS SELECT 1";
+ let stmt = mysql().verified_stmt(sql);
+ if let Statement::CreateView {
+ params:
+ Some(MySQLViewParams {
+ algorithm,
+ definer,
+ security,
+ }),
+ ..
+ } = stmt
+ {
+ assert!(algorithm.is_none());
+ assert_eq!(
+ definer,
+ Some(Grantee::UserHost {
+ user: Ident {
+ value: "jeffrey".to_owned(),
+ quote_style: Some('\'')
+ },
+ host: Ident {
+ value: "localhost".to_owned(),
+ quote_style: Some('\'')
+ },
+ })
+ );
+ assert!(security.is_none());
+ } else {
+ unreachable!()
+ }
+}
+
+#[test]
+fn parse_create_view_security_param() {
+ let sql = "CREATE SQL SECURITY DEFINER VIEW foo AS SELECT 1";
+ let stmt = mysql().verified_stmt(sql);
Review Comment:
similarly, we could add one scenario for invoker? e.g.
```rust
mysql().verified_stmt("CREATE SQL SECURITY INVOKER VIEW foo AS SELECT
1");
```
##########
src/parser/mod.rs:
##########
@@ -11023,23 +11103,43 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_grantee(&mut self) -> Result<Grantee, ParserError> {
+ let user = self.parse_identifier(false)?;
+ if dialect_of!(self is MySqlDialect | GenericDialect) &&
self.consume_token(&Token::AtSign)
+ {
+ let host = self.parse_identifier(false)?;
+ Ok(Grantee::UserHost { user, host })
+ } else {
+ Ok(Grantee::Ident(user))
+ }
+ }
+
/// Parse a REVOKE statement
pub fn parse_revoke(&mut self) -> Result<Statement, ParserError> {
let (privileges, objects) =
self.parse_grant_revoke_privileges_objects()?;
self.expect_keyword(Keyword::FROM)?;
- let grantees = self.parse_comma_separated(|p|
p.parse_identifier(false))?;
+ let grantees = self.parse_comma_separated(|p| p.parse_grantee())?;
let granted_by = self
.parse_keywords(&[Keyword::GRANTED, Keyword::BY])
.then(|| self.parse_identifier(false).unwrap());
let loc = self.peek_token().location;
- let cascade = self.parse_keyword(Keyword::CASCADE);
- let restrict = self.parse_keyword(Keyword::RESTRICT);
- if cascade && restrict {
- return parser_err!("Cannot specify both CASCADE and RESTRICT in
REVOKE", loc);
- }
+ let cascade = if !dialect_of!(self is MySqlDialect) {
Review Comment:
Can we change the representation and parsing of cascade/restrict to
hopefully simplify the code and make the intent explicit, thinking instead of
using a bool, to represent it in the statement we can use the existing cascade
option?
`Statement::Revoke{ cascade: Option<TruncateCascadeOption> }`. That would
also let us reuse the current parser code for that enum
##########
src/ast/mod.rs:
##########
@@ -7311,15 +7345,84 @@ pub enum MySQLColumnPosition {
impl Display for MySQLColumnPosition {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
- MySQLColumnPosition::First => Ok(write!(f, "FIRST")?),
+ MySQLColumnPosition::First => write!(f, "FIRST"),
MySQLColumnPosition::After(ident) => {
let column_name = &ident.value;
- Ok(write!(f, "AFTER {column_name}")?)
+ write!(f, "AFTER {column_name}")
}
}
}
}
+/// MySQL `CREATE VIEW` algorithm parameter: [ALGORITHM = {UNDEFINED | MERGE |
TEMPTABLE}]
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum MySQLViewAlgorithm {
+ Undefined,
+ Merge,
+ TempTable,
+}
+
+impl Display for MySQLViewAlgorithm {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ MySQLViewAlgorithm::Undefined => write!(f, "UNDEFINED"),
+ MySQLViewAlgorithm::Merge => write!(f, "MERGE"),
+ MySQLViewAlgorithm::TempTable => write!(f, "TEMPTABLE"),
+ }
+ }
+}
+/// MySQL `CREATE VIEW` security parameter: [SQL SECURITY { DEFINER | INVOKER
}]
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum MySQLViewSecurity {
+ Definer,
+ Invoker,
+}
+
+impl Display for MySQLViewSecurity {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ match self {
+ MySQLViewSecurity::Definer => write!(f, "DEFINER"),
+ MySQLViewSecurity::Invoker => write!(f, "INVOKER"),
+ }
+ }
+}
+
+/// [MySQL] `CREATE VIEW` additional parameters
+///
+/// [MySQL]: https://dev.mysql.com/doc/refman/9.1/en/create-view.html
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub struct MySQLViewParams {
Review Comment:
```suggestion
pub struct CreateViewParams {
```
Re naming of the structs/enums we can probably use the statement type as the
prefix instead of the dialect, to make it possible for the struct/enum to be
extended to cover other dialects if required in the future. e.g.
`CreateViewAlgorithm` etc
##########
src/parser/mod.rs:
##########
@@ -8353,20 +8409,40 @@ impl<'a> Parser<'a> {
}
}
+ /// Parse a possibly qualified, possibly quoted identifier, optionally
allowing for wildcards,
+ /// e.g. *, `foo`.*, or "foo"."bar"
+ pub fn parse_object_name_with_wildcards(
Review Comment:
The method sounds like what `parse_wildcard_expr` does, minus the return
type. Can we reuse that functionality instead? e.g. if we move that logic out
to its own method so that `parse_wildcard_expr` and other functionality like
grant etc can call it directly?
##########
tests/sqlparser_mysql.rs:
##########
@@ -2971,3 +2971,133 @@ fn parse_bitstring_literal() {
))]
);
}
+
+#[test]
+fn parse_grant() {
+ let sql = "GRANT ALL ON *.* TO 'jeffrey'@'%'";
+ assert_eq!(
+ mysql().verified_stmt(sql),
+ Statement::Grant {
+ privileges: Privileges::All {
+ with_privileges_keyword: false
+ },
+ objects: GrantObjects::Tables(vec![ObjectName(vec!["*".into(),
"*".into()])]),
+ grantees: vec![Grantee::UserHost {
+ user: Ident {
+ value: "jeffrey".to_owned(),
+ quote_style: Some('\'')
+ },
+ host: Ident {
+ value: "%".to_owned(),
+ quote_style: Some('\'')
+ }
+ }],
+ with_grant_option: false,
+ granted_by: None
+ }
+ )
+}
+
+#[test]
+fn parse_revoke() {
+ let sql = "REVOKE ALL ON db1.* FROM 'jeffrey'@'%'";
+ assert_eq!(
+ mysql().verified_stmt(sql),
+ Statement::Revoke {
+ privileges: Privileges::All {
+ with_privileges_keyword: false
+ },
+ objects: GrantObjects::Tables(vec![ObjectName(vec!["db1".into(),
"*".into()])]),
+ grantees: vec![Grantee::UserHost {
+ user: Ident {
+ value: "jeffrey".to_owned(),
+ quote_style: Some('\'')
+ },
+ host: Ident {
+ value: "%".to_owned(),
+ quote_style: Some('\'')
+ }
+ }],
+ granted_by: None,
+ cascade: None,
+ }
+ )
+}
+
+#[test]
+fn parse_create_view_algorithm_param() {
Review Comment:
Also for the create view, can we add a test case showing the behavior on
multiple params in the same create statement?
##########
tests/sqlparser_mysql.rs:
##########
@@ -2971,3 +2971,133 @@ fn parse_bitstring_literal() {
))]
);
}
+
+#[test]
+fn parse_grant() {
+ let sql = "GRANT ALL ON *.* TO 'jeffrey'@'%'";
+ assert_eq!(
+ mysql().verified_stmt(sql),
+ Statement::Grant {
+ privileges: Privileges::All {
+ with_privileges_keyword: false
+ },
+ objects: GrantObjects::Tables(vec![ObjectName(vec!["*".into(),
"*".into()])]),
+ grantees: vec![Grantee::UserHost {
+ user: Ident {
+ value: "jeffrey".to_owned(),
+ quote_style: Some('\'')
+ },
+ host: Ident {
+ value: "%".to_owned(),
+ quote_style: Some('\'')
+ }
+ }],
+ with_grant_option: false,
+ granted_by: None
+ }
+ )
+}
+
+#[test]
+fn parse_revoke() {
+ let sql = "REVOKE ALL ON db1.* FROM 'jeffrey'@'%'";
+ assert_eq!(
+ mysql().verified_stmt(sql),
+ Statement::Revoke {
+ privileges: Privileges::All {
+ with_privileges_keyword: false
+ },
+ objects: GrantObjects::Tables(vec![ObjectName(vec!["db1".into(),
"*".into()])]),
+ grantees: vec![Grantee::UserHost {
+ user: Ident {
+ value: "jeffrey".to_owned(),
+ quote_style: Some('\'')
+ },
+ host: Ident {
+ value: "%".to_owned(),
+ quote_style: Some('\'')
+ }
+ }],
+ granted_by: None,
+ cascade: None,
+ }
+ )
+}
+
+#[test]
+fn parse_create_view_algorithm_param() {
+ let sql = "CREATE ALGORITHM = MERGE VIEW foo AS SELECT 1";
+ let stmt = mysql().verified_stmt(sql);
Review Comment:
can we add a couple scenarios for the other variants to get coverage? since
we already verified the ast, they can be simpler roundtrip tests. e.g.
```rust
mysql().verified_stmt("CREATE ALGORITHM = UNDEFINED VIEW foo AS SELECT 1");
mysql().verified_stmt("CREATE ALGORITHM = TEMPTABLE VIEW foo AS SELECT 1");
```
##########
tests/sqlparser_mysql.rs:
##########
@@ -2971,3 +2971,133 @@ fn parse_bitstring_literal() {
))]
);
}
+
+#[test]
+fn parse_grant() {
+ let sql = "GRANT ALL ON *.* TO 'jeffrey'@'%'";
+ assert_eq!(
+ mysql().verified_stmt(sql),
+ Statement::Grant {
+ privileges: Privileges::All {
+ with_privileges_keyword: false
+ },
+ objects: GrantObjects::Tables(vec![ObjectName(vec!["*".into(),
"*".into()])]),
+ grantees: vec![Grantee::UserHost {
+ user: Ident {
+ value: "jeffrey".to_owned(),
+ quote_style: Some('\'')
+ },
+ host: Ident {
+ value: "%".to_owned(),
+ quote_style: Some('\'')
+ }
+ }],
+ with_grant_option: false,
+ granted_by: None
+ }
+ )
+}
+
+#[test]
+fn parse_revoke() {
+ let sql = "REVOKE ALL ON db1.* FROM 'jeffrey'@'%'";
+ assert_eq!(
+ mysql().verified_stmt(sql),
Review Comment:
```suggestion
mysql_and_generic().verified_stmt(sql),
```
if that's applicable here, we can include generic in the cases to cover the
features it supports
--
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]
