Skip to content

Commit 29065a9

Browse files
committed
fix(format): stop kind's own render policy from transpiling scalar siblings
Recursing _holds_expression into nested Pydantic models to correctly classify TimeColumn (IncrementalByTimeRangeKind.time_column) as warehouse SQL had the side effect of also matching ModelMeta.kind itself, since some member of the ModelKind union holds an expression field. That routed the entire kind (...) subtree through a dialect-specific generator, so on tsql a scalar sibling like forward_only TRUE was rewritten to (1 = 1) -- which reparses fine but silently evaluates to False on reload via str_to_bool. kind's own nested properties are already independently dialect-tagged via the ModelKind expression node's own meta when _props_sql recurses into them, so the outer kind property's policy should never route its subtree through render_with_model_dialect. Stop _holds_expression at _ModelKind subclasses to restore that. Signed-off-by: mday-io <mdaytn@gmail.com>
1 parent ae92536 commit 29065a9

2 files changed

Lines changed: 69 additions & 0 deletions

File tree

sqlmesh/core/dialect.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -744,10 +744,24 @@ def _holds_expression(annotation: t.Any, _visited: t.Optional[t.FrozenSet[t.Any]
744744
nested Tuple[str, Dict[str, exp.Expr]] shape used by audits/signals, and nested
745745
Pydantic models that themselves wrap an expression field, such as `TimeColumn`
746746
(IncrementalByTimeRangeKind.time_column).
747+
748+
Stops at `_ModelKind` subclasses without recursing into their fields: a `kind`
749+
property's own nested properties are independently dialect-tagged via the
750+
`ModelKind` expression node's own meta when `_props_sql` recurses into them, so
751+
treating the `kind` field itself as "holds an expression" -- true only because some
752+
other member of the `ModelKind` union has an expression field, e.g.
753+
`IncrementalByTimeRangeKind.time_column` -- would route its entire subtree,
754+
including scalar sibling properties like `forward_only`, through a dialect-specific
755+
generator and transpile them when they shouldn't be (tsql booleans becoming
756+
`(1 = 1)`, which silently reparses as `False`).
747757
"""
758+
from sqlmesh.core.model.kind import _ModelKind
759+
748760
if isinstance(annotation, type):
749761
if issubclass(annotation, exp.Expr):
750762
return True
763+
if issubclass(annotation, _ModelKind):
764+
return False
751765
visited = _visited or frozenset()
752766
if annotation in visited:
753767
return False

tests/core/test_dialect.py

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,61 @@ def test_format_model_expressions_time_column_dialect():
516516
)
517517

518518

519+
def test_format_model_expressions_kind_scalar_sibling_dialect():
520+
"""A scalar sibling property of an expression-bearing property inside `kind` (e.g.
521+
`forward_only` next to `time_column`) must stay dialect-agnostic even though the
522+
render policy correctly marks `kind` as containing an expression-holding field
523+
somewhere in the `ModelKind` union.
524+
525+
Regression: recursing into nested Pydantic models to fix `time_column` (see
526+
`test_format_model_expressions_time_column_dialect`) made `_holds_expression` also
527+
match on `kind` itself, since *some* member of the `ModelKind` union
528+
(`IncrementalByTimeRangeKind.time_column`) holds an expression. That routed the
529+
entire `kind (...)` subtree through a dialect-specific generator, so tsql's
530+
boolean-literal preprocessing rewrote `forward_only TRUE` into `forward_only (1 = 1)`.
531+
That reparses without error, but `str_to_bool` on `Paren(EQ(1, 1)).name` (`""`)
532+
evaluates to `False`, so the value silently flips on reload.
533+
"""
534+
formatted = format_model_expressions(
535+
parse(
536+
"""
537+
MODEL (
538+
name a.b,
539+
dialect tsql,
540+
kind INCREMENTAL_BY_TIME_RANGE (
541+
time_column [end],
542+
forward_only true
543+
)
544+
);
545+
546+
SELECT 1 AS x, [end] FROM t
547+
""",
548+
default_dialect="tsql",
549+
),
550+
dialect="tsql",
551+
)
552+
553+
assert (
554+
formatted
555+
== """MODEL (
556+
name a.b,
557+
dialect tsql,
558+
kind INCREMENTAL_BY_TIME_RANGE (
559+
time_column [end],
560+
forward_only TRUE
561+
)
562+
);
563+
564+
SELECT
565+
1 AS x,
566+
[end]
567+
FROM t"""
568+
)
569+
570+
model = load_sql_based_model(parse(formatted, default_dialect="tsql"), dialect="tsql")
571+
assert model.kind.forward_only is True
572+
573+
519574
def test_format_model_expressions_macro_property_comments_preserved_with_dialect():
520575
"""Comments inside a macro header-property must survive formatting when the model
521576
has a `dialect` set.

0 commit comments

Comments
 (0)