fix(format): preserve dialect-specific SQL in MODEL/AUDIT/METRIC headers - #5950
Closed
mday-io wants to merge 5 commits into
Closed
fix(format): preserve dialect-specific SQL in MODEL/AUDIT/METRIC headers#5950mday-io wants to merge 5 commits into
mday-io wants to merge 5 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>
4 tasks
Collaborator
Author
|
Superseded by reopened #5949. Both PRs have the identical merge diff. |
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
1 participant
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.
--