From f73a330bd3b7b88b3f05e31bb06dfd627545d9e5 Mon Sep 17 00:00:00 2001 From: Yoav Cohen Date: Sun, 29 Jun 2025 15:32:57 +0200 Subject: [PATCH 1/4] Add support for ALTER COLUMN TYPE in Redshift --- src/ast/ddl.rs | 18 ++++++++++++++---- src/ast/spans.rs | 1 + src/parser/mod.rs | 28 ++++++++++++++++++---------- tests/sqlparser_common.rs | 19 +------------------ tests/sqlparser_postgres.rs | 6 ++---- tests/sqlparser_redshift.rs | 5 +++++ 6 files changed, 41 insertions(+), 36 deletions(-) diff --git a/src/ast/ddl.rs b/src/ast/ddl.rs index afe9f228c..6f29d74ec 100644 --- a/src/ast/ddl.rs +++ b/src/ast/ddl.rs @@ -893,7 +893,10 @@ pub enum AlterColumnOperation { data_type: DataType, /// PostgreSQL specific using: Option, + /// Whether the statement included the optional `SET DATA` keywords + had_set: bool, }, + /// `ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( sequence_options ) ]` /// /// Note: this is a PostgreSQL-specific operation. @@ -914,12 +917,19 @@ impl fmt::Display for AlterColumnOperation { AlterColumnOperation::DropDefault => { write!(f, "DROP DEFAULT") } - AlterColumnOperation::SetDataType { data_type, using } => { + AlterColumnOperation::SetDataType { + data_type, + using, + had_set, + } => { + if *had_set { + write!(f, "SET DATA ")?; + } + write!(f, "TYPE {data_type}")?; if let Some(expr) = using { - write!(f, "SET DATA TYPE {data_type} USING {expr}") - } else { - write!(f, "SET DATA TYPE {data_type}") + write!(f, " USING {expr}")?; } + Ok(()) } AlterColumnOperation::AddGenerated { generated_as, diff --git a/src/ast/spans.rs b/src/ast/spans.rs index 78ed772bd..00882602c 100644 --- a/src/ast/spans.rs +++ b/src/ast/spans.rs @@ -924,6 +924,7 @@ impl Spanned for AlterColumnOperation { AlterColumnOperation::SetDataType { data_type: _, using, + had_set: _, } => using.as_ref().map_or(Span::empty(), |u| u.span()), AlterColumnOperation::AddGenerated { .. } => Span::empty(), } diff --git a/src/parser/mod.rs b/src/parser/mod.rs index a079488f8..824cb861e 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -8734,16 +8734,10 @@ impl<'a> Parser<'a> { } } else if self.parse_keywords(&[Keyword::DROP, Keyword::DEFAULT]) { AlterColumnOperation::DropDefault {} - } else if self.parse_keywords(&[Keyword::SET, Keyword::DATA, Keyword::TYPE]) - || (is_postgresql && self.parse_keyword(Keyword::TYPE)) - { - let data_type = self.parse_data_type()?; - let using = if is_postgresql && self.parse_keyword(Keyword::USING) { - Some(self.parse_expr()?) - } else { - None - }; - AlterColumnOperation::SetDataType { data_type, using } + } else if self.parse_keywords(&[Keyword::SET, Keyword::DATA, Keyword::TYPE]) { + self.parse_set_data_type(true)? + } else if self.parse_keyword(Keyword::TYPE) { + self.parse_set_data_type(false)? } else if self.parse_keywords(&[Keyword::ADD, Keyword::GENERATED]) { let generated_as = if self.parse_keyword(Keyword::ALWAYS) { Some(GeneratedAs::Always) @@ -8909,6 +8903,20 @@ impl<'a> Parser<'a> { Ok(operation) } + fn parse_set_data_type(&mut self, had_set: bool) -> Result { + let data_type = self.parse_data_type()?; + let using = if self.parse_keyword(Keyword::USING) { + Some(self.parse_expr()?) + } else { + None + }; + Ok(AlterColumnOperation::SetDataType { + data_type, + using, + had_set, + }) + } + fn parse_part_or_partition(&mut self) -> Result { let keyword = self.expect_one_of_keywords(&[Keyword::PART, Keyword::PARTITION])?; match keyword { diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index f6f51459c..40e5ef5d9 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -5046,7 +5046,6 @@ fn parse_alter_table_alter_column() { #[test] fn parse_alter_table_alter_column_type() { - let alter_stmt = "ALTER TABLE tab"; match alter_table_op(verified_stmt( "ALTER TABLE tab ALTER COLUMN is_active SET DATA TYPE TEXT", )) { @@ -5057,28 +5056,12 @@ fn parse_alter_table_alter_column_type() { AlterColumnOperation::SetDataType { data_type: DataType::Text, using: None, + had_set: true, } ); } _ => unreachable!(), } - - let dialect = TestedDialects::new(vec![Box::new(GenericDialect {})]); - - let res = - dialect.parse_sql_statements(&format!("{alter_stmt} ALTER COLUMN is_active TYPE TEXT")); - assert_eq!( - ParserError::ParserError("Expected: SET/DROP NOT NULL, SET DEFAULT, or SET DATA TYPE after ALTER COLUMN, found: TYPE".to_string()), - res.unwrap_err() - ); - - let res = dialect.parse_sql_statements(&format!( - "{alter_stmt} ALTER COLUMN is_active SET DATA TYPE TEXT USING 'text'" - )); - assert_eq!( - ParserError::ParserError("Expected: end of statement, found: USING".to_string()), - res.unwrap_err() - ); } #[test] diff --git a/tests/sqlparser_postgres.rs b/tests/sqlparser_postgres.rs index 7b0a8c5da..e1a49c69c 100644 --- a/tests/sqlparser_postgres.rs +++ b/tests/sqlparser_postgres.rs @@ -764,10 +764,7 @@ fn parse_drop_extension() { #[test] fn parse_alter_table_alter_column() { - pg().one_statement_parses_to( - "ALTER TABLE tab ALTER COLUMN is_active TYPE TEXT USING 'text'", - "ALTER TABLE tab ALTER COLUMN is_active SET DATA TYPE TEXT USING 'text'", - ); + pg().verified_stmt("ALTER TABLE tab ALTER COLUMN is_active TYPE TEXT USING 'text'"); match alter_table_op( pg().verified_stmt( @@ -783,6 +780,7 @@ fn parse_alter_table_alter_column() { AlterColumnOperation::SetDataType { data_type: DataType::Text, using: Some(using_expr), + had_set: true, } ); } diff --git a/tests/sqlparser_redshift.rs b/tests/sqlparser_redshift.rs index be2b67223..d7df678ea 100644 --- a/tests/sqlparser_redshift.rs +++ b/tests/sqlparser_redshift.rs @@ -402,3 +402,8 @@ fn parse_extract_single_quotes() { fn parse_string_literal_backslash_escape() { redshift().one_statement_parses_to(r#"SELECT 'l\'auto'"#, "SELECT 'l''auto'"); } + +#[test] +fn test_alter_column_type() { + redshift().verified_stmt("ALTER TABLE customers ALTER COLUMN email TYPE TEXT"); +} From 6b9925b71349e0898710a6b144dba22d41831bce Mon Sep 17 00:00:00 2001 From: Yoav Cohen Date: Sun, 29 Jun 2025 18:19:59 +0200 Subject: [PATCH 2/4] Add dialect options --- src/dialect/mod.rs | 16 ++++++++++++++++ src/dialect/postgresql.rs | 8 ++++++++ src/dialect/redshift.rs | 4 ++++ src/parser/mod.rs | 8 ++++++-- tests/sqlparser_common.rs | 18 ++++++++++++++++++ 5 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/dialect/mod.rs b/src/dialect/mod.rs index 028aa58a7..354f134cf 100644 --- a/src/dialect/mod.rs +++ b/src/dialect/mod.rs @@ -1060,6 +1060,22 @@ pub trait Dialect: Debug + Any { fn supports_space_separated_column_options(&self) -> bool { false } + + /// Returns true if the dialect supports `ALTER TABLE tbl ALTER COLUMN col TYPE ` + /// without specifying `SET DATA` before `TYPE`. + /// + /// - [Redshift](https://docs.aws.amazon.com/redshift/latest/dg/r_ALTER_TABLE.html#r_ALTER_TABLE-synopsis) + /// - [PostgreSQL](https://www.postgresql.org/docs/current/sql-altertable.html) + fn supports_alter_column_type_without_set(&self) -> bool { + false + } + + /// Returns true if the dialect supports `ALTER TABLE tbl ALTER COLUMN col SET DATA TYPE USING ` + /// + /// - [PostgreSQL](https://www.postgresql.org/docs/current/sql-altertable.html) + fn supports_alter_column_type_using(&self) -> bool { + false + } } /// This represents the operators for which precedence must be defined diff --git a/src/dialect/postgresql.rs b/src/dialect/postgresql.rs index a91ab598b..d156e9941 100644 --- a/src/dialect/postgresql.rs +++ b/src/dialect/postgresql.rs @@ -258,4 +258,12 @@ impl Dialect for PostgreSqlDialect { fn supports_set_names(&self) -> bool { true } + + fn supports_alter_column_type_without_set(&self) -> bool { + true + } + + fn supports_alter_column_type_using(&self) -> bool { + true + } } diff --git a/src/dialect/redshift.rs b/src/dialect/redshift.rs index feccca5dd..b3eb78984 100644 --- a/src/dialect/redshift.rs +++ b/src/dialect/redshift.rs @@ -129,4 +129,8 @@ impl Dialect for RedshiftSqlDialect { fn supports_string_literal_backslash_escape(&self) -> bool { true } + + fn supports_alter_column_type_without_set(&self) -> bool { + true + } } diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 824cb861e..f6b92aac1 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -8736,7 +8736,9 @@ impl<'a> Parser<'a> { AlterColumnOperation::DropDefault {} } else if self.parse_keywords(&[Keyword::SET, Keyword::DATA, Keyword::TYPE]) { self.parse_set_data_type(true)? - } else if self.parse_keyword(Keyword::TYPE) { + } else if self.dialect.supports_alter_column_type_without_set() + && self.parse_keyword(Keyword::TYPE) + { self.parse_set_data_type(false)? } else if self.parse_keywords(&[Keyword::ADD, Keyword::GENERATED]) { let generated_as = if self.parse_keyword(Keyword::ALWAYS) { @@ -8905,7 +8907,9 @@ impl<'a> Parser<'a> { fn parse_set_data_type(&mut self, had_set: bool) -> Result { let data_type = self.parse_data_type()?; - let using = if self.parse_keyword(Keyword::USING) { + let using = if self.dialect.supports_alter_column_type_using() + && self.parse_keyword(Keyword::USING) + { Some(self.parse_expr()?) } else { None diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 40e5ef5d9..408e6402d 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -5046,6 +5046,7 @@ fn parse_alter_table_alter_column() { #[test] fn parse_alter_table_alter_column_type() { + let alter_stmt = "ALTER TABLE tab"; match alter_table_op(verified_stmt( "ALTER TABLE tab ALTER COLUMN is_active SET DATA TYPE TEXT", )) { @@ -5062,6 +5063,23 @@ fn parse_alter_table_alter_column_type() { } _ => unreachable!(), } + + let dialects = all_dialects_except(|d| d.supports_alter_column_type_without_set()); + let res = + dialects.parse_sql_statements(&format!("{alter_stmt} ALTER COLUMN is_active TYPE TEXT")); + assert_eq!( + ParserError::ParserError("Expected: SET/DROP NOT NULL, SET DEFAULT, or SET DATA TYPE after ALTER COLUMN, found: TYPE".to_string()), + res.unwrap_err() + ); + + let dialects = all_dialects_except(|d| d.supports_alter_column_type_using()); + let res = dialects.parse_sql_statements(&format!( + "{alter_stmt} ALTER COLUMN is_active SET DATA TYPE TEXT USING 'text'" + )); + assert_eq!( + ParserError::ParserError("Expected: end of statement, found: USING".to_string()), + res.unwrap_err() + ); } #[test] From 16639bc108a6523f87111c6baf54d0bfc19a25ec Mon Sep 17 00:00:00 2001 From: Yoav Cohen Date: Sun, 29 Jun 2025 18:26:46 +0200 Subject: [PATCH 3/4] Generalize positive tests --- tests/sqlparser_common.rs | 8 ++++++++ tests/sqlparser_redshift.rs | 5 ----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 408e6402d..379e8140e 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -5064,6 +5064,9 @@ fn parse_alter_table_alter_column_type() { _ => unreachable!(), } + let dialects = all_dialects_where(|d| d.supports_alter_column_type_without_set()); + dialects.verified_stmt(&format!("{alter_stmt} ALTER COLUMN is_active TYPE TEXT")); + let dialects = all_dialects_except(|d| d.supports_alter_column_type_without_set()); let res = dialects.parse_sql_statements(&format!("{alter_stmt} ALTER COLUMN is_active TYPE TEXT")); @@ -5072,6 +5075,11 @@ fn parse_alter_table_alter_column_type() { res.unwrap_err() ); + let dialects = all_dialects_where(|d| d.supports_alter_column_type_using()); + dialects.verified_stmt(&format!( + "{alter_stmt} ALTER COLUMN is_active SET DATA TYPE TEXT USING 'text'" + )); + let dialects = all_dialects_except(|d| d.supports_alter_column_type_using()); let res = dialects.parse_sql_statements(&format!( "{alter_stmt} ALTER COLUMN is_active SET DATA TYPE TEXT USING 'text'" diff --git a/tests/sqlparser_redshift.rs b/tests/sqlparser_redshift.rs index d7df678ea..be2b67223 100644 --- a/tests/sqlparser_redshift.rs +++ b/tests/sqlparser_redshift.rs @@ -402,8 +402,3 @@ fn parse_extract_single_quotes() { fn parse_string_literal_backslash_escape() { redshift().one_statement_parses_to(r#"SELECT 'l\'auto'"#, "SELECT 'l''auto'"); } - -#[test] -fn test_alter_column_type() { - redshift().verified_stmt("ALTER TABLE customers ALTER COLUMN email TYPE TEXT"); -} From d76f5ddaeccf38eb214a502722cf341e6b47d4da Mon Sep 17 00:00:00 2001 From: Yoav Cohen Date: Tue, 1 Jul 2025 14:35:44 +0200 Subject: [PATCH 4/4] Code review comments --- src/ast/ddl.rs | 2 +- src/dialect/mod.rs | 17 +++++------------ src/dialect/postgresql.rs | 4 ---- src/dialect/redshift.rs | 4 ---- src/parser/mod.rs | 4 +--- tests/sqlparser_common.rs | 12 +----------- 6 files changed, 8 insertions(+), 35 deletions(-) diff --git a/src/ast/ddl.rs b/src/ast/ddl.rs index 6f29d74ec..9134412c4 100644 --- a/src/ast/ddl.rs +++ b/src/ast/ddl.rs @@ -893,7 +893,7 @@ pub enum AlterColumnOperation { data_type: DataType, /// PostgreSQL specific using: Option, - /// Whether the statement included the optional `SET DATA` keywords + /// Set to true if the statement includes the `SET DATA TYPE` keywords had_set: bool, }, diff --git a/src/dialect/mod.rs b/src/dialect/mod.rs index 354f134cf..b214699cf 100644 --- a/src/dialect/mod.rs +++ b/src/dialect/mod.rs @@ -1061,18 +1061,11 @@ pub trait Dialect: Debug + Any { false } - /// Returns true if the dialect supports `ALTER TABLE tbl ALTER COLUMN col TYPE ` - /// without specifying `SET DATA` before `TYPE`. - /// - /// - [Redshift](https://docs.aws.amazon.com/redshift/latest/dg/r_ALTER_TABLE.html#r_ALTER_TABLE-synopsis) - /// - [PostgreSQL](https://www.postgresql.org/docs/current/sql-altertable.html) - fn supports_alter_column_type_without_set(&self) -> bool { - false - } - - /// Returns true if the dialect supports `ALTER TABLE tbl ALTER COLUMN col SET DATA TYPE USING ` - /// - /// - [PostgreSQL](https://www.postgresql.org/docs/current/sql-altertable.html) + /// Returns true if the dialect supports the `USING` clause in an `ALTER COLUMN` statement. + /// Example: + /// ```sql + /// ALTER TABLE tbl ALTER COLUMN col SET DATA TYPE USING ` + /// ``` fn supports_alter_column_type_using(&self) -> bool { false } diff --git a/src/dialect/postgresql.rs b/src/dialect/postgresql.rs index d156e9941..b2d4014cb 100644 --- a/src/dialect/postgresql.rs +++ b/src/dialect/postgresql.rs @@ -259,10 +259,6 @@ impl Dialect for PostgreSqlDialect { true } - fn supports_alter_column_type_without_set(&self) -> bool { - true - } - fn supports_alter_column_type_using(&self) -> bool { true } diff --git a/src/dialect/redshift.rs b/src/dialect/redshift.rs index b3eb78984..feccca5dd 100644 --- a/src/dialect/redshift.rs +++ b/src/dialect/redshift.rs @@ -129,8 +129,4 @@ impl Dialect for RedshiftSqlDialect { fn supports_string_literal_backslash_escape(&self) -> bool { true } - - fn supports_alter_column_type_without_set(&self) -> bool { - true - } } diff --git a/src/parser/mod.rs b/src/parser/mod.rs index f6b92aac1..4e0009c78 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -8736,9 +8736,7 @@ impl<'a> Parser<'a> { AlterColumnOperation::DropDefault {} } else if self.parse_keywords(&[Keyword::SET, Keyword::DATA, Keyword::TYPE]) { self.parse_set_data_type(true)? - } else if self.dialect.supports_alter_column_type_without_set() - && self.parse_keyword(Keyword::TYPE) - { + } else if self.parse_keyword(Keyword::TYPE) { self.parse_set_data_type(false)? } else if self.parse_keywords(&[Keyword::ADD, Keyword::GENERATED]) { let generated_as = if self.parse_keyword(Keyword::ALWAYS) { diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 379e8140e..75776f5d0 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -5063,17 +5063,7 @@ fn parse_alter_table_alter_column_type() { } _ => unreachable!(), } - - let dialects = all_dialects_where(|d| d.supports_alter_column_type_without_set()); - dialects.verified_stmt(&format!("{alter_stmt} ALTER COLUMN is_active TYPE TEXT")); - - let dialects = all_dialects_except(|d| d.supports_alter_column_type_without_set()); - let res = - dialects.parse_sql_statements(&format!("{alter_stmt} ALTER COLUMN is_active TYPE TEXT")); - assert_eq!( - ParserError::ParserError("Expected: SET/DROP NOT NULL, SET DEFAULT, or SET DATA TYPE after ALTER COLUMN, found: TYPE".to_string()), - res.unwrap_err() - ); + verified_stmt(&format!("{alter_stmt} ALTER COLUMN is_active TYPE TEXT")); let dialects = all_dialects_where(|d| d.supports_alter_column_type_using()); dialects.verified_stmt(&format!(