Repository: TobikoData/sqlmesh
Pull request #2820 (2024-06-25 21:02:44)

Fix: Make sure to render all model kind attributes
This greatly impacts the quality of the diffs that SQLMesh prints out when presenting a plan.


Patch:
diff --git a/sqlmesh/core/model/kind.py b/sqlmesh/core/model/kind.py
index ee05b336..b24ba134 100644
--- a/sqlmesh/core/model/kind.py
+++ b/sqlmesh/core/model/kind.py
@@ -193,7 +193,9 @@ def _on_destructive_change_validator(
     cls: t.Type, v: t.Union[OnDestructiveChange, str, exp.Identifier]
 ) -> t.Any:
     if v and not isinstance(v, OnDestructiveChange):
-        return OnDestructiveChange(v.this.upper() if isinstance(v, exp.Identifier) else v.upper())
+        return OnDestructiveChange(
+            v.this.upper() if isinstance(v, (exp.Identifier, exp.Literal)) else v.upper()
+        )
     return v
 
 
@@ -209,7 +211,10 @@ class _ModelKind(PydanticModel, ModelKindMixin):
     def model_kind_name(self) -> t.Optional[ModelKindName]:
         return self.name
 
-    def to_expression(self, **kwargs: t.Any) -> d.ModelKind:
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        kwargs["expressions"] = expressions
         return d.ModelKind(this=self.name.value.upper(), **kwargs)
 
     @property
@@ -326,6 +331,18 @@ class _Incremental(_ModelKind):
             str(self.on_destructive_change),
         ]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                _property(
+                    "on_destructive_change", exp.Literal.string(self.on_destructive_change.value)
+                ),
+            ],
+        )
+
 
 class _IncrementalBy(_Incremental):
     dialect: t.Optional[str] = Field(None, validate_default=True)
@@ -354,6 +371,24 @@ class _IncrementalBy(_Incremental):
             str(self.disable_restatement),
         ]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                *_properties(
+                    {
+                        "batch_size": self.batch_size,
+                        "batch_concurrency": self.batch_concurrency,
+                        "lookback": self.lookback,
+                        "forward_only": self.forward_only,
+                        "disable_restatement": self.disable_restatement,
+                    }
+                ),
+            ],
+        )
+
 
 class IncrementalByTimeRangeKind(_IncrementalBy):
     name: Literal[ModelKindName.INCREMENTAL_BY_TIME_RANGE] = ModelKindName.INCREMENTAL_BY_TIME_RANGE
@@ -361,8 +396,15 @@ class IncrementalByTimeRangeKind(_IncrementalBy):
 
     _time_column_validator = TimeColumn.validator()
 
-    def to_expression(self, dialect: str = "", **kwargs: t.Any) -> d.ModelKind:
-        return super().to_expression(expressions=[self.time_column.to_property(dialect)])
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                self.time_column.to_property(kwargs.get("dialect") or ""),
+            ]
+        )
 
     @property
     def data_hash_values(self) -> t.List[t.Optional[str]]:
@@ -415,6 +457,21 @@ class IncrementalByUniqueKeyKind(_IncrementalBy):
             gen(self.when_matched) if self.when_matched is not None else None,
         ]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                *_properties(
+                    {
+                        "unique_key": exp.Tuple(expressions=self.unique_key),
+                        "when_matched": self.when_matched,
+                    }
+                ),
+            ],
+        )
+
 
 class IncrementalByPartitionKind(_Incremental):
     name: Literal[ModelKindName.INCREMENTAL_BY_PARTITION] = ModelKindName.INCREMENTAL_BY_PARTITION
@@ -429,6 +486,21 @@ class IncrementalByPartitionKind(_Incremental):
             str(self.disable_restatement),
         ]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                *_properties(
+                    {
+                        "forward_only": self.forward_only,
+                        "disable_restatement": self.disable_restatement,
+                    }
+                ),
+            ],
+        )
+
 
 class IncrementalUnmanagedKind(_Incremental):
     name: Literal[ModelKindName.INCREMENTAL_UNMANAGED] = ModelKindName.INCREMENTAL_UNMANAGED
@@ -448,6 +520,22 @@ class IncrementalUnmanagedKind(_Incremental):
             str(self.disable_restatement),
         ]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                *_properties(
+                    {
+                        "insert_overwrite": self.insert_overwrite,
+                        "forward_only": self.forward_only,
+                        "disable_restatement": self.disable_restatement,
+                    }
+                ),
+            ],
+        )
+
 
 class ViewKind(_ModelKind):
     name: Literal[ModelKindName.VIEW] = ModelKindName.VIEW
