fix(format): preserve dialect-specific SQL in MODEL/AUDIT/METRIC headers - #5949
Open
mday-io wants to merge 6 commits into
Open
fix(format): preserve dialect-specific SQL in MODEL/AUDIT/METRIC headers#5949mday-io wants to merge 6 commits into
mday-io wants to merge 6 commits into
Conversation
…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>
mday-io
force-pushed
the
claude/changes-y8l3ib
branch
from
August 10, 2026 14:44
29065a9 to
157e38b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
#5864fixed formatting corrupting SQLMesh's own header properties (on tsql,allow_partials TRUEwas 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, andphysical_properties. Socolumns (ts DATETIME2(6))would silently flatten to genericTIMESTAMP(6). On tsql this compounds across repeated formatting —DATETIME2→TIMESTAMP→ reparsed asROWVERSION→VARBINARY— so twosqlmesh formatpasses 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 insidekindliketime_data_type/unique_key/time_column) render with the model's dialect. Properties that are SQLMesh's own semantics (allow_partials,description, scalarkindproperties likeforward_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) andtest_format_audit_expressions_meta_render_policy, assertingcolumns,audits,physical_properties, andkind'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 overcolumns,audits,kind,physical_properties, SQLMesh-owned scalars, and macro properties, asserting a secondsqlmesh formatpass is a no-op.test_format_model_expressions_time_column_dialect—time_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— akindblock 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 throughload_sql_based_modelto confirmmodel.kind.forward_only is Truesurvives (not just the string).pytest tests/core/test_dialect.py— 173 passed.ruff checkon the changed files — clean.Release Note
Fix:
sqlmesh formatcorrupting warehouse-specific types in MODEL/AUDIT/METRIC headerssqlmesh formaton models with an explicitdialect(tsql, fabric, and others with dialect-specific type spellings) could silently rewrite warehouse-specific types incolumns,audits,physical_properties, andkind-nested properties (e.g.time_data_type,time_column) to a generic spelling. For tsql specifically, this was compounding:DATETIME2→TIMESTAMP→VARBINARYacross two format runs — for an SCD Type 2 model'stime_data_type, that silently turned the physical type of thevalid_from/valid_tocolumns from a datetime into a binary type.Header properties that hold SQLMesh's own semantics (
allow_partials,description,kindname, boolean kind properties likeforward_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 formaton tsql/fabric models with warehouse-specific types incolumns,audits,physical_properties, or SCDkindblocks, re-runsqlmesh formatafter upgrading and review the diff — previously-corrupted types will be restored to their correct dialect-specific spelling.Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO