Skip to content

fix(format): preserve model column dialect - #5926

Open
fresioAS wants to merge 3 commits into
SQLMesh:mainfrom
fresioAS:fix_columns_dialect
Open

fix(format): preserve model column dialect#5926
fresioAS wants to merge 3 commits into
SQLMesh:mainfrom
fresioAS:fix_columns_dialect

Conversation

@fresioAS

Copy link
Copy Markdown
Contributor

Description

Summary

  • Fixes regression introduced in fix(format): keep MODEL/AUDIT/METRIC header dialect-agnostic (#5773) #5864
  • Keep SQLMesh MODEL metadata dialect-agnostic during formatting.
  • Render the MODEL columns schema using the model's dialect.
  • Preserve dialect-specific column types such as DATETIME2(6) for T-SQL and Fabric.
  • Keep generic metadata values such as descriptions and booleans unchanged.

Context

MODEL headers were recently changed to use SQLGlot's generic generator so that
T-SQL would not rewrite SQLMesh boolean properties such as FALSE into
(1 = 0).

However, the columns property contains dialect-specific data types. Rendering
it with the generic generator changed DATETIME2(6) into TIMESTAMP(6) for
T-SQL and Fabric models.

This change keeps the generic header behavior while rendering only the
columns schema with the model dialect.

Test Plan

  • Added regression coverage for both tsql and fabric.
  • Verified descriptions remain string literals.
  • Verified formatting booleans remain FALSE.
  • Full tests/core/test_dialect.py suite passes: 161 tests.
  • Ruff and mypy pass.

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

fresioAS and others added 2 commits July 28, 2026 15:06
@mday-io

mday-io commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

@fresioAS

Sharing some findings from digging into this area. They support the approach here and suggest a way to extend it.

  1. columns isn't the only property that lost its dialect. Rendering the whole header generically affects every property whose value is user-authored SQL. On main today, a T-SQL model with audits (my_audit(threshold := CAST('2024-01-01' AS DATETIME2))) formats to '2024-01-01'::TIMESTAMP, the same silent type downgrade as columns, just unreported so far. The same applies to expression values in physical_properties, and to time_data_type nested inside the SCD kinds. So the columns-only fix leaves some corruption in place.

  2. Probably don't need a hand-maintained property list. ModelMeta already declares which properties hold expressions (columns, audits, signals, partitioned_by, physical_properties, …) versus which are SQLMesh's own scalars (allow_partials, description, kind, …). Deriving the policy from those annotations classifies all of them, and the failure mode inverts usefully: a property that's missed doesn't get canonicalized, rather than having a user's SQL silently rewritten.

I prototyped this as a one-condition change to the hook in this PR. Swapping the columns name check for a policy lookup - extended to AUDIT/METRIC headers and nested kinds. It fixes the audits/physical_properties/time_data_type cases too, and sqlmesh format --transpile starts converting column types correctly again while keeping booleans safe. No regressions found with this approach.

Happy to open it as a follow-up on top of this, or hand it over if you'd rather fold it in. This PR's diagnosis and hook design are what it's built on.

One process note independent of the approach: whatever lands is worth calling out as a behavior change in the release notes. This would be the third consecutive release that reformats T-SQL headers

@fresioAS

fresioAS commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @mday-io

This sounds like a good approach which should cover both this and #5944

If you already have a working prototype of the changes I will leave you to open up a follow-up!

@mday-io

mday-io commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Followed up on this and the audits case is worse than I described — it compounds across runs rather than settling:

format pass 1: my_audit(t := '2024-01-01'::TIMESTAMP)
format pass 2: my_audit(t := '2024-01-01'::VARBINARY)
format pass 3: my_audit(t := '2024-01-01'::VARBINARY)

T-SQL's TIMESTAMP is a deprecated synonym for ROWVERSION, a binary type — exp.DataType.build('TIMESTAMP', dialect='tsql') returns DType.ROWVERSION. The generic render writes TIMESTAMP, the next parse reads it back as rowversion, and it settles on VARBINARY. Two runs of sqlmesh format turn a datetime into a binary type.

Same for time_data_type in the SCD kinds (DATETIME2(6)TIMESTAMP(6)VARBINARY(6)) — that one is the physical type of the valid_from/valid_to columns, so it yields wrong DDL, not just a wrong-looking file. Fabric is unaffected; its TIMESTAMP stays TIMESTAMP.

None of this is caused by this PR — columns is fixed correctly here, and it's all live on main today. The same bug just sits in the properties next door and degrades further on every run.

The catch is that fixing those properly supersedes this hook rather than extending it. The name check becomes a derived lookup:

- if prop.name.lower() == "columns" and columns_dialect and isinstance(value, exp.Expr):
+ if meta_dialect and isinstance(value, exp.Expr) and _meta_render_policy().get(prop.name.lower()):

The policy comes from the field declarations rather than a hand-maintained list — across ModelMeta, ModelAudit, MetricMeta and the *Kind classes that classifies all 78 header properties with no collisions, and a property that gets missed fails safe (a keyword isn't canonicalized, rather than a user's SQL being rewritten). Plus propagating the dialect tag into nested ModelKind so time_data_type is covered, and an idempotency test — format(format(x)) == format(x) — which catches this whole class without anyone enumerating properties. Nothing tests that today, which is how it survived #5864.

Since it replaces this implementation, merging both would mean landing your 70 lines and deleting them in the same release. Cleaner as one PR: I'll open it with you as co-author, carrying your diagnosis and hook design, and close this one out.

Shout if you'd rather handle it differently and I can provide some more of my findings

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.

2 participants