PR feedback

Author: erindru

sqlmesh/core/config/root.py

@@ -148,7 +148,9 @@ class Config(BaseConfig):
     environment_suffix_target: EnvironmentSuffixTarget = Field(
         default=EnvironmentSuffixTarget.default
     )
-    physical_table_naming_convention: t.Optional[TableNamingConvention] = None
+    physical_table_naming_convention: TableNamingConvention = Field(
+        default=TableNamingConvention.default
+    )
     gateway_managed_virtual_layer: bool = False
     infer_python_dependencies: bool = True
     environment_catalog_mapping: RegexKeyDict = {}

sqlmesh/core/snapshot/definition.py

@@ -228,6 +228,7 @@ class SnapshotDataVersion(PydanticModel, frozen=True):
     change_category: t.Optional[SnapshotChangeCategory] = None
     physical_schema_: t.Optional[str] = Field(default=None, alias="physical_schema")
     dev_table_suffix: str
+    table_naming_convention: TableNamingConvention = Field(default=TableNamingConvention.default)
 
     def snapshot_id(self, name: str) -> SnapshotId:
         return SnapshotId(name=name, identifier=self.fingerprint.to_identifier())
@@ -334,7 +335,7 @@ class SnapshotInfoMixin(ModelKindMixin):
     # This can be removed from this model once Pydantic 1 support is dropped (must remain in `Snapshot` though)
     base_table_name_override: t.Optional[str]
     dev_table_suffix: str
-    table_naming_convention: t.Optional[TableNamingConvention] = None
+    table_naming_convention: TableNamingConvention = Field(default=TableNamingConvention.default)
 
     @cached_property
     def identifier(self) -> str:
@@ -609,8 +610,8 @@ class Snapshot(PydanticModel, SnapshotInfoMixin):
     base_table_name_override: t.Optional[str] = None
     next_auto_restatement_ts: t.Optional[int] = None
     dev_table_suffix: str = "dev"
