You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "LucaCappelletti94 (via GitHub)" <gi...@apache.org> on 2026/10/20 11:08:07 UTC
Re: [PR] MSSQL: Support multiple columns after ALTER TABLE ADD [datafusion-sqlparser-rs]
LucaCappelletti94 commented on code in PR #2449:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2449#discussion_r4324258532
##########
src/parser/mod.rs:
##########
@@ -11506,7 +11561,18 @@ impl<'a> Parser<'a> {
let only = self.parse_keyword(Keyword::ONLY); // [ ONLY ]
let table_name = self.parse_object_name(false)?;
let on_cluster = self.parse_optional_on_cluster()?;
- let operations = self.parse_comma_separated(Parser::parse_alter_table_operation)?;
+ let mut operations = vec![];
+ loop {
+ let operation = if self.can_parse_implicit_alter_table_add_item(&operations) {
+ self.parse_alter_table_add_item()?
Review Comment:
Would preserving the shared `ADD` clause in the AST be better here?
On `dev/mssql-alter-table-add-columns` at `2f1a810`, `ALTER TABLE t ADD a INT, b INT` renders as `ALTER TABLE t ADD a INT, ADD b INT`, which violates [SQL Server's grammar](https://learn.microsoft.com/en-us/sql/t-sql/statements/alter-table-transact-sql?view=sql-server-ver17#syntax-for-disk-based-tables). Constraints acquire the same extra `ADD`, and reparsing with `sqlparser` accepts that invalid output.
##########
src/parser/mod.rs:
##########
@@ -11172,6 +11124,109 @@ impl<'a> Parser<'a> {
Ok(operation)
}
+ fn parse_alter_table_add_operation(&mut self) -> Result<AlterTableOperation, ParserError> {
+ if let Some(constraint) = self.parse_optional_table_constraint()? {
+ let not_valid = self.parse_keywords(&[Keyword::NOT, Keyword::VALID]);
+ return Ok(AlterTableOperation::AddConstraint {
+ constraint,
+ not_valid,
+ });
+ }
+
+ if dialect_of!(self is ClickHouseDialect | GenericDialect)
+ && self.parse_keyword(Keyword::PROJECTION)
+ {
+ return self.parse_alter_table_add_projection();
+ }
+
+ let if_not_exists = self.parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]);
+ let mut new_partitions = vec![];
+ while self.parse_keyword(Keyword::PARTITION) {
+ new_partitions.push(self.parse_partition()?);
+ }
+ if !new_partitions.is_empty() {
+ return Ok(AlterTableOperation::AddPartitions {
+ if_not_exists,
+ new_partitions,
+ });
+ }
+
+ self.parse_alter_table_add_column(if_not_exists)
+ }
+
+ fn parse_alter_table_add_column(
+ &mut self,
+ if_not_exists: bool,
+ ) -> Result<AlterTableOperation, ParserError> {
+ let column_keyword = self.parse_keyword(Keyword::COLUMN);
+ let if_not_exists = if dialect_of!(self is PostgreSqlDialect | BigQueryDialect | DuckDbDialect | GenericDialect)
+ {
+ self.parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]) || if_not_exists
+ } else {
+ false
+ };
+ let column_def = self.parse_column_def()?;
+ let column_position = self.parse_column_position()?;
+
+ Ok(AlterTableOperation::AddColumn {
+ column_keyword,
+ if_not_exists,
+ column_def,
+ column_position,
+ })
+ }
+
+ fn parse_alter_table_add_item(&mut self) -> Result<AlterTableOperation, ParserError> {
+ if let Some(constraint) = self.parse_optional_table_constraint()? {
+ let not_valid = self.parse_keywords(&[Keyword::NOT, Keyword::VALID]);
+ return Ok(AlterTableOperation::AddConstraint {
+ constraint,
+ not_valid,
+ });
+ }
+
+ self.parse_alter_table_add_column(false)
+ }
+
+ fn can_parse_implicit_alter_table_add_item(&self, operations: &[AlterTableOperation]) -> bool {
+ self.dialect.supports_alter_table_add_multiple_columns()
+ && matches!(
+ operations.last(),
+ Some(
+ AlterTableOperation::AddColumn { .. }
+ | AlterTableOperation::AddConstraint { .. }
+ )
+ )
+ && !self.is_alter_table_operation_starter()
+ }
+
+ fn is_alter_table_operation_starter(&self) -> bool {
+ if self
+ .peek_one_of_keywords(&[
+ Keyword::ADD,
+ Keyword::ALTER,
+ Keyword::DISABLE,
+ Keyword::DROP,
+ Keyword::ENABLE,
+ Keyword::PARTITION,
+ Keyword::RENAME,
+ Keyword::REPLICA,
+ Keyword::SET,
+ Keyword::SWAP,
+ ])
+ .is_some()
+ {
+ return true;
+ }
+
+ matches!(
+ &self.peek_token_ref().token,
+ Token::Word(word)
+ if word.quote_style.is_none()
+ && matches!(word.value.to_ascii_uppercase().as_str(), "NOCHECK" | "SWITCH")
+ )
+ }
Review Comment:
You could recognize complete operation prefixes so nonreserved column names remain usable in the new list syntax. On `dev/mssql-alter-table-add-columns` at `2f1a810`, `ALTER TABLE t ADD a INT, replica INT` fails at `replica`, and `swap`, `rename`, `disable`, and `enable` fail similarly, although each single-column form parses.
```suggestion
fn is_alter_table_operation_starter(&self) -> bool {
let keyword = |n| match &self.peek_nth_token_ref(n).token {
Token::Word(word) if word.quote_style.is_none() => word.keyword,
_ => Keyword::NoKeyword,
};
match keyword(0) {
Keyword::ADD | Keyword::ALTER | Keyword::DROP | Keyword::SET => true,
Keyword::DISABLE => {
matches!(keyword(1), Keyword::ROW | Keyword::RULE | Keyword::TRIGGER)
}
Keyword::ENABLE => matches!(
keyword(1),
Keyword::ALWAYS
| Keyword::ROW
| Keyword::REPLICA
| Keyword::RULE
| Keyword::TRIGGER
),
Keyword::PARTITION => true,
Keyword::RENAME => {
matches!(keyword(1), Keyword::CONSTRAINT | Keyword::TO | Keyword::AS)
|| keyword(2) == Keyword::TO
|| (keyword(1) == Keyword::COLUMN && keyword(3) == Keyword::TO)
}
Keyword::REPLICA => keyword(1) == Keyword::IDENTITY,
Keyword::SWAP => keyword(1) == Keyword::WITH,
_ => false,
}
}
```
--
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