@@ -457,6 +545,16 @@ class ViewKind(_ModelKind):
     def data_hash_values(self) -> t.List[t.Optional[str]]:
         return [*super().data_hash_values, str(self.materialized)]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                _property("materialized", self.materialized),
+            ],
+        )
+
 
 class SeedKind(_ModelKind):
     name: Literal[ModelKindName.SEED] = ModelKindName.SEED
@@ -478,14 +576,18 @@ class SeedKind(_ModelKind):
             return CsvSettings(**v)
         return v
 
-    def to_expression(self, **kwargs: t.Any) -> d.ModelKind:
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
         """Convert the seed kind into a SQLGlot expression."""
         return super().to_expression(
             expressions=[
-                exp.Property(this=exp.Var(this="path"), value=exp.Literal.string(self.path)),
-                exp.Property(
-                    this=exp.Var(this="batch_size"),
-                    value=exp.Literal.number(self.batch_size),
+                *(expressions or []),
+                *_properties(
+                    {
+                        "path": exp.Literal.string(self.path),
+                        "batch_size": self.batch_size,
+                    }
                 ),
             ],
         )
@@ -564,6 +666,26 @@ class _SCDType2Kind(_Incremental):
             str(self.disable_restatement),
         ]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                *_properties(
+                    {
+                        "unique_key": exp.Tuple(expressions=self.unique_key),
+                        "valid_from_name": self.valid_from_name,
+                        "valid_to_name": self.valid_to_name,
+                        "invalidate_hard_deletes": self.invalidate_hard_deletes,
+                        "time_data_type": self.time_data_type,
+                        "forward_only": self.forward_only,
+                        "disable_restatement": self.disable_restatement,
+                    }
+                ),
+            ],
+        )
+
 
 class SCDType2ByTimeKind(_SCDType2Kind):
     name: Literal[ModelKindName.SCD_TYPE_2, ModelKindName.SCD_TYPE_2_BY_TIME] = (
@@ -585,6 +707,21 @@ class SCDType2ByTimeKind(_SCDType2Kind):
             str(self.updated_at_as_valid_from),
         ]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                *_properties(
+                    {
+                        "updated_at_name": self.updated_at_name,
+                        "updated_at_as_valid_from": self.updated_at_as_valid_from,
+                    }
+                ),
+            ],
+        )
+
 
 class SCDType2ByColumnKind(_SCDType2Kind):
     name: Literal[ModelKindName.SCD_TYPE_2_BY_COLUMN] = ModelKindName.SCD_TYPE_2_BY_COLUMN
@@ -600,6 +737,23 @@ class SCDType2ByColumnKind(_SCDType2Kind):
         )
         return [*super().data_hash_values, *columns_sql, str(self.execution_time_as_valid_from)]
 
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
+        return super().to_expression(
+            expressions=[
+                *(expressions or []),
+                *_properties(
+                    {
+                        "columns": exp.Tuple(expressions=self.columns)
+                        if isinstance(self.columns, list)
+                        else self.columns,
+                        "execution_time_as_valid_from": self.execution_time_as_valid_from,
+                    }
+                ),
+            ],
+        )
+
 
 class EmbeddedKind(_ModelKind):
     name: Literal[ModelKindName.EMBEDDED] = ModelKindName.EMBEDDED
@@ -653,21 +807,19 @@ class CustomKind(_ModelKind):
             str(self.disable_restatement),
         ]
 
