Skip to content

fix: drop clustering key before dropping columns it references - #6095

Open
srujankgandla wants to merge 2 commits into
SQLMesh:mainfrom
srujankgandla:fix/5813-snowflake-clustering-key-order
Open

srujankgandla wants to merge 2 commits into
SQLMesh:mainfrom
srujankgandla:fix/5813-snowflake-clustering-key-order

Conversation

@srujankgandla

Copy link
Copy Markdown

Description

Fixes #5813.

When a model removes both a column and its clustered_by in the same edit, the non-breaking path clones the table (the clone inherits the clustering key) and then issues ALTERs. ClusteredByMixin.get_alter_operations appended the drop/replace clustering-key operation after the column alterations, so Snowflake received DROP COLUMN while the key still referenced it and rejected it with error 003203 — before the key was ever dropped.

This change sequences the clustering-key operation first, so the key is dropped (or replaced) before any column it references is altered. It also fixes the latent twin: changing the key while dropping a column referenced by the old key.

Added test_alter_table_drops_clustering_key_before_drop_column, which asserts the generated statement order on a mocked Snowflake adapter.

Test Plan

  • Added test_alter_table_drops_clustering_key_before_drop_column in
    tests/core/engine_adapter/test_snowflake.py: on a mocked Snowflake adapter
    it asserts the generated statements are
    ALTER TABLE "test_table" DROP CLUSTERING KEY followed by
    ALTER TABLE "test_table" DROP COLUMN "b".
  • python -m pytest tests/core/engine_adapter/test_snowflake.py -k "alter_table" -q → 2 passed.
  • Broader engine-adapter suites for all three ClusteredByMixin consumers
    (Snowflake, BigQuery, StarRocks): 219 passed, 1 skipped; the only 2 failures
    are pre-existing pandas-environment issues that fail identically on the clean tree.
  • ruff check and ruff format --check clean on both changed files.

Checklist

  • I have run make style and fixed any issues
  • I have added tests for my changes (if applicable)
  • All existing tests pass (make fast-test)
  • My commits are signed off (git commit -s) per the DCO

@nicksspirit

Copy link
Copy Markdown
Collaborator

Thanks for this! The fix handles the case in the issue nicely.

One catch: moving the clustering operation first also moves setting a new key first. So adding a column and clustering by it in the same change now fails on Snowflake:

ALTER TABLE t CLUSTER BY (c)  -- c doesn't exist yet
ALTER TABLE t ADD c INT

A possible fix:

  • Drop the old key first
  • Run the column changes
  • Set the new key last

Could you add tests so these cases all pass?

  • Drop the key and its column (the issue)
  • Shrink the key from (a, b) to (a) and drop b
  • Add c and cluster by c
  • Drop b, add c, and change the key from b to c

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-breaking plan fails when removed column is part of existing clustering key (Snowflake)

3 participants