-    table_naming_convention_: t.Optional[TableNamingConvention] = Field(
-        default=None, alias="table_naming_convention"
+    table_naming_convention_: TableNamingConvention = Field(
+        default=TableNamingConvention.default, alias="table_naming_convention"
     )
 
     @field_validator("ttl")
@@ -663,7 +664,7 @@ def from_node(
         ttl: str = c.DEFAULT_SNAPSHOT_TTL,
         version: t.Optional[str] = None,
         cache: t.Optional[t.Dict[str, SnapshotFingerprint]] = None,
-        table_naming_convention: t.Optional[TableNamingConvention] = None,
+        table_naming_convention: TableNamingConvention = TableNamingConvention.default,
     ) -> Snapshot:
         """Creates a new snapshot for a node.
 
@@ -1023,6 +1024,7 @@ def categorize_as(self, category: SnapshotChangeCategory) -> None:
             previous_version = self.previous_version
             self.version = previous_version.data_version.version
             self.physical_schema_ = previous_version.physical_schema
+            self.table_naming_convention = previous_version.table_naming_convention
             if self.is_materialized and (category.is_indirect_non_breaking or category.is_metadata):
                 # Reuse the dev table for indirect non-breaking changes.
                 self.dev_version_ = (
@@ -1229,6 +1231,7 @@ def data_version(self) -> SnapshotDataVersion:
             change_category=self.change_category,
             physical_schema=self.physical_schema,
             dev_table_suffix=self.dev_table_suffix,
+            table_naming_convention=self.table_naming_convention,
         )
 
     @property

tests/core/test_config.py

@@ -1419,7 +1419,7 @@ def test_load_yaml_config_custom_dotenv_path(tmp_path_factory):
 @pytest.mark.parametrize(
     "convention_str, expected",
     [
-        (None, None),
+        (None, TableNamingConvention.SCHEMA_AND_TABLE),
         ("schema_and_table", TableNamingConvention.SCHEMA_AND_TABLE),
         ("table_only", TableNamingConvention.TABLE_ONLY),
         ("hash_md5", TableNamingConvention.HASH_MD5),

tests/core/test_snapshot.py

@@ -166,6 +166,7 @@ def test_json(snapshot: Snapshot):
         "name": '"name"',
         "parents": [{"name": '"parent"."tbl"', "identifier": snapshot.parents[0].identifier}],
         "previous_versions": [],
+        "table_naming_convention": "schema_and_table",
         "updated_ts": 1663891973000,
         "version": snapshot.fingerprint.to_version(),
         "migrated": False,
@@ -1140,6 +1141,9 @@ def test_snapshot_table_name(snapshot: Snapshot, make_snapshot: t.Callable):
         data_hash="1", metadata_hash="1", parent_data_hash="1"
     )
     snapshot.categorize_as(SnapshotChangeCategory.BREAKING)
+    assert snapshot.table_naming_convention == TableNamingConvention.SCHEMA_AND_TABLE
+    assert snapshot.data_version.table_naming_convention == TableNamingConvention.SCHEMA_AND_TABLE
+
     snapshot.previous_versions = ()
     assert snapshot.table_name(is_deployable=True) == "sqlmesh__default.name__3078928823"
     assert snapshot.table_name(is_deployable=False) == "sqlmesh__default.name__3078928823__dev"
@@ -1196,6 +1200,8 @@ def test_table_name_naming_convention_table_only(make_snapshot: t.Callable[...,
         table_naming_convention=TableNamingConvention.TABLE_ONLY,
     )
     snapshot.categorize_as(SnapshotChangeCategory.BREAKING)
+    assert snapshot.table_naming_convention == TableNamingConvention.TABLE_ONLY
+    assert snapshot.data_version.table_naming_convention == TableNamingConvention.TABLE_ONLY
 
     assert snapshot.table_name(is_deployable=True) == f"foo.sqlmesh__bar.baz__{snapshot.version}"
     assert (
@@ -1220,6 +1226,8 @@ def test_table_name_naming_convention_hash_md5(make_snapshot: t.Callable[..., Sn
         table_naming_convention=TableNamingConvention.HASH_MD5,
     )
     snapshot.categorize_as(SnapshotChangeCategory.BREAKING)
+    assert snapshot.table_naming_convention == TableNamingConvention.HASH_MD5
+    assert snapshot.data_version.table_naming_convention == TableNamingConvention.HASH_MD5
 
     hash = md5(f"foo.sqlmesh__bar.bar__baz__{snapshot.version}")
     assert snapshot.table_name(is_deployable=True) == f"foo.sqlmesh__bar.sqlmesh_md5__{hash}"
@@ -1273,6 +1281,36 @@ def test_table_name_view(make_snapshot: t.Callable):
     assert new_snapshot.dev_version != snapshot.dev_version
 
 
+def test_table_naming_convention_change_reuse_previous_version(make_snapshot):
+    # Ensure that snapshots that trigger "reuse previous version" inherit the naming convention of the previous snapshot
+    original_snapshot: Snapshot = make_snapshot(
+        SqlModel(name="a", query=parse_one("select 1, ds")),
+        table_naming_convention=TableNamingConvention.SCHEMA_AND_TABLE,
+    )
+    original_snapshot.categorize_as(SnapshotChangeCategory.BREAKING)
+
+    assert original_snapshot.table_naming_convention == TableNamingConvention.SCHEMA_AND_TABLE
+    assert original_snapshot.table_name() == "sqlmesh__default.a__4145234055"
+
+    changed_snapshot: Snapshot = make_snapshot(
+        SqlModel(name="a", query=parse_one("select 1, 'forward_only' as a, ds")),
+        table_naming_convention=TableNamingConvention.HASH_MD5,
+    )
+    changed_snapshot.previous_versions = original_snapshot.all_versions
+
+    assert changed_snapshot.previous_version == original_snapshot.data_version
+
+    changed_snapshot.categorize_as(SnapshotChangeCategory.FORWARD_ONLY)
+
+    # inherited from previous version even though changed_snapshot was created with TableNamingConvention.HASH_MD5
+    assert changed_snapshot.table_naming_convention == TableNamingConvention.SCHEMA_AND_TABLE
+    assert (
+        changed_snapshot.previous_version.table_naming_convention
+        == TableNamingConvention.SCHEMA_AND_TABLE
+    )
+    assert changed_snapshot.table_name() == "sqlmesh__default.a__4145234055"
+
+
 def test_categorize_change_sql(make_snapshot):
     old_snapshot = make_snapshot(SqlModel(name="a", query=parse_one("select 1, ds")))