Skip to content

fix(format): preserve dialect-specific SQL in MODEL/AUDIT/METRIC headers - #5949

Open
mday-io wants to merge 6 commits into
SQLMesh:mainfrom
mday-io:claude/changes-y8l3ib
Open

fix(format): preserve dialect-specific SQL in MODEL/AUDIT/METRIC headers#5949
mday-io wants to merge 6 commits into
SQLMesh:mainfrom
mday-io:claude/changes-y8l3ib

Conversation

@mday-io

@mday-io mday-io commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Description

#5864 fixed formatting corrupting SQLMesh's own header properties (on tsql, allow_partials TRUE was becoming the unparseable (1 = 1)) by no longer transpiling MODEL/AUDIT/METRIC headers at all.

That was too broad a fix: it also stopped transpiling the properties that are real warehouse SQL, like columns, audits, and physical_properties. So columns (ts DATETIME2(6)) would silently flatten to generic TIMESTAMP(6). On tsql this compounds across repeated formatting — DATETIME2TIMESTAMP → reparsed as ROWVERSIONVARBINARY — so two sqlmesh format passes could turn a datetime column into a binary one with no error.

This PR renders header properties individually instead of applying one blanket rule. Properties holding real warehouse SQL (columns, audits, physical_properties, macro properties, and expression fields nested inside kind like time_data_type/unique_key/time_column) render with the model's dialect. Properties that are SQLMesh's own semantics (allow_partials, description, scalar kind properties like forward_only) stay dialect-agnostic. Which bucket a property falls into is derived automatically from its field declaration, so the split stays correct as new properties are added, and a missed property fails safe — a keyword goes uncanonicalized rather than a user's SQL being corrupted.

Covers MODEL, AUDIT, and METRIC headers.

Test Plan

  • test_format_model_expressions_meta_render_policy (parametrized over tsql/fabric) and test_format_audit_expressions_meta_render_policy, asserting columns, audits, physical_properties, and kind's nested expression properties keep their dialect-specific spelling while SQLMesh's own scalar properties stay dialect-agnostic.
  • test_format_model_expressions_is_idempotent, parametrized over columns, audits, kind, physical_properties, SQLMesh-owned scalars, and macro properties, asserting a second sqlmesh format pass is a no-op.
  • test_format_model_expressions_time_column_dialecttime_column's identifier quoting (e.g. tsql [end]) survives formatting instead of falling back to ANSI quoting.
  • test_format_model_expressions_macro_property_comments_preserved_with_dialect — comments inside a macro header-property's arguments survive formatting on a dialect-bearing model, plus an idempotency check.
  • test_format_model_expressions_kind_scalar_sibling_dialect — a kind block combining an expression property (time_column) with a boolean scalar sibling (forward_only) on tsql round-trips both correctly; asserts on the formatted string and round-trips through load_sql_based_model to confirm model.kind.forward_only is True survives (not just the string).
  • pytest tests/core/test_dialect.py — 173 passed.
  • ruff check on the changed files — clean.

Release Note

Fix: sqlmesh format corrupting warehouse-specific types in MODEL/AUDIT/METRIC headers

sqlmesh format on models with an explicit dialect (tsql, fabric, and others with dialect-specific type spellings) could silently rewrite warehouse-specific types in columns, audits, physical_properties, and kind-nested properties (e.g. time_data_type, time_column) to a generic spelling. For tsql specifically, this was compounding: DATETIME2TIMESTAMPVARBINARY across two format runs — for an SCD Type 2 model's time_data_type, that silently turned the physical type of the valid_from/valid_to columns from a datetime into a binary type.

Header properties that hold SQLMesh's own semantics (allow_partials, description, kind name, boolean kind properties like forward_only, etc.) are unaffected and continue to render dialect-agnostically, so this only changes output for the subset of header properties that carry actual warehouse SQL.

Action for affected users: if you run sqlmesh format on tsql/fabric models with warehouse-specific types in columns, audits, physical_properties, or SCD kind blocks, re-run sqlmesh format after upgrading and review the diff — previously-corrupted types will be restored to their correct dialect-specific spelling.

Checklist

  • I have run make style and fixed any issues
  • I have added tests for my changes (if applicable)
  • All existing tests pass (make fast-test)
  • My commits are signed off (git commit -s) per the DCO

…alect

SQLMesh#5864 stopped transpiling MODEL/AUDIT/METRIC headers so that SQLMesh's own
boolean properties would survive formatting -- on tsql, `allow_partials TRUE`
was being rewritten to `(1 = 1)`, which then fails to parse at all and leaves
the model file broken.

That fix rendered the entire header generically, including the properties whose
values are the user's warehouse SQL. Those lose their dialect: `columns
(ts DATETIME2(6))` becomes `TIMESTAMP(6)`, and an audit argument such as
`CAST('2024-01-01' AS DATETIME2)` is silently downgraded the same way.

Split the header per property instead of per expression. The split is derived
from the field declarations themselves: expression-typed fields (columns,
audits, signals, partitioned_by, physical_properties, ...) hold warehouse SQL
and render with the model dialect, while scalar-typed fields (allow_partials,
description, kind, ...) are SQLMesh's own semantics and stay dialect-agnostic.
Deriving it means the policy stays correct as properties are added, and a field
that is missed fails safe -- a keyword is not canonicalized, rather than a
user's SQL being corrupted.

Covers MODEL, AUDIT and METRIC headers, and the expression properties nested
inside `kind` such as `time_data_type` and `unique_key`.

Signed-off-by: mday-io <mdaytn@gmail.com>
Rendering a dialect-specific type with the generic generator compounds across
runs rather than merely looking different: tsql `DATETIME2` renders as
`TIMESTAMP`, and tsql parses `TIMESTAMP` as ROWVERSION, so a second pass writes
`VARBINARY`. Two runs of `sqlmesh format` silently turned a datetime into a
binary type -- and for an SCD kind's `time_data_type` that is the physical type
of the valid_from/valid_to columns.

Covers columns, audits, nested kind properties, physical_properties and the
SQLMesh-owned scalars.

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

A macro in property position wraps user-authored arguments, so it carries
warehouse SQL the same way `columns` or `audits` do. It took a separate branch
in _props_sql and kept rendering generically, which left it on the compounding
path: DATETIME2 -> TIMESTAMP -> VARBINARY across two format runs.

Signed-off-by: mday-io <mdaytn@gmail.com>
…roperty 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>
…blings

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant