Repository: TobikoData/sqlmesh
Pull request #2984 (2024-08-05 19:42:35)

fix: remove error when deleting expired external model snapshots
Prior to this change we always did a check on the table's database against the physical schema as a safety check to avoid deleting tables outside of where we expect to delete tables. This logic though assumes the following `evaluation_strategy.delete` call actually does a delete. This isn't true for symbolic evaluations. Therefore we had a bug where users could get errors when their symbolic models would expire and it would say it was going to delete outside of the scope of the physical db but that wasn't really the case since the actual delete was a no-op. 

Since the Evaluation Strategy is responsible for determining what deletes are actually happening, I push this check down to the strategy implementation layer. Now for deletes that do nothing there will not be a check and therefore there will no longer be false errors. 


Patch:
diff --git a/sqlmesh/core/snapshot/evaluator.py b/sqlmesh/core/snapshot/evaluator.py
index 386809ff..7426095d 100644
--- a/sqlmesh/core/snapshot/evaluator.py
+++ b/sqlmesh/core/snapshot/evaluator.py
@@ -799,12 +799,11 @@ class SnapshotEvaluator:
         evaluation_strategy = _evaluation_strategy(snapshot, self.adapter)
 
         for is_table_deployable, table_name in table_names:
-            table = exp.to_table(table_name)
-            if table.db != snapshot.physical_schema:
-                raise SQLMeshError(
-                    f"Table '{table_name}' is not a part of the physical schema '{snapshot.physical_schema}' and so can't be dropped."
-                )
-            evaluation_strategy.delete(table_name, is_table_deployable=is_table_deployable)
+            evaluation_strategy.delete(
+                table_name,
+                is_table_deployable=is_table_deployable,
+                physical_schema=snapshot.physical_schema,
+            )
 
             if on_complete is not None:
                 on_complete(table_name)
@@ -1225,6 +1224,7 @@ class MaterializableStrategy(PromotableStrategy):
         self.adapter.alter_table(alter_expressions)
 
     def delete(self, name: str, **kwargs: t.Any) -> None:
+        _check_table_db_is_physical_schema(name, kwargs["physical_schema"])
         self.adapter.drop_table(name)
         logger.info("Dropped table '%s'", name)
 
@@ -1792,6 +1792,7 @@ class EngineManagedStrategy(MaterializableStrategy):
 
     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
+        _check_table_db_is_physical_schema(name, kwargs["physical_schema"])
         if kwargs["is_table_deployable"]:
             self.adapter.drop_managed_table(name)
             logger.info("Dropped managed table '%s'", name)
@@ -1825,3 +1826,11 @@ def _check_destructive_schema_change(
         raise SQLMeshError(
             f"{warning_msg} To allow this, change the model's `on_destructive_change` setting to `warn` or `allow` or include it in the plan's `--allow-destructive-model` option."
         )
+
+
+def _check_table_db_is_physical_schema(table_name: str, physical_schema: str) -> None:
+    table = exp.to_table(table_name)
+    if table.db != physical_schema:
+        raise SQLMeshError(
+            f"Table '{table_name}' is not a part of the physical schema '{physical_schema}' and so can't be dropped."
+        )


Test patch:
diff --git a/tests/core/test_snapshot_evaluator.py b/tests/core/test_snapshot_evaluator.py
index 014effa0..cac20b35 100644
--- a/tests/core/test_snapshot_evaluator.py
+++ b/tests/core/test_snapshot_evaluator.py
@@ -32,8 +32,9 @@ from sqlmesh.core.model import (
     TimeColumn,
     ViewKind,
     load_sql_based_model,
+    ExternalModel,
 )
-from sqlmesh.core.model.kind import OnDestructiveChange
+from sqlmesh.core.model.kind import OnDestructiveChange, ExternalKind
 from sqlmesh.core.node import IntervalUnit
 from sqlmesh.core.snapshot import (
     DeployabilityIndex,
@@ -428,6 +429,29 @@ def test_cleanup(mocker: MockerFixture, adapter_mock, make_snapshot):
     )
 
 
+def test_cleanup_external_model(mocker: MockerFixture, adapter_mock, make_snapshot):
+    evaluator = SnapshotEvaluator(adapter_mock)
+
+    def create_and_cleanup_external_model(name: str, dev_table_only: bool):
+        model = ExternalModel(
+            name=name,
+            kind=ExternalKind(),
+        )
+
+        snapshot = make_snapshot(model)
+        snapshot.categorize_as(SnapshotChangeCategory.BREAKING)
+        snapshot.version = "test_version"
+
+        evaluator.promote([snapshot], EnvironmentNamingInfo(name="test_env"))
+        evaluator.cleanup(
+            [SnapshotTableCleanupTask(snapshot=snapshot.table_info, dev_table_only=dev_table_only)]
+        )
+        return snapshot
+
+    create_and_cleanup_external_model("catalog.test_schema.test_model", True)
+    adapter_mock.drop_table.assert_not_called()
+
+
 @pytest.mark.parametrize("view_exists", [True, False])
 def test_evaluate_materialized_view(
     mocker: MockerFixture, adapter_mock, make_snapshot, view_exists: bool