-    def to_expression(self, **kwargs: t.Any) -> d.ModelKind:
+    def to_expression(
+        self, expressions: t.Optional[t.List[exp.Expression]] = None, **kwargs: t.Any
+    ) -> d.ModelKind:
         return super().to_expression(
             expressions=[
-                exp.Property(
-                    this=exp.var("materialization"),
-                    value=exp.Literal.string(self.materialization),
-                ),
-                exp.Property(
-                    this=exp.var("materialization_properties"),
-                    value=self.materialization_properties_,
-                ),
-                exp.Property(this=exp.var("forward_only"), value=exp.convert(self.forward_only)),
-                exp.Property(
-                    this=exp.var("disable_restatement"),
-                    value=exp.convert(self.disable_restatement),
+                *(expressions or []),
+                *_properties(
+                    {
+                        "materialization": exp.Literal.string(self.materialization),
+                        "materialization_properties": self.materialization_properties_,
+                        "forward_only": self.forward_only,
+                        "disable_restatement": self.disable_restatement,
+                    }
                 ),
             ],
         )
@@ -759,3 +911,11 @@ def _model_kind_validator(cls: t.Type, v: t.Any, values: t.Dict[str, t.Any]) ->
 
 
 model_kind_validator = field_validator("kind", mode="before")(_model_kind_validator)
+
+
+def _property(name: str, value: t.Any) -> exp.Property:
+    return exp.Property(this=exp.var(name), value=exp.convert(value))
+
+
+def _properties(name_value_pairs: t.Dict[str, t.Any]) -> t.List[exp.Property]:
+    return [_property(k, v) for k, v in name_value_pairs.items() if v is not None]


Test patch:
diff --git a/tests/core/test_model.py b/tests/core/test_model.py
index 687fbc22..29433495 100644
--- a/tests/core/test_model.py
+++ b/tests/core/test_model.py
@@ -966,7 +966,10 @@ def test_render_definition():
             owner owner_name,
             dialect spark,
             kind INCREMENTAL_BY_TIME_RANGE (
-                time_column (`a`, 'yyyymmdd')
+                time_column (`a`, 'yyyymmdd'),
+                forward_only FALSE,
+                disable_restatement FALSE,
+                on_destructive_change 'ERROR'
             ),
             storage_format iceberg,
             partitioned_by `a`,
@@ -1058,7 +1061,9 @@ def test_render_definition_with_defaults():
             owner owner_name,
             cron '@daily',
             dialect spark,
-            kind VIEW,
+            kind VIEW (
+                materialized FALSE
+            )
         );
 
         {query}
@@ -4779,3 +4784,315 @@ def test_custom_kind():
         "key_c": True,
         "key_d": 1.23,
     }
