iffyio commented on code in PR #1616:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1616#discussion_r1898470965
##########
src/parser/mod.rs:
##########
@@ -1044,6 +1046,16 @@ impl<'a> Parser<'a> {
Ok(Statement::NOTIFY { channel, payload })
}
+ pub fn parse_rename(&mut self) -> Result<Statement, ParserError> {
Review Comment:
```suggestion
/// Parses a `RENAME TABLE` statement. See [Statement::Rename]
pub fn parse_rename(&mut self) -> Result<Statement, ParserError> {
```
thinking we can add documentation especially since this is a public api
##########
src/parser/mod.rs:
##########
@@ -6682,6 +6694,14 @@ impl<'a> Parser<'a> {
Ok(Tag::new(name, value))
}
+ pub(crate) fn parse_rename_object_def(&mut self) ->
Result<RenameObjectDef, ParserError> {
Review Comment:
```suggestion
fn parse_rename_object_def(&mut self) -> Result<RenameObjectDef,
ParserError> {
```
maybe better since this method is small and only used once, we can inline it
into the callsite instead of having it as a standalone method? e.g.
```rust
let operations = self.parse_comma_separated(|p| {
let old_name = self.parse_object_name(false)?;
self.expect_keyword(Keyword::TO)?;
let new_name = self.parse_object_name(false)?;
Ok((old_name, new_name))
})?;
```
##########
src/parser/mod.rs:
##########
@@ -1044,6 +1046,16 @@ impl<'a> Parser<'a> {
Ok(Statement::NOTIFY { channel, payload })
}
+ pub fn parse_rename(&mut self) -> Result<Statement, ParserError> {
+ if self.dialect.supports_rename_table() &&
self.peek_keyword(Keyword::TABLE) {
+ self.expect_keyword(Keyword::TABLE)?;
+ let operations =
self.parse_comma_separated(Parser::parse_rename_object_def)?;
+ Ok(Statement::RenameTable { operations })
+ } else {
+ self.expected("an object type after RENAME", self.peek_token())
Review Comment:
does it make sense to say e.g. 'table name' instead of 'object type'?
Thinking the object type/def terminology in the sounds a bit generic if we're
only dealing with tables in the context of this statement
##########
tests/sqlparser_common.rs:
##########
@@ -4026,6 +4026,120 @@ fn parse_alter_table() {
}
}
+#[test]
+fn parse_rename_table() {
+ let dialects = all_dialects_where(|d| d.supports_rename_table());
+
+ match dialects.verified_stmt("RENAME TABLE `test`.`test1` TO
`test_db`.`test2`") {
+ Statement::RenameTable { operations } => {
+ assert_eq!(
+ vec![RenameObjectDef {
+ old_name: ObjectName(vec![
+ Ident {
+ value: "test".to_string(),
+ quote_style: Some('`'),
+ span: Span::empty(),
+ },
Review Comment:
in order to shorten the test case can we replace occurrences of `Ident
{...}` with `Ident::new()` and `Ident::with_quote()` where applicable?
##########
src/ast/mod.rs:
##########
@@ -3401,6 +3401,13 @@ pub enum Statement {
partitioned: Option<Vec<Expr>>,
table_format: Option<HiveLoadDataFormat>,
},
+ /// ```sql
+ /// Rename TABLE tbl_name TO new_tbl_name[, tbl_name2 TO new_tbl_name2] ...
+ /// ```
+ /// renames one or more tables
Review Comment:
```suggestion
/// Renames one or more tables
```
##########
src/ast/mod.rs:
##########
@@ -3401,6 +3401,13 @@ pub enum Statement {
partitioned: Option<Vec<Expr>>,
table_format: Option<HiveLoadDataFormat>,
},
+ /// ```sql
+ /// Rename TABLE tbl_name TO new_tbl_name[, tbl_name2 TO new_tbl_name2] ...
+ /// ```
+ /// renames one or more tables
+ ///
+ /// See Mysql <https://dev.mysql.com/doc/refman/9.1/en/rename-table.html>
+ RenameTable { operations: Vec<RenameObjectDef> },
Review Comment:
Could we do something like?
```rust
struct RenameTable {
old_table: ObjectName,
new_table: ObjectName
}
Statement::RenameTable(RenameTable)
```
using a dedicated struct makes it easier to extend (e.g with spans later on)
and for downstream crates to work with the statement
##########
src/dialect/mod.rs:
##########
@@ -715,6 +715,11 @@ pub trait Dialect: Debug + Any {
true
}
+ // Returns true if the dialect supports the `RENAME TABLE` statement
+ fn supports_rename_table(&self) -> bool {
+ false
+ }
Review Comment:
I'm thinking it could be easier to skip the dialect method in this case and
have the parser always accept a rename table statement. since this is a full
statement I imagine its not bound to conflict with other dialects
--
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]
