fix(clickhouse): parse MODIFY COLUMN as Alter / ModifyColumn - #8020
fix(clickhouse): parse MODIFY COLUMN as Alter / ModifyColumn#8020dgvj-work wants to merge 2 commits into
Conversation
ClickHouse MODIFY COLUMN was falling through to Command because the MODIFY alter parser only handled SQL SECURITY properties. Parse the native column form into ModifyColumn and enable generation for it. Fixes tobymao#8019
Only emit IF EXISTS for MODIFY COLUMN, and cover the REMOVE fallback.
| if properties := self._parse_properties(): | ||
| return self.expression(exp.AlterModifySqlSecurity(expressions=properties.expressions)) | ||
| return None | ||
|
|
There was a problem hiding this comment.
Thanks for the PR!
This kind of a confusing situation, given the differences in behavior across dialects and current implementation.
Existing behavior
The existing implementation is exclusively intended to support ALTER TABLE MODIFY SQL SECURITY.
It's parsed by _parse_alter_table_modify, which was ok before but is a return type issue now that we're expanding the coverage.
Return exp type
In Clickhouse, ALTER COLUMN and MODIFY COLUMN are semantically equivalent, so we should return an AlterColumn exp here. Snowflake has the same semantics - you can see it mapping keyword MODIFY to the ALTER parser here.
(Only in MySQL do ALTER COLUMN and MODIFY COLUMN have different semantics, which is why the ModifyColumn exp class exists.)
Different approach
Let's switch approaches to override the base _parse_alter_table_alter.
Note that this means sqlglot will canonicalize MODIFY COLUMN to ALTER COLUMN during generation.
|
|
||
| ALTER_PARSERS = { | ||
| **parser.Parser.ALTER_PARSERS, | ||
| "MODIFY": lambda self: self._parse_alter_table_modify(), |
There was a problem hiding this comment.
| "MODIFY": lambda self: self._parse_alter_table_alter(), |
| @@ -911,7 +911,29 @@ | |||
| def _parse_alter_table_modify(self) -> exp.Expr | None: | |||
There was a problem hiding this comment.
| def _parse_alter_table_modify(self) -> exp.Expr | None: | |
| def _parse_alter_table_alter(self) -> exp.Expr | None: |
| @@ -911,7 +911,29 @@ | |||
| def _parse_alter_table_modify(self) -> exp.Expr | None: | |||
| if properties := self._parse_properties(): | |||
There was a problem hiding this comment.
Gate this to MODIFY SQL SECURITY since ALTER can now get here
if self._prev.text.upper() == "MODIFY" and self._match(
TokenType.SQL_SECURITY, advance=False
):
if properties := self._parse_properties():
return self.expression(
exp.AlterModifySqlSecurity(expressions=properties.expressions)
)
| # https://clickhouse.com/docs/sql-reference/statements/alter/column#modify-column | ||
| if not self._match(TokenType.COLUMN): | ||
| return None | ||
|
|
||
| exists = self._parse_exists() | ||
| column = self._parse_field(any_token=True) | ||
| if column is None: | ||
| return None | ||
|
|
||
| # REMOVE / MODIFY SETTING / RESET SETTING / ADD ENUM VALUES stay as Command | ||
| if ( | ||
| self._match_texts(("REMOVE", "RESET"), advance=False) | ||
| or self._match_text_seq("MODIFY", "SETTING", advance=False) | ||
| or self._match_text_seq("ADD", "ENUM", advance=False) | ||
| ): | ||
| return None | ||
|
|
||
| column_def = self._parse_column_def(column) | ||
| if not isinstance(column_def, exp.ColumnDef): | ||
| return None | ||
|
|
||
| return self.expression(exp.ModifyColumn(this=column_def, exists=exists or None)) |
There was a problem hiding this comment.
Remove and rely on base parser - the return has it fall back to Command if there are still other tokens left to consume (like SETTINGS).
alter = super()._parse_alter_table_alter()
return None if self._curr else alter
|
|
||
| class ModifyColumn(Expression): | ||
| arg_types = {"this": True, "rename_from": False} | ||
| arg_types = {"this": True, "rename_from": False, "exists": False} |
There was a problem hiding this comment.
- Move arg to
AlterColumnclass above - Add
IF EXISTSparsing to base_parse_alter_table_alter - Add
SUPPORTS_ALTER_COLUMN_IF_EXISTS=Falseto base generator - Add
SUPPORTS_ALTER_COLUMN_IF_EXISTS=Trueto Clickhouse generator - Add generation to base generator, gated by
SUPPORTS_ALTER_COLUMN_IF_EXISTS - Annotate other dialects'
altercolumn_sqloverrides with@unsupported_args("exists")
| STRUCT_DELIMITER = ("(", ")") | ||
| NVL2_SUPPORTED = False | ||
| ALTER_SET_TYPE = "TYPE" | ||
| SUPPORTS_MODIFY_COLUMN = True |
| if not self.SUPPORTS_MODIFY_COLUMN: | ||
| self.unsupported("MODIFY COLUMN is not supported in this dialect") | ||
| return f"MODIFY COLUMN {this}" | ||
| exists = " IF EXISTS" if expression.args.get("exists") else "" |
2 participants
ClickHouse
ALTER TABLE ... MODIFY COLUMNwas falling back toCommandbecause the ClickHouseMODIFYalter parser only handledMODIFY SQL SECURITY ....This parses the column form into
ModifyColumn(same AST MySQL already uses), enables ClickHouse generation for it, and leavesREMOVE/ column settings /ADD ENUM VALUESasCommandfor now.Fixes #8019
Disclosure: used LLM assistance while investigating; I reviewed and tested the change.