+
+    assert (
+        kind.to_expression().sql()
+        == """CUSTOM (
+materialization 'MyTestStrategy',
+materialization_properties ('key_a' = 'value_a', key_b = 2, 'key_c' = TRUE, 'key_d' = 1.23),
+forward_only TRUE,
+disable_restatement TRUE
+)"""
+    )
+
+
+def test_model_kind_to_expression():
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind INCREMENTAL_BY_TIME_RANGE(
+                time_column a,
+            ),
+        );
+        SELECT a, b
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """INCREMENTAL_BY_TIME_RANGE (
+time_column ("a", '%Y-%m-%d'),
+forward_only FALSE,
+disable_restatement FALSE,
+on_destructive_change 'ERROR'
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind INCREMENTAL_BY_TIME_RANGE(
+                time_column a,
+                batch_size 1,
+                batch_concurrency 2,
+                lookback 3,
+                forward_only TRUE,
+                disable_restatement TRUE,
+                on_destructive_change WARN,
+            ),
+        );
+        SELECT a, b
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """INCREMENTAL_BY_TIME_RANGE (
+time_column ("a", '%Y-%m-%d'),
+batch_size 1,
+batch_concurrency 2,
+lookback 3,
+forward_only TRUE,
+disable_restatement TRUE,
+on_destructive_change 'WARN'
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind INCREMENTAL_BY_UNIQUE_KEY(
+                unique_key a,
+            ),
+        );
+        SELECT a, b
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """INCREMENTAL_BY_UNIQUE_KEY (
+unique_key ("a"),
+batch_concurrency 1,
+forward_only FALSE,
+disable_restatement FALSE,
+on_destructive_change 'ERROR'
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind INCREMENTAL_BY_UNIQUE_KEY(
+                unique_key a,
+                when_matched WHEN MATCHED THEN UPDATE SET target.b = COALESCE(source.b, target.b)
+            ),
+        );
+        SELECT a, b
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """INCREMENTAL_BY_UNIQUE_KEY (
+unique_key ("a"),
+when_matched WHEN MATCHED THEN UPDATE SET __MERGE_TARGET__.b = COALESCE(__MERGE_SOURCE__.b, __MERGE_TARGET__.b),
+batch_concurrency 1,
+forward_only FALSE,
+disable_restatement FALSE,
+on_destructive_change 'ERROR'
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind INCREMENTAL_BY_PARTITION,
+            partitioned_by ["a"],
+        );
+        SELECT a, b
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """INCREMENTAL_BY_PARTITION (
+forward_only TRUE,
+disable_restatement TRUE,
+on_destructive_change 'ERROR'
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.seed,
+            kind SEED (
+              path '../seeds/waiter_names.csv',
+            )
+        );
+        """
+            ),
+            path=Path("./examples/sushi/models/test_model.sql"),
+        )
+        .kind.to_expression()
+        .sql()
+        == """SEED (
+path '../seeds/waiter_names.csv',
+batch_size 1000
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind SCD_TYPE_2_BY_TIME (
+                unique_key [a, b]
+            )
+        );
+        SELECT a, b
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """SCD_TYPE_2_BY_TIME (
+updated_at_name "updated_at",
+updated_at_as_valid_from FALSE,
+unique_key ("a", "b"),
+valid_from_name "valid_from",
+valid_to_name "valid_to",
+invalidate_hard_deletes FALSE,
+time_data_type TIMESTAMP,
+forward_only TRUE,
+disable_restatement TRUE,
+on_destructive_change 'ERROR'
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind SCD_TYPE_2_BY_COLUMN (
+                unique_key [a, b],
+                columns [b]
+            )
+        );
+        SELECT a, b, c
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """SCD_TYPE_2_BY_COLUMN (
+columns ("b"),
+execution_time_as_valid_from FALSE,
+unique_key ("a", "b"),
+valid_from_name "valid_from",
+valid_to_name "valid_to",
+invalidate_hard_deletes FALSE,
+time_data_type TIMESTAMP,
+forward_only TRUE,
+disable_restatement TRUE,
+on_destructive_change 'ERROR'
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind SCD_TYPE_2_BY_COLUMN (
+                unique_key [a, b],
+                columns *
+            )
+        );
+        SELECT a, b, c
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """SCD_TYPE_2_BY_COLUMN (
+columns *,
+execution_time_as_valid_from FALSE,
+unique_key ("a", "b"),
+valid_from_name "valid_from",
+valid_to_name "valid_to",
+invalidate_hard_deletes FALSE,
+time_data_type TIMESTAMP,
+forward_only TRUE,
+disable_restatement TRUE,
+on_destructive_change 'ERROR'
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind FULL
+        );
+        SELECT a, b, c
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == "FULL"
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind VIEW
+        );
+        SELECT a, b, c
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """VIEW (
+materialized FALSE
+)"""
+    )
+
+    assert (
+        load_sql_based_model(
+            d.parse(
+                """
+        MODEL (
+            name db.table,
+            kind VIEW (materialized true)
+        );
+        SELECT a, b, c
+        """
+            )
+        )
+        .kind.to_expression()
+        .sql()
+        == """VIEW (
+materialized TRUE
+)"""
+    )
diff --git a/tests/integrations/github/cicd/test_integration.py b/tests/integrations/github/cicd/test_integration.py
index e5d318d2..51a0367b 100644
--- a/tests/integrations/github/cicd/test_integration.py
+++ b/tests/integrations/github/cicd/test_integration.py
@@ -164,7 +164,7 @@ def test_merge_pr_has_non_breaking_change(
 
 +++ 
 
-@@ -11,7 +11,8 @@
+@@ -15,7 +15,8 @@
 
  SELECT
    CAST(o.waiter_id AS INT) AS waiter_id,
@@ -361,7 +361,7 @@ def test_merge_pr_has_non_breaking_change_diff_start(
 
 +++ 
 
-@@ -11,7 +11,8 @@
+@@ -15,7 +15,8 @@
 
  SELECT
    CAST(o.waiter_id AS INT) AS waiter_id,
@@ -866,7 +866,7 @@ def test_no_merge_since_no_deploy_signal(
 
 +++ 
 
-@@ -11,7 +11,8 @@
+@@ -15,7 +15,8 @@
 
  SELECT
    CAST(o.waiter_id AS INT) AS waiter_id,
@@ -1047,7 +1047,7 @@ def test_no_merge_since_no_deploy_signal_no_approvers_defined(
 
 +++ 
 
-@@ -11,7 +11,8 @@
+@@ -15,7 +15,8 @@
 
  SELECT
    CAST(o.waiter_id AS INT) AS waiter_id,
@@ -1217,7 +1217,7 @@ def test_deploy_comment_pre_categorized(
 
 +++ 
 
-@@ -11,7 +11,8 @@
+@@ -15,7 +15,8 @@
 
  SELECT
    CAST(o.waiter_id AS INT) AS waiter_id,