Fix/snowflake iceberg clone dialect error - #5722
Conversation
429eee9 to
9c0c155
Compare
|
@sineline It looks like @izeigerman provided feedback. Could you take a look? |
|
@StuffbyYuki I've been procrastinating too much on this. I'll see if I can pick it next week. Thanks for the nudge! |
…bles Snowflake rejects `CREATE TABLE ... CLONE` and `ALTER TABLE` for Iceberg tables, requiring `CREATE ICEBERG TABLE ... CLONE` and `ALTER ICEBERG TABLE` instead. The model's `table_format` was already honoured when creating tables but was never propagated to the clone and alter code paths, so both failed with a SQL compilation error during the virtual layer update and schema migration respectively. `clone_table` now accepts `table_format`/`table_kind` and `alter_table` accepts `table_format`, mirroring the existing `_create_table` convention. The Snowflake adapter derives the Iceberg-specific table kind from the format, and the evaluator passes the model's table format through both paths. Fixes SQLMesh#5721 Signed-off-by: Guillem G <guillem.gimenez@titanos.tv>
392f98e to
2f0404c
Compare
|
@izeigerman @StuffbyYuki sorry for the long delay. I've rebased this onto current 1. "any reason to make this change?" (the No — that was unrelated noise. Dropped entirely. 2. "why do we need to ignore this?" ( You were right on both, and they were really the same problem: the override didn't match the base signature, and the
The Snowflake overrides now match their supertypes exactly, so there's no That signature change meant updating the other adapters that override these methods — Tests Added Verification
Integration tests against a live Snowflake account I can't run here, so the generated DDL is verified at the unit level — the emitted statements are One scoping note: |
mypy does not require the assignment ignore on the patched columns lambda. Signed-off-by: Guillem G <guillem.gimenez@titanos.tv>
|
One adjacent gap I found while auditing the rest of the DDL paths, which I've deliberately left out of this PR — happy to fold it in here or split it into its own issue, whichever you prefer.
combined_sql = f"ALTER {table_kind} {table_sql} ALTER {', '.join(list_comment_sql)}"
It's narrower and less severe than the two cases fixed here:
I haven't confirmed the rejection against a live Snowflake account, so treat that as inferred from the error-message pattern rather than verified. For completeness, the other table DDL paths look correct as they stand and deliberately should not get the keyword:
I also checked that stamping the keyword across every expression in |
|
@izeigerman would you be so kind to take a look? |
This pull request introduces enhancements to support Iceberg table operations in Snowflake, ensuring correct DDL syntax and behavior for table creation, cloning, and schema alterations. The changes primarily address the unique requirements of Iceberg tables, such as proper handling of
PARTITION BYclauses and the use ofICEBERG TABLEsyntax in relevant commands. Additionally, the pull request propagates table format information throughout the snapshot evaluation and migration processes.Snowflake Iceberg Table Support:
_create_tableto correctly injectPARTITION BYclauses for Iceberg tables, ensuring compliance with Snowflake's ordering requirements and handling CTAS limitations.clone_tableto useCREATE ICEBERG TABLE ... CLONEsyntax when cloning Iceberg tables, by passingtable_kindbased on the table format.alter_tablemethod that usesALTER ICEBERG TABLEinstead ofALTER TABLEfor schema changes on Iceberg tables.Propagation of Table Format:
table_formatduring snapshot cloning and table migration, enabling downstream methods to select the correct DDL syntax. [1] [2]Base Adapter Improvements:
clone_tableandalter_tablesignatures in the base engine adapter to accept and propagatetable_kindandtable_formatparameters, allowing engine-specific logic to be triggered as needed. [1] [2]