Repository: TobikoData/sqlmesh
Pull request #3099 (2024-09-05 23:49:47)

Fix!: Dont clone managed tables to create dev previews
Prior to this change, making a forward-only change to a managed model only worked if you did it via a dev environment first.

Applying it directly to prod resulted in the following error on Snowflake: `SQL compilation error: Invalid operation Cannot clone from an dynamic table.` because in order to create the dev preview table (which [always gets created](https://github.com/TobikoData/sqlmesh/issues/2496) regardless of the environment), SQLMesh triggers a clone of the current prod table.

This PR does two things:
 - Allows the evaluation strategy to decide if `clone()` is suitable for creating dev preview tables
 - Implements `migrate()` for the `EngineManagedStrategy` now that I better understand the conditions that can trigger `migrate()` in the first place.

The usual function of `migrate()` is to mutate the schema of an existing table to match a new definition without re-creating that table (which can be expensive). However:

- Managed models are more like views in that you cant just mutate the schema, you have to replace the entire query backing the model
- So, if the new model definition results in schema changes, we throw an error because it has to be rebuilt which defeats the purpose of foward-only if we were to treat it as forward-only
- If there are no schema changes (eg an unrelated macro that doesn't affect the model schema got updated), `migrate()` is a no-op


Patch:
diff --git a/sqlmesh/core/snapshot/evaluator.py b/sqlmesh/core/snapshot/evaluator.py
index af956dcb..b72e6b3b 100644
--- a/sqlmesh/core/snapshot/evaluator.py
+++ b/sqlmesh/core/snapshot/evaluator.py
@@ -667,6 +667,8 @@ class SnapshotEvaluator:
                 and snapshot.is_materialized
                 and snapshot.previous_versions
                 and self.adapter.SUPPORTS_CLONING
+                # managed models cannot have their schema mutated because theyre based on queries, so clone + alter wont work
+                and not snapshot.is_managed
             ):
                 target_table_name = snapshot.table_name(is_deployable=False)
                 tmp_table_name = f"{target_table_name}__schema_migration_source"
@@ -1786,9 +1788,14 @@ class EngineManagedStrategy(MaterializableStrategy):
         snapshot: Snapshot,
         **kwargs: t.Any,
     ) -> None:
-        # Not entirely true, many engines support modifying some of the metadata fields on a managed table
-        # eg Snowflake allows you to ALTER DYNAMIC TABLE foo SET WAREHOUSE=my_other_wh;
-        raise ConfigError(f"Cannot mutate managed table: {target_table_name}")
+        potential_alter_expressions = self.adapter.get_alter_expressions(
+            target_table_name, source_table_name
+        )
+        if len(potential_alter_expressions) > 0:
+            # this can happen if a user changes a managed model and deliberately overrides a plan to be forward only, eg `sqlmesh plan --forward-only`
+            raise SQLMeshError(
+                f"The schema of the managed model '{target_table_name}' cannot be updated in a forward-only fashion."
+            )
 
     def delete(self, name: str, **kwargs: t.Any) -> None:
         # a dev preview table is created as a normal table, so it needs to be dropped as a normal table


Test patch:
diff --git a/tests/core/test_snapshot_evaluator.py b/tests/core/test_snapshot_evaluator.py
index 5440f9b8..e923f98a 100644
--- a/tests/core/test_snapshot_evaluator.py
+++ b/tests/core/test_snapshot_evaluator.py
@@ -42,6 +42,8 @@ from sqlmesh.core.snapshot import (
     DeployabilityIndex,
     Intervals,
     Snapshot,
+    SnapshotDataVersion,
+    SnapshotFingerprint,
     SnapshotChangeCategory,
     SnapshotEvaluator,
     SnapshotTableCleanupTask,
@@ -49,7 +51,7 @@ from sqlmesh.core.snapshot import (
 from sqlmesh.core.snapshot.evaluator import CustomMaterialization
 from sqlmesh.utils.concurrency import NodeExecutionFailedError
 from sqlmesh.utils.date import to_timestamp
-from sqlmesh.utils.errors import AuditError, ConfigError
+from sqlmesh.utils.errors import AuditError, ConfigError, SQLMeshError
 from sqlmesh.utils.metaprogramming import Executable
 
 
@@ -2520,3 +2522,87 @@ def test_cleanup_managed(adapter_mock, make_snapshot, mocker: MockerFixture):
     adapter_mock.drop_managed_table.assert_called_once_with(
         "sqlmesh__test_schema.test_schema__test_model__1556851963"
     )
+
+
+def test_create_managed_forward_only_with_previous_version_doesnt_clone_for_dev_preview(
+    adapter_mock, make_snapshot
+):
+    evaluator = SnapshotEvaluator(adapter_mock)
+
+    model = load_sql_based_model(
+        parse(  # type: ignore
+            """
+            MODEL (
+                name test_schema.test_model,
+                kind MANAGED
+            );
+
+            select a, b from foo;
+            """
+        )
+    )
+
+    snapshot: Snapshot = make_snapshot(model)
+    snapshot.categorize_as(SnapshotChangeCategory.FORWARD_ONLY)
+    snapshot.previous_versions = (
+        SnapshotDataVersion(
+            fingerprint=SnapshotFingerprint(
+                data_hash="test_data_hash",
+                metadata_hash="test_metadata_hash",
+            ),
+            version="test_version",
+            change_category=SnapshotChangeCategory.FORWARD_ONLY,
+        ),
+    )
+
+    evaluator.create(target_snapshots=[snapshot], snapshots={})
+
+    # We dont clone managed tables to create dev previews, we use normal tables
+    adapter_mock.clone_table.assert_not_called()
+    adapter_mock.create_managed_table.assert_not_called()
+    adapter_mock.create_table.assert_not_called()
+
+    # The table gets created using ctas() because the model column types arent known
+    adapter_mock.ctas.assert_called_once()
+    assert adapter_mock.ctas.call_args_list[0].args[0] == snapshot.table_name(is_deployable=False)
+
+
+def test_migrate_managed(adapter_mock, make_snapshot, mocker: MockerFixture):
+    evaluator = SnapshotEvaluator(adapter_mock)
+
+    model = load_sql_based_model(
+        parse(  # type: ignore
+            """
+            MODEL (
+                name test_schema.test_model,
+                kind MANAGED
+            );
+
+            select a, b from foo;
+            """
+        )
+    )
+    snapshot: Snapshot = make_snapshot(model)
+    snapshot.categorize_as(SnapshotChangeCategory.FORWARD_ONLY)
+
+    # no schema changes - no-op
+    adapter_mock.get_alter_expressions.return_value = []
+    evaluator.migrate(target_snapshots=[snapshot], snapshots={})
+
+    adapter_mock.create_table.assert_not_called()
+    adapter_mock.create_managed_table.assert_not_called()
+    adapter_mock.ctas.assert_not_called()
+
+    # schema changes - exception thrown
+    adapter_mock.get_alter_expressions.return_value = [exp.Alter()]
+
+    with pytest.raises(NodeExecutionFailedError, match=r".*Execution failed for node*") as exc_info:
+        evaluator.migrate(target_snapshots=[snapshot], snapshots={})
+
+    cause = exc_info.value.__cause__
+    assert isinstance(cause, SQLMeshError)
+    assert "cannot be updated" in str(cause)
+
+    adapter_mock.create_table.assert_not_called()
+    adapter_mock.ctas.assert_not_called()
+    adapter_mock.create_managed_table.assert_not_called()