iffyio commented on code in PR #1727:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1727#discussion_r1957667202
##########
tests/sqlparser_postgres.rs:
##########
@@ -5320,6 +5320,117 @@ fn parse_create_type_as_enum() {
}
}
+#[test]
+fn parse_alter_type() {
+ struct TestCase {
+ sql: &'static str,
+ canonical: &'static str,
+ name: &'static str,
+ operation: AlterTypeOperation,
+ }
+ vec![
+ TestCase {
+ sql: r#"ALTER TYPE public.my_type
+ RENAME TO my_new_type"#,
+ canonical: "ALTER TYPE public.my_type RENAME TO my_new_type",
+ name: "public.my_type",
+ operation: AlterTypeOperation::Rename {
+ new_name: Ident::new("my_new_type"),
+ },
+ },
+ TestCase {
+ sql: r#"ALTER TYPE public.my_type
+ ADD VALUE IF NOT EXISTS 'label3.5'
+ BEFORE 'label4'"#,
+ canonical:
+ "ALTER TYPE public.my_type ADD VALUE IF NOT EXISTS 'label3.5'
BEFORE 'label4'",
+ name: "public.my_type",
+ operation: AlterTypeOperation::AddValue {
+ if_not_exists: true,
+ value: Ident::with_quote('\'', "label3.5"),
+ position:
AlterTypeOperationAddValuePosition::Before(Ident::with_quote(
+ '\'', "label4",
+ )),
+ },
+ },
+ TestCase {
+ sql: r#"ALTER TYPE public.my_type
+ ADD VALUE 'label3.5' BEFORE 'label4'"#,
+ canonical: "ALTER TYPE public.my_type ADD VALUE 'label3.5' BEFORE
'label4'",
+ name: "public.my_type",
+ operation: AlterTypeOperation::AddValue {
+ if_not_exists: false,
+ value: Ident::with_quote('\'', "label3.5"),
+ position:
AlterTypeOperationAddValuePosition::Before(Ident::with_quote(
+ '\'', "label4",
+ )),
+ },
+ },
+ TestCase {
+ sql: r#"ALTER TYPE public.my_type
+ ADD VALUE IF NOT EXISTS 'label3.5'
+ AFTER 'label3'"#,
+ canonical:
+ "ALTER TYPE public.my_type ADD VALUE IF NOT EXISTS 'label3.5'
AFTER 'label3'",
+ name: "public.my_type",
+ operation: AlterTypeOperation::AddValue {
+ if_not_exists: true,
+ value: Ident::with_quote('\'', "label3.5"),
+ position:
AlterTypeOperationAddValuePosition::After(Ident::with_quote(
+ '\'', "label3",
+ )),
+ },
+ },
+ TestCase {
+ sql: r#"ALTER TYPE public.my_type
+ ADD VALUE 'label3.5'
+ AFTER 'label3'"#,
+ canonical: "ALTER TYPE public.my_type ADD VALUE 'label3.5' AFTER
'label3'",
+ name: "public.my_type",
+ operation: AlterTypeOperation::AddValue {
+ if_not_exists: false,
+ value: Ident::with_quote('\'', "label3.5"),
+ position:
AlterTypeOperationAddValuePosition::After(Ident::with_quote(
+ '\'', "label3",
+ )),
+ },
+ },
+ TestCase {
+ sql: r#"ALTER TYPE public.my_type
+ ADD VALUE IF NOT EXISTS 'label5'"#,
+ canonical: "ALTER TYPE public.my_type ADD VALUE IF NOT EXISTS
'label5'",
+ name: "public.my_type",
+ operation: AlterTypeOperation::AddValue {
+ if_not_exists: true,
+ value: Ident::with_quote('\'', "label5"),
+ position: AlterTypeOperationAddValuePosition::Default,
+ },
+ },
+ TestCase {
+ sql: r#"ALTER TYPE public.my_type
+ ADD VALUE 'label5'"#,
+ canonical: "ALTER TYPE public.my_type ADD VALUE 'label5'",
+ name: "public.my_type",
+ operation: AlterTypeOperation::AddValue {
+ if_not_exists: false,
+ value: Ident::with_quote('\'', "label5"),
+ position: AlterTypeOperationAddValuePosition::Default,
+ },
+ },
+ ]
+ .into_iter()
+ .enumerate()
+ .for_each(|(index, tc)| {
+ let statement = pg_and_generic().one_statement_parses_to(tc.sql,
tc.canonical);
Review Comment:
```suggestion
let statement = pg_and_generic().verified_stmt(tc.sql);
```
can we use verified_stmt instead?
##########
src/parser/mod.rs:
##########
@@ -14258,6 +14316,21 @@ impl<'a> Parser<'a> {
})
}
+ // See
[PostgreSQL](https://www.postgresql.org/docs/current/sql-createtype.html)
Review Comment:
```suggestion
/// See
[PostgreSQL](https://www.postgresql.org/docs/current/sql-createtype.html)
```
Can we also add a short description what the function does?
##########
src/ast/ddl.rs:
##########
@@ -640,6 +640,68 @@ impl fmt::Display for AlterIndexOperation {
}
}
+/// An `ALTER TYPE` (`Statement::AlterType`) operation
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum AlterTypeOperation {
+ Rename {
+ new_name: Ident,
+ },
+ AddValue {
+ if_not_exists: bool,
+ value: Ident,
+ position: AlterTypeOperationAddValuePosition,
+ },
+ RenameValue {
+ from: Ident,
+ to: Ident,
+ },
+}
+
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum AlterTypeOperationAddValuePosition {
Review Comment:
```suggestion
pub enum AlterTypeAddValuePosition {
```
maybe an opportunity to shorten the name, since AddValue is already an
operation
##########
src/ast/mod.rs:
##########
@@ -2691,6 +2692,14 @@ pub enum Statement {
with_options: Vec<SqlOption>,
},
/// ```sql
+ /// ALTER TYPE
+ /// See
[PostgreSQL](https://www.postgresql.org/docs/current/sql-altertype.html)
+ /// ```
+ AlterType {
+ name: ObjectName,
+ operation: AlterTypeOperation,
+ },
Review Comment:
Can we replace the anonymous enum variants (this and `AlterTypeOperation`
with structs? we're slowly [moving a way from using this
representation](https://github.com/apache/datafusion-sqlparser-rs/issues/1204)
##########
src/parser/mod.rs:
##########
@@ -14258,6 +14316,21 @@ impl<'a> Parser<'a> {
})
}
+ // See
[PostgreSQL](https://www.postgresql.org/docs/current/sql-createtype.html)
+ pub fn parse_create_type_enum(&mut self, name: ObjectName) ->
Result<Statement, ParserError> {
+ if !self.consume_token(&Token::LParen) {
+ return self.expected("'(' after CREATE TYPE AS ENUM",
self.peek_token());
+ }
Review Comment:
can we use `self.expect_token()` here?
##########
src/ast/ddl.rs:
##########
@@ -640,6 +640,68 @@ impl fmt::Display for AlterIndexOperation {
}
}
+/// An `ALTER TYPE` (`Statement::AlterType`) operation
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum AlterTypeOperation {
+ Rename {
+ new_name: Ident,
+ },
+ AddValue {
+ if_not_exists: bool,
+ value: Ident,
+ position: AlterTypeOperationAddValuePosition,
Review Comment:
I think in order to follow convention we can make this field optional in
order to remove the `Default` position variant?
##########
src/parser/mod.rs:
##########
@@ -8122,6 +8127,55 @@ impl<'a> Parser<'a> {
})
}
+ pub fn parse_alter_type(&mut self) -> Result<Statement, ParserError> {
+ let name = self.parse_object_name(false)?;
+
+ if self.parse_keywords(&[Keyword::RENAME, Keyword::TO]) {
+ let new_name = self.parse_identifier()?;
+ Ok(Statement::AlterType {
+ name,
+ operation: AlterTypeOperation::Rename { new_name },
+ })
+ } else if self.parse_keywords(&[Keyword::ADD, Keyword::VALUE]) {
+ let if_not_exists = self.parse_keywords(&[Keyword::IF,
Keyword::NOT, Keyword::EXISTS]);
+ let new_enum_value = self.parse_identifier()?;
+ let position = if self.parse_keyword(Keyword::BEFORE) {
+
AlterTypeOperationAddValuePosition::Before(self.parse_identifier()?)
+ } else if self.parse_keyword(Keyword::AFTER) {
+
AlterTypeOperationAddValuePosition::After(self.parse_identifier()?)
+ } else {
+ AlterTypeOperationAddValuePosition::Default
+ };
+
+ Ok(Statement::AlterType {
+ name,
+ operation: AlterTypeOperation::AddValue {
+ if_not_exists,
+ value: new_enum_value,
+ position,
+ },
+ })
+ } else if self.parse_keywords(&[Keyword::RENAME, Keyword::VALUE]) {
+ let existing_enum_value = self.parse_identifier()?;
+ self.expect_keyword(Keyword::TO)?;
+ let new_enum_value = self.parse_identifier()?;
+
+ Ok(Statement::AlterType {
+ name,
+ operation: AlterTypeOperation::RenameValue {
+ from: existing_enum_value,
+ to: new_enum_value,
+ },
+ })
+ } else {
+ self.expected_ref(
+ "{RENAME TO | { RENAME | ADD } VALUE}",
+ self.peek_token_ref(),
+ )?;
+ unreachable!()
Review Comment:
```suggestion
)
```
not sure I follow the use of unreachable here, i imagine returning the error
should suffice? (we would want to avoid the panic in any case)
##########
src/dialect/mod.rs:
##########
@@ -490,6 +490,26 @@ pub trait Dialect: Debug + Any {
false
}
+ /// Return true if the dialect supports creating enum types
+ ///
+ /// Example:
+ /// ```sql
+ /// CREATE TYPE some_enum AS ENUM('one', 'two', 'three');
+ /// ```
+ fn supports_create_type_as_enum(&self) -> bool {
+ false
+ }
+
+ /// Return true if the dialect supports ALTER TYPE statements
+ ///
+ /// Example:
+ /// ```sql
+ /// ALTER TYPE some_enum ADD VALUE 'some_value';
+ /// ```
+ fn supports_alter_type(&self) -> bool {
+ false
+ }
Review Comment:
Thinking we could skip the dialect flags for this case and have the parse
the statement unconditionally, if it does not conflict with existing statements?
##########
src/parser/mod.rs:
##########
@@ -8122,6 +8127,55 @@ impl<'a> Parser<'a> {
})
}
+ pub fn parse_alter_type(&mut self) -> Result<Statement, ParserError> {
Review Comment:
```suggestion
/// Parse a [Statement::AlterType].
pub fn parse_alter_type(&mut self) -> Result<Statement, ParserError> {
```
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
