fix(format): recurse into nested Pydantic models and preserve macro property comments

Two gaps in the header-property dialect-render policy from the previous fix:

- `_holds_expression` only checked the outer type annotation and typing
  generics (`Optional`, `List`, ...), so a nested Pydantic model wrapping an
  expression field -- `TimeColumn` on
  `IncrementalByTimeRangeKind.time_column` -- was misclassified as a scalar
  property and fell back to generic rendering, losing dialect-specific
  identifier quoting (tsql `[end]` became ANSI `"end"`). Recurse into
  `model_fields` for any type that exposes them, guarded by a visited set.

- The `MacroFunc` dialect-render branch passed `comments=False` into
  `render_with_model_dialect`, which threads it to `Expression.sql()`'s
  fresh per-call `Generator` constructor -- a generator-wide flag that
  disables every comment in the subtree, not just the redundant outer
  `maybe_comment` call. Comments inside macro header-properties (e.g.
  `@my_prop(cutoff := ... /* note */)`) were silently dropped whenever the
  model declared a `dialect`. Render a copy of the property with its own
  top-level comments cleared instead, leaving `.this`'s comments -- which
  `_macro_func_sql` already attaches -- untouched.

Signed-off-by: mday-io <mdaytn@gmail.com>

Author: mday-io

sqlmesh/core/dialect.py

@@ -737,15 +737,28 @@ def parse(self: Parser) -> t.Optional[exp.Expr]:
 _SQLMESH_META_DIALECT = "sqlmesh_meta_dialect"
 
 
-def _holds_expression(annotation: t.Any) -> bool:
+def _holds_expression(annotation: t.Any, _visited: t.Optional[t.FrozenSet[t.Any]] = None) -> bool:
     """Whether a declared field type bottoms out in a SQLGlot expression.
 
-    Covers List[exp.Expr], Optional[Dict[str, exp.DataType]], Optional[exp.Tuple] and
-    the nested Tuple[str, Dict[str, exp.Expr]] shape used by audits/signals.
+    Covers List[exp.Expr], Optional[Dict[str, exp.DataType]], Optional[exp.Tuple], the
+    nested Tuple[str, Dict[str, exp.Expr]] shape used by audits/signals, and nested
+    Pydantic models that themselves wrap an expression field, such as `TimeColumn`
+    (IncrementalByTimeRangeKind.time_column).
     """
-    if isinstance(annotation, type) and issubclass(annotation, exp.Expr):
-        return True
-    return any(_holds_expression(arg) for arg in t.get_args(annotation))
+    if isinstance(annotation, type):
+        if issubclass(annotation, exp.Expr):
+            return True
+        visited = _visited or frozenset()
+        if annotation in visited:
+            return False
+        if hasattr(annotation, "model_fields"):
+            visited = visited | {annotation}
+            return any(
+                _holds_expression(field.annotation, visited)
+                for field in annotation.model_fields.values()
+            )
+        return False
+    return any(_holds_expression(arg, _visited) for arg in t.get_args(annotation))
 
 
 @functools.lru_cache(maxsize=1)
@@ -806,12 +819,19 @@ def render_with_model_dialect(node: exp.Expr, **overrides: t.Any) -> str:
 
         if isinstance(prop, MacroFunc):
             # A macro in property position wraps user-authored arguments, so it carries
-            # warehouse SQL the same way `columns` or `audits` do.
-            sql = self.indent(
-                render_with_model_dialect(prop, comments=False)
-                if meta_dialect
-                else self.sql(prop, comment=False)
-            )
+            # warehouse SQL the same way `columns` or `audits` do. Clear the outer node's
+            # own comments (not `.this`'s, which `_macro_func_sql` already attaches)
+            # before rendering with the model dialect, mirroring what `comment=False`
+            # does for the non-dialect path below -- passing `comments=False` here
+            # instead would build a fresh Generator with comments globally disabled,
+            # silently dropping every comment in the subtree rather than just the
+            # redundant outer one.
+            if meta_dialect:
+                prop_for_render = prop.copy()
+                prop_for_render.comments = None
+                sql = self.indent(render_with_model_dialect(prop_for_render))
+            else:
+                sql = self.indent(self.sql(prop, comment=False))
         else:
             value = prop.args.get("value")
 

tests/core/test_dialect.py

@@ -473,6 +473,95 @@ def test_format_audit_expressions_meta_render_policy():
     assert "cutoff := '2024-01-01'::DATETIME2" in formatted
 
 
+def test_format_model_expressions_time_column_dialect():
+    """`time_column` is a nested Pydantic model (`TimeColumn`) wrapping an expression, not
+    an `exp.Expr` annotation itself, so the render-policy reflection must recurse into
+    nested Pydantic models to classify it as warehouse SQL. Otherwise it falls back to
+    generic rendering and loses dialect-specific identifier quoting: tsql's `[end]`
+    becomes ANSI `"end"`, even though the same identifier in the query body is correctly
+    kept as `[end]`.
+    """
+    formatted = format_model_expressions(
+        parse(
+            """
+            MODEL (
+              name a.b,
+              dialect tsql,
+              kind INCREMENTAL_BY_TIME_RANGE (
+                time_column [end]
+              )
+            );
+
+            SELECT 1 AS x, [end] FROM t
+            """,
+            default_dialect="tsql",
+        ),
+        dialect="tsql",
+    )
+
+    assert (
+        formatted
+        == """MODEL (
+  name a.b,
+  dialect tsql,
+  kind INCREMENTAL_BY_TIME_RANGE (
+    time_column [end]
+  )
+);
+
+SELECT
+  1 AS x,
+  [end]
+FROM t"""
+    )
+
+
+def test_format_model_expressions_macro_property_comments_preserved_with_dialect():
+    """Comments inside a macro header-property must survive formatting when the model
+    has a `dialect` set.
+
+    The dialect-render path goes through `Expression.sql(dialect=...)`, which builds a
+    fresh `Generator` with `comments` as a constructor flag: passing `comments=False`
+    there disables comment rendering for the *entire* subtree, rather than just
+    suppressing the redundant outer-level `maybe_comment` call the way `comment=False`
+    does for `Generator.sql()`. That previously caused comments like `/* inline note */`
+    to be silently dropped whenever the model declared a `dialect`.
+    """
+    formatted = format_model_expressions(
+        parse(
+            """
+            MODEL (
+              name a.b,
+              dialect tsql,
+              @my_prop(cutoff := CAST('2024-01-01' AS DATETIME2) /* inline note */)
+            );
+
+            SELECT 1 AS x
+            """,
+            default_dialect="tsql",
+        ),
+        dialect="tsql",
+    )
+
+    assert "/* inline note */" in formatted
+    assert (
+        formatted
+        == """MODEL (
+  name a.b,
+  dialect tsql,
+  @my_prop(cutoff := '2024-01-01'::DATETIME2 /* inline note */)
+);
+
+SELECT
+  1 AS x"""
+    )
+
+    # Idempotency: formatting an already-formatted macro property must not duplicate or
+    # drop the comment on a second pass.
+    twice = format_model_expressions(parse(formatted, default_dialect="tsql"), dialect="tsql")
+    assert formatted == twice
+
+
 def test_format_model_expressions_normalize_functions():
     """Regression: formatter function-name casing behavior.