You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "iffyio (via GitHub)" <gi...@apache.org> on 2025/07/01 11:36:50 UTC
Re: [PR] Redshift alter column type no set [datafusion-sqlparser-rs]
iffyio commented on code in PR #1912:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1912#discussion_r2177358792
##########
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 <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 {
Review Comment:
not sure I followed that this flag is necessary, it looks like we would be able to accept either variant with or without the `SET DATA` prefix? The error message of existing tests if I understood the PR description correctly we can change to match the new behavior I think
##########
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 <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 <type> USING <exp>`
Review Comment:
```suggestion
/// 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 <type> USING <exp>`
/// ```
```
thinking something like this so that its a bit clearer which part of the statement the flag refers to?
##########
src/ast/ddl.rs:
##########
@@ -893,7 +893,10 @@ pub enum AlterColumnOperation {
data_type: DataType,
/// PostgreSQL specific
using: Option<Expr>,
+ /// Whether the statement included the optional `SET DATA` keywords
+ had_set: bool,
Review Comment:
```suggestion
/// Set to true if the statement includes the `SET DATA TYPE` keywords
set_data_type: bool,
```
--
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