From 3cc4daa97c23d1b11141c6500bdfcd1dcd580397 Mon Sep 17 00:00:00 2001 From: mday-io Date: Fri, 11 Sep 2026 10:50:11 -0400 Subject: [PATCH 01/11] perf: avoid full snapshot mapping in _resolve_table/_resolve_tables _resolve_table always merged the environment-wide snapshot->table-name mapping and handed it to exp.replace_tables, which re-normalizes (parses) every mapping key on every call, even to resolve a single table. _resolve_tables did the same for property expressions (virtual_properties, session_properties) that contain no table reference at all. For an environment with N promoted views, this made "Updating virtual layer" O(N^2) in pure Python. _resolve_table now looks up only the one relevant snapshot/table_mapping entry instead of building the full mapping (table_name arrives already normalized to the same key format snapshots/table_mapping use, via d.normalize_model_name at both call sites). _resolve_tables now skips building the mapping and calling replace_tables entirely when the expression has no exp.Table node to replace. Fixes #6017 (one of three sub-issues split out of #6014). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HyFdP5xRu9D368mjGcYDLn Signed-off-by: mday-io --- sqlmesh/core/renderer.py | 47 ++++++++++++---- tests/core/test_model.py | 116 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 12 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index 9f403cbcb4..37fb0c3273 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -330,12 +330,27 @@ def _resolve_table( table_mapping: t.Optional[t.Dict[str, str]] = None, deployability_index: t.Optional[DeployabilityIndex] = None, ) -> exp.Table: + table_mapping = table_mapping or {} + if isinstance(table_name, str): + # table_name arrives here already normalized to a model FQN (see the `resolve_table` + # closure below and the `this_model` call site), the same key format `snapshots` and + # `table_mapping` use. Only the one relevant snapshot needs mapping, not the whole + # environment - building the full mapping made this call O(N) in the number of + # snapshots in the environment for every table resolved. + snapshot = snapshots.get(table_name) if snapshots else None + mapping = { + **self._to_table_mapping([snapshot] if snapshot else [], deployability_index), + **({table_name: table_mapping[table_name]} if table_name in table_mapping else {}), + } + else: + mapping = { + **self._to_table_mapping((snapshots or {}).values(), deployability_index), + **table_mapping, + } + table = exp.replace_tables( t.cast(exp.Table, exp.maybe_parse(table_name, into=exp.Table, dialect=self._dialect)), - { - **self._to_table_mapping((snapshots or {}).values(), deployability_index), - **(table_mapping or {}), - }, + mapping, dialect=self._dialect, copy=False, ) @@ -365,10 +380,6 @@ def _resolve_tables( with self._normalize_and_quote(expression) as expression: snapshots = snapshots or {} table_mapping = table_mapping or {} - mapping = { - **self._to_table_mapping(snapshots.values(), deployability_index), - **table_mapping, - } expand = set(expand) | { name for name, snapshot in snapshots.items() if snapshot.is_embedded } @@ -410,10 +421,22 @@ def _expand(node: exp.Expr) -> exp.Expr: expression = expression.transform(_expand, copy=False) # type: ignore - if mapping: - expression = exp.replace_tables( - expression, mapping, dialect=self._dialect, copy=False - ) + # Building the full snapshot -> table-name mapping and normalizing it in + # exp.replace_tables is O(N) in the number of snapshots in the environment; skip it + # entirely for expressions that don't reference any table at all (e.g. session/ + # virtual properties), since there's nothing for the mapping to replace. + if expression.find(exp.Table): + # mypy loses the `snapshots`/`table_mapping` narrowing above because they're + # captured by the `_expand` closure defined earlier in this block. + assert snapshots is not None and table_mapping is not None + mapping = { + **self._to_table_mapping(snapshots.values(), deployability_index), + **table_mapping, + } + if mapping: + expression = exp.replace_tables( + expression, mapping, dialect=self._dialect, copy=False + ) return expression diff --git a/tests/core/test_model.py b/tests/core/test_model.py index c9045c9506..ab2fdd2e52 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -9748,6 +9748,122 @@ def resolve_parent(evaluator, name): assert post_statements[0].sql() == f'"main"."sqlmesh__schema"."schema__parent__{version}"' +def test_resolve_table_large_environment(make_snapshot: t.Callable, mocker: MockerFixture): + """`_resolve_table` should only build a mapping for the one table being resolved, not the + entire environment (https://github.com/SQLMesh/sqlmesh/issues/6017).""" + + @macro() + def resolve_named(evaluator, name): + return evaluator.resolve_table(name.name) + + target = load_sql_based_model(d.parse("MODEL (name target); SELECT 1 AS c")) + target_snapshot = make_snapshot(target) + target_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + snapshots = {'"target"': target_snapshot} + for i in range(50): + other = load_sql_based_model(d.parse(f"MODEL (name other_{i}); SELECT 1 AS c")) + other_snapshot = make_snapshot(other) + other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + snapshots[f'"other_{i}"'] = other_snapshot + + child = load_sql_based_model( + d.parse( + """ + MODEL (name child); + SELECT c FROM target; + @resolve_named('target') + """ + ) + ) + + spy = mocker.spy(exp, "replace_tables") + + post_statements = child.render_post_statements(snapshots=snapshots) + assert len(post_statements) == 1 + assert post_statements[0].sql() == f'"sqlmesh__default"."target__{target_snapshot.version}"' + + # every replace_tables call made while resolving the single `target` reference should only + # ever see that one mapping entry, not all 51 snapshots in the environment + for call in spy.call_args_list: + assert len(call.args[1]) <= 1 + + # an explicit table_mapping entry takes precedence over the snapshot-derived one (rendered + # via a separate model instance so the statement-render cache doesn't return the earlier result) + child_for_override = load_sql_based_model( + d.parse( + """ + MODEL (name child_override); + SELECT c FROM target; + @resolve_named('target') + """ + ) + ) + override = child_for_override.render_post_statements( + snapshots=snapshots, table_mapping={'"target"': "overridden_table"} + ) + assert override[0].sql() == '"overridden_table"' + + # a name absent from both snapshots and table_mapping resolves unchanged + unmapped = load_sql_based_model( + d.parse( + """ + MODEL (name unmapped_child); + SELECT 1 AS c; + @resolve_named('does_not_exist') + """ + ) + ) + unmapped_result = unmapped.render_post_statements(snapshots=snapshots) + assert unmapped_result[0].sql() == '"does_not_exist"' + + +def test_render_virtual_properties_skips_mapping_without_table_refs( + make_snapshot: t.Callable, mocker: MockerFixture +): + """Rendering a property expression with no table references shouldn't build the full + snapshot -> table-name mapping at all (https://github.com/SQLMesh/sqlmesh/issues/6017).""" + import sqlmesh.core.snapshot as snapshot_module + + model = load_sql_based_model( + d.parse( + """ + MODEL ( + name test_schema.test_model, + virtual_properties ( + labels = [('team', 'data')] + ), + session_properties ( + "spark.executor.memory" = '1G' + ), + ); + SELECT a FROM tbl; + """ + ) + ) + + snapshots = {} + for i in range(50): + other = load_sql_based_model(d.parse(f"MODEL (name other_{i}); SELECT 1 AS c")) + other_snapshot = make_snapshot(other) + other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + snapshots[f'"other_{i}"'] = other_snapshot + + to_table_mapping_spy = mocker.spy(snapshot_module, "to_table_mapping") + + assert model.render_virtual_properties(snapshots=snapshots) == { + "labels": exp.maybe_parse("[('team', 'data')]") + } + assert model.render_session_properties(snapshots=snapshots) == { + "spark.executor.memory": "1G", + } + + # `this_model` resolution may still make a narrow, single-snapshot (or empty) call, but the + # full N-snapshot mapping build in `_resolve_tables` must never fire for a table-less property + for call in to_table_mapping_spy.call_args_list: + assert len(call.args[0]) <= 1 + + def test_cluster_with_complex_expression(): expressions = d.parse( """ From c0eecffa1543040cf1d3bb15502cb3ca3144b580 Mon Sep 17 00:00:00 2001 From: Michael Day Date: Mon, 14 Sep 2026 17:24:17 -0400 Subject: [PATCH 02/11] fix: preserve cross-dialect resolution in narrowed _resolve_table lookup The narrowed single-snapshot lookup added in the previous commit did a raw snapshots.get(table_name) dict lookup. table_name is normalized under the referencing renderer's own dialect, while a snapshots dict key is each model's fqn, normalized under that model's own dialect. These can disagree in casing when models use different dialects (e.g. a case-uppercasing dialect like snowflake referenced from a case-insensitive one like duckdb), causing the lookup to silently miss an existing snapshot and leave the table name unmapped, even though the old full-mapping + exp.replace_tables path (which reconciles casing per-dialect during matching) would have resolved it correctly. _resolve_table now falls back to building the full mapping only when the narrowed lookup misses and the name isn't in table_mapping either, so the common same-dialect case stays O(1) while the rare cross-dialect miss still gets exp.replace_tables' dialect-aware reconciliation. Also adds tests for: the cross-dialect regression itself, table_mapping-only resolution with no snapshots, the non-string exp.Expr branch (otherwise unreachable from any real call site), expand-then-find-Table ordering in _resolve_tables, a table reference appearing only inside a string literal, and deployability_index handling through the narrowed path. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01DzEtt434q32KhtoGDtD424 Signed-off-by: Michael Day --- sqlmesh/core/renderer.py | 25 +++- tests/core/test_model.py | 240 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 261 insertions(+), 4 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index 37fb0c3273..50c05dcc84 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -338,10 +338,27 @@ def _resolve_table( # environment - building the full mapping made this call O(N) in the number of # snapshots in the environment for every table resolved. snapshot = snapshots.get(table_name) if snapshots else None - mapping = { - **self._to_table_mapping([snapshot] if snapshot else [], deployability_index), - **({table_name: table_mapping[table_name]} if table_name in table_mapping else {}), - } + if snapshot is None and snapshots and table_name not in table_mapping: + # table_name is normalized under this renderer's own dialect, but a snapshot's + # fqn (the snapshots dict key) is normalized under that model's own dialect - + # these can disagree in casing when models use different dialects (e.g. a + # case-uppercasing dialect referenced from a case-insensitive one). A direct + # dict lookup can miss in that case even though the table is present, so fall + # back to the full, dialect-reconciling mapping that exp.replace_tables itself + # performs. This only pays the O(N) cost on a miss, not on every resolution. + mapping = { + **self._to_table_mapping(snapshots.values(), deployability_index), + **table_mapping, + } + else: + mapping = { + **self._to_table_mapping([snapshot] if snapshot else [], deployability_index), + **( + {table_name: table_mapping[table_name]} + if table_name in table_mapping + else {} + ), + } else: mapping = { **self._to_table_mapping((snapshots or {}).values(), deployability_index), diff --git a/tests/core/test_model.py b/tests/core/test_model.py index ab2fdd2e52..df748d7c28 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -9864,6 +9864,246 @@ def test_render_virtual_properties_skips_mapping_without_table_refs( assert len(call.args[0]) <= 1 +def test_resolve_table_cross_dialect_fqn_mismatch(make_snapshot: t.Callable): + """`_resolve_table`'s narrowed lookup keys `snapshots` by the caller's already-normalized + `table_name` string. That string is built with the *referencing* model's own dialect + (`self._dialect` in the `resolve_table` macro closure), while the entry in `snapshots` is + keyed by the *referenced* model's fqn, which is normalized using that model's own dialect. + + When the two models use dialects with different identifier-casing rules (e.g. a + case-insensitive dialect like duckdb referencing a model whose fqn was computed under a + case-uppercasing dialect like snowflake), the raw string lookup can miss even though + `exp.replace_tables`'s own (dialect-aware) matching -- which is what ran before this + optimization, and which the narrowed lookup's own final `exp.replace_tables` call still + performs when the key IS found -- would have matched them. + """ + + @macro() + def resolve_named(evaluator, name): + return evaluator.resolve_table(name.name) + + # parent is declared/rendered under snowflake, which uppercases unquoted identifiers, so its + # fqn (the key that will appear in `snapshots`) is uppercase-quoted. + parent = load_sql_based_model( + d.parse("MODEL (name parent); SELECT 1 AS c"), dialect="snowflake" + ) + parent_snapshot = make_snapshot(parent) + parent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + assert parent.fqn == '"PARENT"' + + # child is declared/rendered under duckdb (case-insensitive), referencing `parent` in lowercase + child = load_sql_based_model( + d.parse( + """ + MODEL (name child); + SELECT c FROM parent; + @resolve_named('parent') + """ + ), + dialect="duckdb", + ) + + snapshots = {parent.fqn: parent_snapshot} + post_statements = child.render_post_statements(snapshots=snapshots) + + assert len(post_statements) == 1 + resolved_sql = post_statements[0].sql() + # BUG: if this assertion fails with the resolved name still literally "parent" (unmapped) + # instead of the physical table name, the narrowed single-snapshot lookup in `_resolve_table` + # failed to find `parent` in `snapshots` due to the cross-dialect casing mismatch between the + # lookup key and the dict key, even though the table legitimately exists in `snapshots`. + assert resolved_sql == f'"sqlmesh__default"."parent__{parent_snapshot.version}"', ( + f"expected parent to resolve to its physical table name, but got {resolved_sql!r} -- " + "this indicates the narrowed snapshots.get(table_name) lookup in _resolve_table missed " + "a snapshot that the old full-mapping + exp.replace_tables path would have matched" + ) + + +def test_resolve_table_table_mapping_only_no_snapshots(make_snapshot: t.Callable): + """A `table_mapping` entry with no corresponding `snapshots` entry should still be honored + by the narrowed lookup in `_resolve_table` (mirrors the override case in + `test_resolve_table_large_environment`, but with `snapshots=None`/empty entirely, to make + sure the narrowed code path doesn't assume `snapshots` is non-empty before consulting + `table_mapping`).""" + + @macro() + def resolve_named(evaluator, name): + return evaluator.resolve_table(name.name) + + child = load_sql_based_model( + d.parse( + """ + MODEL (name child); + SELECT 1 AS c; + @resolve_named('parent') + """ + ) + ) + + post_statements = child.render_post_statements( + snapshots=None, table_mapping={'"parent"': "explicit_physical_table"} + ) + assert post_statements[0].sql() == '"explicit_physical_table"' + + +def test_resolve_table_non_string_expr_path(make_snapshot: t.Callable): + """When `table_name` is an `exp.Expr` (not a `str`), `_resolve_table` falls back to building + the full snapshot mapping (the `else` branch of the new code). This exercises that branch -- + which the `this_model`/`resolve_table` macro call sites never hit, since they always pass a + pre-normalized string -- directly at the renderer level, to make sure it's still reachable + and correct, and not dead code that silently bit-rots.""" + + from sqlmesh.core.renderer import ExpressionRenderer + + parent = load_sql_based_model(d.parse("MODEL (name parent); SELECT 1 AS c")) + parent_snapshot = make_snapshot(parent) + parent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + other = load_sql_based_model(d.parse("MODEL (name other); SELECT 1 AS c")) + other_snapshot = make_snapshot(other) + other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + expr_renderer = ExpressionRenderer( + exp.select("*"), + dialect="", + macro_definitions=[], + path=Path("."), + ) + + table_expr = exp.to_table('"parent"') + resolved = expr_renderer._resolve_table( + table_expr, + snapshots={'"parent"': parent_snapshot, '"other"': other_snapshot}, + ) + assert ( + resolved.sql(comments=False) + == f'"sqlmesh__default"."parent__{parent_snapshot.version}"' + ) + + +def test_resolve_tables_table_ref_only_in_string_literal_not_expanded(make_snapshot: t.Callable): + """Adversarial case for the `expression.find(exp.Table)` short-circuit in `_resolve_tables`: + an expression that references a table only inside a string literal (not a parsed `exp.Table` + node) has no `exp.Table` node for `find()` to see, so the mapping build is correctly skipped. + This documents/locks in that the short-circuit is safe because `exp.replace_tables` itself + only ever rewrites `exp.Table` nodes -- it would never have touched a string literal either, + mapping built or not -- so skipping the mapping cannot change behavior here.""" + + parent = load_sql_based_model(d.parse("MODEL (name parent); SELECT 1 AS c")) + parent_snapshot = make_snapshot(parent) + parent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + model = load_sql_based_model( + d.parse( + """ + MODEL ( + name test_schema.string_ref_model, + virtual_properties ( + description = 'references parent as a plain string, not a table node' + ), + ); + SELECT a FROM tbl; + """ + ) + ) + + snapshots = {'"parent"': parent_snapshot} + props = model.render_virtual_properties(snapshots=snapshots) + assert ( + props["description"].this + == "references parent as a plain string, not a table node" + ) + + +def test_resolve_tables_expand_reveals_table_after_find_check(make_snapshot: t.Callable): + """Embedded-model expansion (`expand=`) runs as an `expression.transform` *before* the new + `expression.find(exp.Table)` short-circuit in `_resolve_tables`, so a table reference that + only exists *after* inlining an embedded model's query must still be seen by `find()` and + mapped. This locks in that ordering: `grandparent` is not a literal `exp.Table` node in + `child`'s original query -- it only appears once the embedded `mid` model is expanded -- and + must still resolve to its physical table name, not be silently skipped because it wasn't + present at the time `_resolve_tables` was first called.""" + + grandparent = load_sql_based_model(d.parse("MODEL (name grandparent); SELECT 1 AS c")) + grandparent_snapshot = make_snapshot(grandparent) + grandparent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + mid = load_sql_based_model( + d.parse("MODEL (name mid, kind EMBEDDED); SELECT c FROM grandparent;") + ) + mid_snapshot = make_snapshot(mid) + mid_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + child = load_sql_based_model(d.parse("MODEL (name child); SELECT c FROM mid;")) + + snapshots = {'"grandparent"': grandparent_snapshot, '"mid"': mid_snapshot} + query = child.render_query(snapshots=snapshots) + assert query is not None + rendered_sql = query.sql() + + # the physical table name for `grandparent` must appear -- if the find(exp.Table) check had + # run before expansion (or expansion didn't feed into it), `grandparent` would remain + # unmapped in the rendered output. + assert f"grandparent__{grandparent_snapshot.version}" in rendered_sql + assert "FROM grandparent" not in rendered_sql + + +def test_resolve_table_deployability_index_consistency(make_snapshot: t.Callable): + """The narrowed `_resolve_table` single-snapshot mapping must respect `deployability_index` + identically to the full-mapping path: a non-deployable (dev-preview) snapshot should map to + its dev table, not its deployable/prod table. + + A snapshot's dev table only differs from its prod table when `dev_version_` differs from + `version` (see `Snapshot._table_name`); that normally arises from a forward-only change + against a previous version. `SnapshotChangeCategory.FORWARD_ONLY` is deprecated/blocked by + `categorize_as`, so this sets `dev_version_` directly to force that condition deterministically + without relying on a deprecated code path. + """ + from sqlmesh.core.snapshot import DeployabilityIndex + + parent = load_sql_based_model( + d.parse("MODEL (name parent); SELECT 1 AS c"), + dialect="duckdb", + ) + parent_snapshot = make_snapshot(parent) + parent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + parent_snapshot.dev_version_ = "customdevversion123" + assert parent_snapshot.table_name(is_deployable=True) != parent_snapshot.table_name( + is_deployable=False + ) + + @macro() + def resolve_named(evaluator, name): + return evaluator.resolve_table(name.name) + + child_sql = """ + MODEL (name child); + SELECT 1 AS c; + @resolve_named('parent') + """ + + snapshots = {parent.fqn: parent_snapshot} + + # separate model instances per render call so the statement-render cache (keyed independent + # of `deployability_index`) doesn't just return the first call's cached result. + deployable_result = load_sql_based_model(d.parse(child_sql)).render_post_statements( + snapshots=snapshots, deployability_index=DeployabilityIndex.all_deployable() + )[0].sql() + non_deployable_result = load_sql_based_model(d.parse(child_sql)).render_post_statements( + snapshots=snapshots, + deployability_index=DeployabilityIndex.all_deployable().with_non_deployable( + parent_snapshot + ), + )[0].sql() + + # the narrowed single-snapshot mapping must still pick the right table for each index. + assert deployable_result != non_deployable_result + assert parent_snapshot.table_name(is_deployable=True) in deployable_result.replace('"', "") + assert parent_snapshot.table_name(is_deployable=False) in non_deployable_result.replace( + '"', "" + ) + + def test_cluster_with_complex_expression(): expressions = d.parse( """ From 5dbdf2f15943ae6532132ef5dfc771274f8dbb1b Mon Sep 17 00:00:00 2001 From: Michael Day Date: Mon, 14 Sep 2026 21:00:32 -0400 Subject: [PATCH 03/11] fix: two more gaps in the narrowed resolve_table/resolve_tables paths An independent review of the previous two commits found two more correctness/ performance gaps in the same narrowed-lookup change: 1. _resolve_table's dialect-reconciling fallback only triggered when `snapshots` was non-empty (`if snapshot is None and snapshots and table_name not in table_mapping`). When `snapshots` is None/empty - e.g. sqlmesh test's render_query_or_raise(table_mapping=...) call, which passes a table_mapping built under the project's dialect with no snapshots at all - a table_mapping key differing only in casing/quoting from the resolved name silently missed, instead of falling back to exp.replace_tables' own normalization like the pre-existing snapshots case does. Fixed by dropping the `and snapshots` condition so the fallback covers a miss in either dict. 2. _resolve_tables' "skip the mapping build when there's no table to replace" check ran after building the `expand` set and `model_mapping`, both of which are themselves O(N) in the number of snapshots (the `expand` set comprehension scans every snapshot's `is_embedded` flag unconditionally). So the claimed O(1)/no-op behavior for table-less expressions (virtual_properties, session_properties) was not actually achieved when the environment had any embedded models - the mapping build was skipped, but the O(N) expand-set scan was not. Moved the `expression.find(exp.Table)` check to the top of the function, before expand is computed, since an expression with no table node can't be affected by expand either. Adds test_resolve_table_table_mapping_only_dialect_mismatch (verifies fix 1 - fails on the prior commit, passes here) and test_resolve_tables_skips_expand_computation_without_table_refs (verifies fix 2 via a dict subclass that counts .items() calls on `snapshots`, asserting it's never called for a table-less expression even with an embedded snapshot present - also fails on the prior commit, passes here). Signed-off-by: Michael Day --- sqlmesh/core/renderer.py | 26 ++++++++----- tests/core/test_model.py | 80 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 9 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index 50c05dcc84..583566edef 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -338,16 +338,17 @@ def _resolve_table( # environment - building the full mapping made this call O(N) in the number of # snapshots in the environment for every table resolved. snapshot = snapshots.get(table_name) if snapshots else None - if snapshot is None and snapshots and table_name not in table_mapping: - # table_name is normalized under this renderer's own dialect, but a snapshot's - # fqn (the snapshots dict key) is normalized under that model's own dialect - - # these can disagree in casing when models use different dialects (e.g. a - # case-uppercasing dialect referenced from a case-insensitive one). A direct - # dict lookup can miss in that case even though the table is present, so fall - # back to the full, dialect-reconciling mapping that exp.replace_tables itself - # performs. This only pays the O(N) cost on a miss, not on every resolution. + if snapshot is None and table_name not in table_mapping: + # table_name is normalized under this renderer's own dialect, but a snapshots key + # is normalized under that model's own dialect and a table_mapping key may come + # from yet another dialect (e.g. a test fixture's table_mapping, normalized under + # the project's dialect) - these can disagree in casing/quoting even though an + # entry for this table exists in one of them. A direct dict lookup can miss in + # that case, so on a miss in both dicts, fall back to the full, dialect- + # reconciling mapping that exp.replace_tables itself performs. This only pays the + # O(N) cost on a miss, not on every resolution. mapping = { - **self._to_table_mapping(snapshots.values(), deployability_index), + **self._to_table_mapping((snapshots or {}).values(), deployability_index), **table_mapping, } else: @@ -395,6 +396,13 @@ def _resolve_tables( expression = expression.copy() with self._normalize_and_quote(expression) as expression: + # An expression with no exp.Table node at all (e.g. session/virtual properties) has + # nothing for `expand` to expand or for a table mapping to replace - skip building + # the expand set and model_mapping too, not just the mapping/replace_tables below, + # since both of those are themselves O(N) in the number of snapshots. + if not expression.find(exp.Table): + return expression + snapshots = snapshots or {} table_mapping = table_mapping or {} expand = set(expand) | { diff --git a/tests/core/test_model.py b/tests/core/test_model.py index df748d7c28..52e447190d 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -10104,6 +10104,86 @@ def resolve_named(evaluator, name): ) +def test_resolve_table_table_mapping_only_dialect_mismatch(make_snapshot: t.Callable): + """When `snapshots` is empty/None, `_resolve_table`'s narrowed lookup must still fall back to + the full, dialect-reconciling mapping on a miss - not just when `snapshots` is non-empty. + + `table_name` and a `table_mapping` key can be normalized under different dialects (e.g. a + unit-test `table_mapping` built from the project's dialect vs. a model's own dialect for the + macro-resolved name), so they can disagree in casing/quoting even though an entry for this + table exists. The old exp.replace_tables-based path reconciled this via its own + normalization; a raw `table_name in table_mapping` string-equality check does not. + """ + + @macro() + def resolve_named(evaluator, name): + return evaluator.resolve_table(name.name) + + child = load_sql_based_model( + d.parse( + """ + MODEL (name child); + SELECT 1 AS c; + @resolve_named('a.b') + """ + ) + ) + + # `table_mapping` key differs from the resolved name only in quoting - a raw dict lookup on + # `'"a"."b"'` would miss `'a.b'`, but exp.replace_tables' normalization matches them. + post_statements = child.render_post_statements( + snapshots=None, table_mapping={"a.b": "c"} + ) + assert post_statements[0].sql(comments=False) == '"c"' + + +def test_resolve_tables_skips_expand_computation_without_table_refs( + make_snapshot: t.Callable, +): + """Rendering a table-less expression (e.g. `virtual_properties`) must skip building the + `expand` set and `model_mapping` entirely, not just the final mapping/replace_tables call - + both of those are themselves O(N) in the number of snapshots when any snapshot is embedded, + so doing them for an expression with no `exp.Table` node at all defeats the point of skipping + the mapping build.""" + + embedded = load_sql_based_model( + d.parse("MODEL (name embedded, kind EMBEDDED); SELECT 1 AS c") + ) + embedded_snapshot = make_snapshot(embedded) + embedded_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + class ItemsCountingDict(dict): + items_call_count = 0 + + def items(self): + ItemsCountingDict.items_call_count += 1 + return super().items() + + snapshots = ItemsCountingDict({embedded.fqn: embedded_snapshot}) + + model = load_sql_based_model( + d.parse( + """ + MODEL ( + name test_schema.test_model, + virtual_properties ( + labels = [('team', 'data')] + ), + ); + SELECT a FROM tbl; + """ + ) + ) + + assert model.render_virtual_properties(snapshots=snapshots) == { + "labels": exp.maybe_parse("[('team', 'data')]") + } + # `_resolve_tables` computing `expand` (which scans `snapshots.items()` for embedded + # snapshots) and `model_mapping` must not happen for a table-less expression, even though + # this environment has an embedded snapshot that would otherwise trigger both. + assert ItemsCountingDict.items_call_count == 0 + + def test_cluster_with_complex_expression(): expressions = d.parse( """ From e3cf6b7c2741eb9f685f8e4b2b14d2745a762578 Mon Sep 17 00:00:00 2001 From: mday-io Date: Tue, 15 Sep 2026 14:33:54 -0400 Subject: [PATCH 04/11] fix: preserve normalized table mapping override precedence Signed-off-by: mday-io --- sqlmesh/core/renderer.py | 9 ++--- tests/core/test_model.py | 84 +++++++++++++++++++++++++++------------- 2 files changed, 62 insertions(+), 31 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index 583566edef..e32650553e 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -354,11 +354,10 @@ def _resolve_table( else: mapping = { **self._to_table_mapping([snapshot] if snapshot else [], deployability_index), - **( - {table_name: table_mapping[table_name]} - if table_name in table_mapping - else {} - ), + # Keep the complete explicit mapping so exp.replace_tables can preserve + # its dialect-aware matching and precedence for equivalent keys. This still + # avoids scanning the full snapshots environment. + **table_mapping, } else: mapping = { diff --git a/tests/core/test_model.py b/tests/core/test_model.py index 52e447190d..04153323a5 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -9818,6 +9818,42 @@ def resolve_named(evaluator, name): assert unmapped_result[0].sql() == '"does_not_exist"' +@pytest.mark.parametrize("include_exact_mapping", [False, True]) +def test_resolve_table_preserves_dialect_equivalent_table_mapping_override( + make_snapshot: t.Callable, include_exact_mapping: bool +): + """An explicit mapping should override a snapshot mapping when its key is dialect-equivalent + to the resolved table name, even when the snapshot lookup is an exact match.""" + + @macro() + def resolve_named(evaluator, name): + return evaluator.resolve_table(name.name) + + parent = load_sql_based_model(d.parse("MODEL (name parent); SELECT 1 AS c")) + parent_snapshot = make_snapshot(parent) + parent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + child = load_sql_based_model( + d.parse( + """ + MODEL (name child); + SELECT 1 AS c; + @resolve_named('parent') + """ + ) + ) + + table_mapping = {"parent": "override_table"} + if include_exact_mapping: + table_mapping = {parent.fqn: "earlier_table", **table_mapping} + + post_statements = child.render_post_statements( + snapshots={parent.fqn: parent_snapshot}, table_mapping=table_mapping + ) + + assert post_statements[0].sql() == '"override_table"' + + def test_render_virtual_properties_skips_mapping_without_table_refs( make_snapshot: t.Callable, mocker: MockerFixture ): @@ -9975,10 +10011,7 @@ def test_resolve_table_non_string_expr_path(make_snapshot: t.Callable): table_expr, snapshots={'"parent"': parent_snapshot, '"other"': other_snapshot}, ) - assert ( - resolved.sql(comments=False) - == f'"sqlmesh__default"."parent__{parent_snapshot.version}"' - ) + assert resolved.sql(comments=False) == f'"sqlmesh__default"."parent__{parent_snapshot.version}"' def test_resolve_tables_table_ref_only_in_string_literal_not_expanded(make_snapshot: t.Callable): @@ -10009,10 +10042,7 @@ def test_resolve_tables_table_ref_only_in_string_literal_not_expanded(make_snaps snapshots = {'"parent"': parent_snapshot} props = model.render_virtual_properties(snapshots=snapshots) - assert ( - props["description"].this - == "references parent as a plain string, not a table node" - ) + assert props["description"].this == "references parent as a plain string, not a table node" def test_resolve_tables_expand_reveals_table_after_find_check(make_snapshot: t.Callable): @@ -10086,22 +10116,28 @@ def resolve_named(evaluator, name): # separate model instances per render call so the statement-render cache (keyed independent # of `deployability_index`) doesn't just return the first call's cached result. - deployable_result = load_sql_based_model(d.parse(child_sql)).render_post_statements( - snapshots=snapshots, deployability_index=DeployabilityIndex.all_deployable() - )[0].sql() - non_deployable_result = load_sql_based_model(d.parse(child_sql)).render_post_statements( - snapshots=snapshots, - deployability_index=DeployabilityIndex.all_deployable().with_non_deployable( - parent_snapshot - ), - )[0].sql() + deployable_result = ( + load_sql_based_model(d.parse(child_sql)) + .render_post_statements( + snapshots=snapshots, deployability_index=DeployabilityIndex.all_deployable() + )[0] + .sql() + ) + non_deployable_result = ( + load_sql_based_model(d.parse(child_sql)) + .render_post_statements( + snapshots=snapshots, + deployability_index=DeployabilityIndex.all_deployable().with_non_deployable( + parent_snapshot + ), + )[0] + .sql() + ) # the narrowed single-snapshot mapping must still pick the right table for each index. assert deployable_result != non_deployable_result assert parent_snapshot.table_name(is_deployable=True) in deployable_result.replace('"', "") - assert parent_snapshot.table_name(is_deployable=False) in non_deployable_result.replace( - '"', "" - ) + assert parent_snapshot.table_name(is_deployable=False) in non_deployable_result.replace('"', "") def test_resolve_table_table_mapping_only_dialect_mismatch(make_snapshot: t.Callable): @@ -10131,9 +10167,7 @@ def resolve_named(evaluator, name): # `table_mapping` key differs from the resolved name only in quoting - a raw dict lookup on # `'"a"."b"'` would miss `'a.b'`, but exp.replace_tables' normalization matches them. - post_statements = child.render_post_statements( - snapshots=None, table_mapping={"a.b": "c"} - ) + post_statements = child.render_post_statements(snapshots=None, table_mapping={"a.b": "c"}) assert post_statements[0].sql(comments=False) == '"c"' @@ -10146,9 +10180,7 @@ def test_resolve_tables_skips_expand_computation_without_table_refs( so doing them for an expression with no `exp.Table` node at all defeats the point of skipping the mapping build.""" - embedded = load_sql_based_model( - d.parse("MODEL (name embedded, kind EMBEDDED); SELECT 1 AS c") - ) + embedded = load_sql_based_model(d.parse("MODEL (name embedded, kind EMBEDDED); SELECT 1 AS c")) embedded_snapshot = make_snapshot(embedded) embedded_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) From 2ee0fedb81908b173247c12623ac20e20193d2d6 Mon Sep 17 00:00:00 2001 From: mday-io Date: Sat, 19 Sep 2026 17:58:02 -0500 Subject: [PATCH 05/11] test: isolate cross-dialect table resolution settings Signed-off-by: mday-io --- tests/core/test_model.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/core/test_model.py b/tests/core/test_model.py index 04153323a5..c3c113f0d8 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -9914,6 +9914,11 @@ def test_resolve_table_cross_dialect_fqn_mismatch(make_snapshot: t.Callable): performs when the key IS found -- would have matched them. """ + # Use explicit per-model normalization settings so this regression is independent of + # mutable process-global SQLGlot dialect settings. + parent_dialect = "snowflake,normalization_strategy=uppercase" + child_dialect = "duckdb,normalization_strategy=case_insensitive" + @macro() def resolve_named(evaluator, name): return evaluator.resolve_table(name.name) @@ -9921,7 +9926,7 @@ def resolve_named(evaluator, name): # parent is declared/rendered under snowflake, which uppercases unquoted identifiers, so its # fqn (the key that will appear in `snapshots`) is uppercase-quoted. parent = load_sql_based_model( - d.parse("MODEL (name parent); SELECT 1 AS c"), dialect="snowflake" + d.parse("MODEL (name parent); SELECT 1 AS c"), dialect=parent_dialect ) parent_snapshot = make_snapshot(parent) parent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) @@ -9936,7 +9941,7 @@ def resolve_named(evaluator, name): @resolve_named('parent') """ ), - dialect="duckdb", + dialect=child_dialect, ) snapshots = {parent.fqn: parent_snapshot} From 07e2ddf47ae99a0a244ee915b919dd1d781736b1 Mon Sep 17 00:00:00 2001 From: mday-io Date: Sun, 20 Sep 2026 16:40:41 -0500 Subject: [PATCH 06/11] docs: shorten table resolution comments Signed-off-by: mday-io --- sqlmesh/core/renderer.py | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index e32650553e..a0e89ff505 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -332,21 +332,11 @@ def _resolve_table( ) -> exp.Table: table_mapping = table_mapping or {} if isinstance(table_name, str): - # table_name arrives here already normalized to a model FQN (see the `resolve_table` - # closure below and the `this_model` call site), the same key format `snapshots` and - # `table_mapping` use. Only the one relevant snapshot needs mapping, not the whole - # environment - building the full mapping made this call O(N) in the number of - # snapshots in the environment for every table resolved. + # An exact FQN match avoids scanning unrelated snapshots. snapshot = snapshots.get(table_name) if snapshots else None if snapshot is None and table_name not in table_mapping: - # table_name is normalized under this renderer's own dialect, but a snapshots key - # is normalized under that model's own dialect and a table_mapping key may come - # from yet another dialect (e.g. a test fixture's table_mapping, normalized under - # the project's dialect) - these can disagree in casing/quoting even though an - # entry for this table exists in one of them. A direct dict lookup can miss in - # that case, so on a miss in both dicts, fall back to the full, dialect- - # reconciling mapping that exp.replace_tables itself performs. This only pays the - # O(N) cost on a miss, not on every resolution. + # Keys normalized under different dialects may differ in casing or quoting. + # Fall back to the full mapping so exp.replace_tables can reconcile them. mapping = { **self._to_table_mapping((snapshots or {}).values(), deployability_index), **table_mapping, @@ -354,9 +344,7 @@ def _resolve_table( else: mapping = { **self._to_table_mapping([snapshot] if snapshot else [], deployability_index), - # Keep the complete explicit mapping so exp.replace_tables can preserve - # its dialect-aware matching and precedence for equivalent keys. This still - # avoids scanning the full snapshots environment. + # Keep all explicit overrides to preserve precedence for equivalent keys. **table_mapping, } else: From 406cd458fbc5c3c18697585c73974b68f04cbe36 Mon Sep 17 00:00:00 2001 From: mday-io Date: Thu, 24 Sep 2026 18:57:58 +0000 Subject: [PATCH 07/11] perf: resolve promotion properties against a pre-normalized view mapping Promotion passed snapshots keyed by SnapshotId into render_virtual_properties, which expects them keyed by model name. Snapshot lookups, @model_kind_name and embedded-model expansion therefore never worked in virtual properties during promotion. _promote_snapshot now re-keys them by name once and uses that dict for both virtual properties and on_virtual_update. to_view_mapping now returns a TableMapping, which normalizes its keys once per dialect. _resolve_table looks the table up in that index and passes only the matching entry to exp.replace_tables, instead of re-normalizing every model in the environment for each rendered property. The explicit mapping still takes precedence over snapshots, and among equivalent keys the last one still wins. Signed-off-by: mday-io --- sqlmesh/core/renderer.py | 108 ++++++++++++++++++------- sqlmesh/core/snapshot/definition.py | 14 ++-- sqlmesh/core/snapshot/evaluator.py | 12 ++- tests/core/test_model.py | 87 +++++++++++++++++++- tests/core/test_snapshot_evaluator.py | 109 ++++++++++++++++++++++++++ 5 files changed, 292 insertions(+), 38 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index a0e89ff505..0dc272b9c5 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -45,6 +45,65 @@ logger = logging.getLogger(__name__) +class TableMapping(t.Dict[str, str]): + """A table name mapping that caches the dialect-normalized form of its keys. + + `exp.replace_tables` normalizes every key of the mapping it's given, so resolving a single + table against a mapping of every model in an environment costs O(N). Resolving it against + this mapping costs a dictionary lookup, since each key is normalized once per dialect. + """ + + def __init__(self, *args: t.Any, **kwargs: t.Any): + super().__init__(*args, **kwargs) + self._normalized_keys: t.Dict[DialectType, t.Dict[str, str]] = {} + + def normalized_keys(self, dialect: DialectType) -> t.Dict[str, str]: + """Returns a mapping from each normalized key to the last key that normalizes to it.""" + normalized_keys = self._normalized_keys.get(dialect) + if normalized_keys is None: + normalized_keys = {exp.normalize_table_name(key, dialect=dialect): key for key in self} + self._normalized_keys[dialect] = normalized_keys + return normalized_keys + + def __setitem__(self, key: str, value: str) -> None: + self._normalized_keys.clear() + super().__setitem__(key, value) + + def __delitem__(self, key: str) -> None: + self._normalized_keys.clear() + super().__delitem__(key) + + def __ior__(self, other: t.Any) -> TableMapping: # type: ignore[override,misc] + self._normalized_keys.clear() + return super().__ior__(other) + + def update(self, *args: t.Any, **kwargs: t.Any) -> None: + self._normalized_keys.clear() + super().update(*args, **kwargs) + + def setdefault(self, key: str, default: str) -> str: # type: ignore[override] + self._normalized_keys.clear() + return super().setdefault(key, default) + + def pop(self, key: str, *args: t.Any) -> t.Any: + self._normalized_keys.clear() + return super().pop(key, *args) + + def popitem(self) -> t.Tuple[str, str]: + self._normalized_keys.clear() + return super().popitem() + + def clear(self) -> None: + self._normalized_keys.clear() + super().clear() + + +def _normalize_keys(mapping: t.Dict[str, str], dialect: DialectType) -> t.Dict[str, str]: + if isinstance(mapping, TableMapping): + return mapping.normalized_keys(dialect) + return {exp.normalize_table_name(key, dialect=dialect): key for key in mapping} + + class BaseExpressionRenderer: def __init__( self, @@ -330,35 +389,30 @@ def _resolve_table( table_mapping: t.Optional[t.Dict[str, str]] = None, deployability_index: t.Optional[DeployabilityIndex] = None, ) -> exp.Table: - table_mapping = table_mapping or {} - if isinstance(table_name, str): + table = t.cast( + exp.Table, exp.maybe_parse(table_name, into=exp.Table, dialect=self._dialect) + ) + + mapping: t.Dict[str, str] = {} + if table_mapping: + # An explicit mapping takes precedence over snapshots, so when one of its keys matches + # the table, that key alone decides the result. Among equivalent keys, the last wins. + key = _normalize_keys(table_mapping, self._dialect).get( + exp.normalize_table_name(table, dialect=self._dialect) + ) + if key is not None: + mapping = {key: table_mapping[key]} + + if not mapping and snapshots: # An exact FQN match avoids scanning unrelated snapshots. - snapshot = snapshots.get(table_name) if snapshots else None - if snapshot is None and table_name not in table_mapping: - # Keys normalized under different dialects may differ in casing or quoting. - # Fall back to the full mapping so exp.replace_tables can reconcile them. - mapping = { - **self._to_table_mapping((snapshots or {}).values(), deployability_index), - **table_mapping, - } - else: - mapping = { - **self._to_table_mapping([snapshot] if snapshot else [], deployability_index), - # Keep all explicit overrides to preserve precedence for equivalent keys. - **table_mapping, - } - else: - mapping = { - **self._to_table_mapping((snapshots or {}).values(), deployability_index), - **table_mapping, - } + snapshot = snapshots.get(table_name) if isinstance(table_name, str) else None + # Keys normalized under different dialects may differ in casing or quoting. + # Fall back to the full mapping so exp.replace_tables can reconcile them. + mapping = self._to_table_mapping( + [snapshot] if snapshot else snapshots.values(), deployability_index + ) - table = exp.replace_tables( - t.cast(exp.Table, exp.maybe_parse(table_name, into=exp.Table, dialect=self._dialect)), - mapping, - dialect=self._dialect, - copy=False, - ) + table = exp.replace_tables(table, mapping, dialect=self._dialect, copy=False) # We quote the table here to mimic the behavior of _resolve_tables, otherwise we may end # up normalizing twice, because _to_table_mapping returns the mapped names unquoted. return ( diff --git a/sqlmesh/core/snapshot/definition.py b/sqlmesh/core/snapshot/definition.py index 0c9635a7c2..2d813a06b1 100644 --- a/sqlmesh/core/snapshot/definition.py +++ b/sqlmesh/core/snapshot/definition.py @@ -24,6 +24,7 @@ from sqlmesh.core.model import Model, ModelKindMixin, ModelKindName, ViewKind, CustomKind from sqlmesh.core.model.definition import _Model from sqlmesh.core.node import IntervalUnit, NodeType +from sqlmesh.core.renderer import TableMapping from sqlmesh.utils import sanitize_name, unique from sqlmesh.utils.dag import DAG from sqlmesh.utils.date import ( @@ -2007,14 +2008,17 @@ def to_view_mapping( environment_naming_info: EnvironmentNamingInfo, default_catalog: t.Optional[str] = None, dialect: t.Optional[str] = None, -) -> t.Dict[str, str]: - return { - snapshot.name: snapshot.display_name( - environment_naming_info, default_catalog=default_catalog, dialect=dialect +) -> TableMapping: + return TableMapping( + ( + snapshot.name, + snapshot.display_name( + environment_naming_info, default_catalog=default_catalog, dialect=dialect + ), ) for snapshot in snapshots if snapshot.is_model - } + ) def has_paused_forward_only( diff --git a/sqlmesh/core/snapshot/evaluator.py b/sqlmesh/core/snapshot/evaluator.py index ad935310f4..3e77d6a640 100644 --- a/sqlmesh/core/snapshot/evaluator.py +++ b/sqlmesh/core/snapshot/evaluator.py @@ -1284,6 +1284,8 @@ def _promote_snapshot( table_mapping=table_mapping, runtime_stage=RuntimeStage.PROMOTING, ) + # Renderers look snapshots up by model name, not by SnapshotId. + snapshots_by_name = {s.name: s for s in (snapshots or {}).values()} with ( adapter.transaction(), @@ -1294,14 +1296,16 @@ def _promote_snapshot( view_name=view_name, model=snapshot.model, environment=environment_naming_info.name, - snapshots=snapshots, + snapshots=snapshots_by_name, snapshot=snapshot, **render_kwargs, ) - snapshot_by_name = {s.name: s for s in (snapshots or {}).values()} - render_kwargs["snapshots"] = snapshot_by_name - adapter.execute(snapshot.model.render_on_virtual_update(**render_kwargs)) + adapter.execute( + snapshot.model.render_on_virtual_update( + snapshots=snapshots_by_name, **render_kwargs + ) + ) if on_complete is not None: on_complete(snapshot) diff --git a/tests/core/test_model.py b/tests/core/test_model.py index c3c113f0d8..fb95281267 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -15,6 +15,7 @@ from sqlglot.schema import MappingSchema from sqlmesh.cli.project_init import init_example_project, ProjectTemplate from sqlmesh.core.environment import EnvironmentNamingInfo +from sqlmesh.core.renderer import TableMapping from sqlmesh.core.model.kind import TimeColumn, ModelKindName, SeedKind from sqlmesh import CustomMaterialization, CustomKind @@ -9818,9 +9819,10 @@ def resolve_named(evaluator, name): assert unmapped_result[0].sql() == '"does_not_exist"' +@pytest.mark.parametrize("mapping_type", [dict, TableMapping]) @pytest.mark.parametrize("include_exact_mapping", [False, True]) def test_resolve_table_preserves_dialect_equivalent_table_mapping_override( - make_snapshot: t.Callable, include_exact_mapping: bool + make_snapshot: t.Callable, include_exact_mapping: bool, mapping_type: t.Callable ): """An explicit mapping should override a snapshot mapping when its key is dialect-equivalent to the resolved table name, even when the snapshot lookup is an exact match.""" @@ -9848,7 +9850,7 @@ def resolve_named(evaluator, name): table_mapping = {parent.fqn: "earlier_table", **table_mapping} post_statements = child.render_post_statements( - snapshots={parent.fqn: parent_snapshot}, table_mapping=table_mapping + snapshots={parent.fqn: parent_snapshot}, table_mapping=mapping_type(table_mapping) ) assert post_statements[0].sql() == '"override_table"' @@ -10221,6 +10223,87 @@ def items(self): assert ItemsCountingDict.items_call_count == 0 +def test_resolve_table_with_view_mapping_uses_single_entry( + make_snapshot: t.Callable, mocker: MockerFixture +): + """During promotion `table_mapping` maps every model in the environment to its view. Resolving + one table against it must not normalize every key in that mapping on every call + (https://github.com/SQLMesh/sqlmesh/issues/6017).""" + from sqlmesh.core.snapshot.definition import to_view_mapping + + @macro() + def resolve_named(evaluator, name): + return evaluator.resolve_table(name.name) + + snapshots = {} + for i in range(50): + other = load_sql_based_model(d.parse(f"MODEL (name db.other_{i}); SELECT 1 AS c")) + other_snapshot = make_snapshot(other) + other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + snapshots[other.fqn] = other_snapshot + + children = [ + load_sql_based_model( + d.parse( + f""" + MODEL (name db.child_{i}); + SELECT 1 AS c; + @resolve_named('db.other_{i}') + """ + ) + ) + for i in range(3) + ] + for child in children: + child_snapshot = make_snapshot(child) + child_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + snapshots[child.fqn] = child_snapshot + + table_mapping = to_view_mapping(snapshots.values(), EnvironmentNamingInfo(name="dev")) + spy = mocker.spy(exp, "replace_tables") + + for i, child in enumerate(children): + rendered = child.render_post_statements(snapshots=snapshots, table_mapping=table_mapping) + assert rendered[0].sql() == f'"db__dev"."other_{i}"' + + # One call for `this_model` and one for the resolved table, per child. + assert spy.call_count == 6 + for call in spy.call_args_list: + assert len(call.args[1]) == 1 + + +@pytest.mark.parametrize("dialect", ["duckdb", "snowflake"]) +def test_table_mapping_normalized_keys(dialect: str): + table_mapping = TableMapping({'"db"."a"': "view_a", "db.A": "view_a_upper"}) + + normalized = table_mapping.normalized_keys(dialect) + # Keys that normalize to the same name resolve to the last one, like exp.replace_tables. + if dialect == "snowflake": + assert normalized == {"db.a": '"db"."a"', "DB.A": "db.A"} + else: + assert normalized == {"db.a": "db.A"} + # Normalization happens once per dialect. + assert table_mapping.normalized_keys(dialect) is normalized + + # Every mutation invalidates the cache. + table_mapping["db.b"] = "view_b" + assert "db.b" in table_mapping.normalized_keys("duckdb") + table_mapping.update({"db.c": "view_c"}) + assert "db.c" in table_mapping.normalized_keys("duckdb") + table_mapping.setdefault("db.d", "view_d") + assert "db.d" in table_mapping.normalized_keys("duckdb") + table_mapping |= {"db.e": "view_e"} + assert "db.e" in table_mapping.normalized_keys("duckdb") + del table_mapping["db.b"] + assert "db.b" not in table_mapping.normalized_keys("duckdb") + table_mapping.pop("db.c") + assert "db.c" not in table_mapping.normalized_keys("duckdb") + table_mapping.popitem() + assert "db.e" not in table_mapping.normalized_keys("duckdb") + table_mapping.clear() + assert table_mapping.normalized_keys("duckdb") == {} + + def test_cluster_with_complex_expression(): expressions = d.parse( """ diff --git a/tests/core/test_snapshot_evaluator.py b/tests/core/test_snapshot_evaluator.py index 3950e0d53f..8f921bfa86 100644 --- a/tests/core/test_snapshot_evaluator.py +++ b/tests/core/test_snapshot_evaluator.py @@ -5656,3 +5656,112 @@ def test_grants_in_production_with_dev_only_vde( # Should still apply grants to physical table when target layer is ALL or PHYSICAL sync_grants_mock.assert_called_once() assert sync_grants_mock.call_args[0][1] == {"select": ["user1"], "insert": ["role1"]} + + +def test_promote_virtual_properties_see_snapshots_by_name(mocker: MockerFixture, make_snapshot): + """Promotion receives snapshots keyed by SnapshotId, but renderers expect them keyed by name. + Virtual properties must see the same name-keyed snapshots as `on_virtual_update` does.""" + + @macro() + def upstream_version(evaluator): + upstream = evaluator.snapshots.get('"test_schema"."upstream"') + return exp.Literal.string(upstream.version if upstream else "missing") + + @macro() + def local_or_missing(evaluator, name): + value = evaluator.locals.get(name.name) + if isinstance(value, exp.Expr): + value = value.sql(evaluator.dialect, comments=False) + return exp.Literal.string(value or "missing") + + adapter_mock = mocker.patch("sqlmesh.core.engine_adapter.EngineAdapter") + adapter_mock.dialect = "duckdb" + adapter_mock.with_settings.return_value = adapter_mock + evaluator = SnapshotEvaluator(adapter_mock) + + upstream = load_sql_based_model( + parse("MODEL (name test_schema.upstream, kind FULL); SELECT 1 AS a") + ) + upstream_snapshot = make_snapshot(upstream) + upstream_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + model = load_sql_based_model( + parse( + """ + MODEL ( + name test_schema.test_model, + kind FULL, + virtual_properties ( + upstream_version = @upstream_version(), + kind_name = @local_or_missing('model_kind_name'), + this_view = @local_or_missing('this_model'), + ), + ); + SELECT a FROM test_schema.upstream + """ + ) + ) + snapshot = make_snapshot(model, nodes={upstream.fqn: upstream}) + snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + + snapshots = {s.snapshot_id: s for s in (upstream_snapshot, snapshot)} + environment_naming_info = EnvironmentNamingInfo(name="test_env") + evaluator.promote( + [snapshot], + environment_naming_info, + snapshots=snapshots, + table_mapping=to_view_mapping(snapshots.values(), environment_naming_info), + ) + + view_properties = adapter_mock.create_view.call_args.kwargs["view_properties"] + assert view_properties["upstream_version"] == exp.Literal.string(upstream_snapshot.version) + assert view_properties["kind_name"] == exp.Literal.string("FULL") + # The environment view mapping still takes precedence over the physical table. + assert view_properties["this_view"] == exp.Literal.string( + '"test_schema__test_env"."test_model"' + ) + + +def test_promote_resolves_this_model_with_single_mapping_entry( + mocker: MockerFixture, make_snapshot +): + """Rendering a promoted view's properties must not re-normalize the whole environment's view + mapping for every view (https://github.com/SQLMesh/sqlmesh/issues/6017).""" + adapter_mock = mocker.patch("sqlmesh.core.engine_adapter.EngineAdapter") + adapter_mock.dialect = "duckdb" + adapter_mock.with_settings.return_value = adapter_mock + evaluator = SnapshotEvaluator(adapter_mock) + + snapshots = {} + for i in range(20): + model = load_sql_based_model( + parse( + f""" + MODEL ( + name test_schema.model_{i}, + kind FULL, + virtual_properties (description = 'model {i}'), + ); + SELECT 1 AS a + """ + ) + ) + snapshot = make_snapshot(model) + snapshot.categorize_as(SnapshotChangeCategory.BREAKING) + snapshots[snapshot.snapshot_id] = snapshot + + environment_naming_info = EnvironmentNamingInfo(name="test_env") + table_mapping = to_view_mapping(snapshots.values(), environment_naming_info) + spy = mocker.spy(exp, "replace_tables") + + evaluator.promote( + list(snapshots.values()), + environment_naming_info, + snapshots=snapshots, + table_mapping=table_mapping, + ) + + assert adapter_mock.create_view.call_count == 20 + assert spy.call_count >= 20 + for call in spy.call_args_list: + assert len(call.args[1]) == 1 From abad0e1fba917a651242e3b137de9776d7516299 Mon Sep 17 00:00:00 2001 From: mday-io Date: Thu, 24 Sep 2026 18:57:58 +0000 Subject: [PATCH 08/11] refactor: key promotion snapshots by name once per promotion Build the name-keyed snapshots dict once in SnapshotEvaluator.promote instead of once per promoted view, keep the TableMapping cache across copy(), and pin the number of replace_tables calls made during promotion. Signed-off-by: mday-io --- sqlmesh/core/renderer.py | 4 ++++ sqlmesh/core/snapshot/evaluator.py | 14 ++++++-------- tests/core/test_model.py | 3 +++ tests/core/test_snapshot_evaluator.py | 3 ++- 4 files changed, 15 insertions(+), 9 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index 0dc272b9c5..62081142f7 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -65,6 +65,10 @@ def normalized_keys(self, dialect: DialectType) -> t.Dict[str, str]: self._normalized_keys[dialect] = normalized_keys return normalized_keys + def copy(self) -> TableMapping: + # dict.copy() would return a plain dict and lose the cache. + return TableMapping(self) + def __setitem__(self, key: str, value: str) -> None: self._normalized_keys.clear() super().__setitem__(key, value) diff --git a/sqlmesh/core/snapshot/evaluator.py b/sqlmesh/core/snapshot/evaluator.py index 3e77d6a640..9d9ecf7672 100644 --- a/sqlmesh/core/snapshot/evaluator.py +++ b/sqlmesh/core/snapshot/evaluator.py @@ -312,6 +312,8 @@ def promote( self._get_virtual_data_objects(target_snapshots, environment_naming_info) deployability_index = deployability_index or DeployabilityIndex.all_deployable() + # Renderers look snapshots up by model name, not by SnapshotId. + snapshots_by_name = {s.name: s for s in (snapshots or {}).values()} with self.concurrent_context(): concurrent_apply_to_snapshots( target_snapshots, @@ -320,7 +322,7 @@ def promote( start=start, end=end, execution_time=execution_time, - snapshots=snapshots, + snapshots=snapshots_by_name, table_mapping=table_mapping, environment_naming_info=environment_naming_info, deployability_index=deployability_index, # type: ignore @@ -1260,7 +1262,7 @@ def _promote_snapshot( start: t.Optional[TimeLike] = None, end: t.Optional[TimeLike] = None, execution_time: t.Optional[TimeLike] = None, - snapshots: t.Optional[t.Dict[SnapshotId, Snapshot]] = None, + snapshots: t.Optional[t.Dict[str, Snapshot]] = None, table_mapping: t.Optional[t.Dict[str, str]] = None, ) -> None: if not snapshot.is_model: @@ -1284,8 +1286,6 @@ def _promote_snapshot( table_mapping=table_mapping, runtime_stage=RuntimeStage.PROMOTING, ) - # Renderers look snapshots up by model name, not by SnapshotId. - snapshots_by_name = {s.name: s for s in (snapshots or {}).values()} with ( adapter.transaction(), @@ -1296,15 +1296,13 @@ def _promote_snapshot( view_name=view_name, model=snapshot.model, environment=environment_naming_info.name, - snapshots=snapshots_by_name, + snapshots=snapshots, snapshot=snapshot, **render_kwargs, ) adapter.execute( - snapshot.model.render_on_virtual_update( - snapshots=snapshots_by_name, **render_kwargs - ) + snapshot.model.render_on_virtual_update(snapshots=snapshots, **render_kwargs) ) if on_complete is not None: diff --git a/tests/core/test_model.py b/tests/core/test_model.py index fb95281267..da863a16ce 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -10285,6 +10285,9 @@ def test_table_mapping_normalized_keys(dialect: str): # Normalization happens once per dialect. assert table_mapping.normalized_keys(dialect) is normalized + assert isinstance(table_mapping.copy(), TableMapping) + assert table_mapping.copy() == table_mapping + # Every mutation invalidates the cache. table_mapping["db.b"] = "view_b" assert "db.b" in table_mapping.normalized_keys("duckdb") diff --git a/tests/core/test_snapshot_evaluator.py b/tests/core/test_snapshot_evaluator.py index 8f921bfa86..78c78ea8db 100644 --- a/tests/core/test_snapshot_evaluator.py +++ b/tests/core/test_snapshot_evaluator.py @@ -5762,6 +5762,7 @@ def test_promote_resolves_this_model_with_single_mapping_entry( ) assert adapter_mock.create_view.call_count == 20 - assert spy.call_count >= 20 + # One call per view, to resolve `this_model`. + assert spy.call_count == 20 for call in spy.call_args_list: assert len(call.args[1]) == 1 From 21adbfb6775c1ab2267adba990326744bd41805d Mon Sep 17 00:00:00 2001 From: mday-io Date: Thu, 24 Sep 2026 19:15:03 +0000 Subject: [PATCH 09/11] test: prune redundant table resolution tests Remove tests that duplicate stronger coverage or guard nothing a credible regression would break: the table_mapping-only lookup already covered by the dialect-mismatch case, the string-literal property case, the model-level single-entry check already covered at the promotion boundary, and the mapping-skip check that only guarded a redundant second find(exp.Table). Drop that second check from _resolve_tables, build the mapping in the same place as before this change, drop the unused TableMapping.copy() override, and fold the per-dialect normalized-keys cases into one test. Signed-off-by: mday-io --- sqlmesh/core/renderer.py | 34 +++---- tests/core/test_model.py | 187 ++------------------------------------- 2 files changed, 16 insertions(+), 205 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index 62081142f7..ffaf616357 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -65,10 +65,6 @@ def normalized_keys(self, dialect: DialectType) -> t.Dict[str, str]: self._normalized_keys[dialect] = normalized_keys return normalized_keys - def copy(self) -> TableMapping: - # dict.copy() would return a plain dict and lose the cache. - return TableMapping(self) - def __setitem__(self, key: str, value: str) -> None: self._normalized_keys.clear() super().__setitem__(key, value) @@ -441,15 +437,17 @@ def _resolve_tables( expression = expression.copy() with self._normalize_and_quote(expression) as expression: - # An expression with no exp.Table node at all (e.g. session/virtual properties) has - # nothing for `expand` to expand or for a table mapping to replace - skip building - # the expand set and model_mapping too, not just the mapping/replace_tables below, - # since both of those are themselves O(N) in the number of snapshots. + # An expression with no table (e.g. most session or virtual properties) has nothing + # to expand or replace, so skip building the O(N) expand set and mapping. if not expression.find(exp.Table): return expression snapshots = snapshots or {} table_mapping = table_mapping or {} + mapping = { + **self._to_table_mapping(snapshots.values(), deployability_index), + **table_mapping, + } expand = set(expand) | { name for name, snapshot in snapshots.items() if snapshot.is_embedded } @@ -491,22 +489,10 @@ def _expand(node: exp.Expr) -> exp.Expr: expression = expression.transform(_expand, copy=False) # type: ignore - # Building the full snapshot -> table-name mapping and normalizing it in - # exp.replace_tables is O(N) in the number of snapshots in the environment; skip it - # entirely for expressions that don't reference any table at all (e.g. session/ - # virtual properties), since there's nothing for the mapping to replace. - if expression.find(exp.Table): - # mypy loses the `snapshots`/`table_mapping` narrowing above because they're - # captured by the `_expand` closure defined earlier in this block. - assert snapshots is not None and table_mapping is not None - mapping = { - **self._to_table_mapping(snapshots.values(), deployability_index), - **table_mapping, - } - if mapping: - expression = exp.replace_tables( - expression, mapping, dialect=self._dialect, copy=False - ) + if mapping: + expression = exp.replace_tables( + expression, mapping, dialect=self._dialect, copy=False + ) return expression diff --git a/tests/core/test_model.py b/tests/core/test_model.py index da863a16ce..7eb21c2478 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -9789,22 +9789,6 @@ def resolve_named(evaluator, name): for call in spy.call_args_list: assert len(call.args[1]) <= 1 - # an explicit table_mapping entry takes precedence over the snapshot-derived one (rendered - # via a separate model instance so the statement-render cache doesn't return the earlier result) - child_for_override = load_sql_based_model( - d.parse( - """ - MODEL (name child_override); - SELECT c FROM target; - @resolve_named('target') - """ - ) - ) - override = child_for_override.render_post_statements( - snapshots=snapshots, table_mapping={'"target"': "overridden_table"} - ) - assert override[0].sql() == '"overridden_table"' - # a name absent from both snapshots and table_mapping resolves unchanged unmapped = load_sql_based_model( d.parse( @@ -9856,52 +9840,6 @@ def resolve_named(evaluator, name): assert post_statements[0].sql() == '"override_table"' -def test_render_virtual_properties_skips_mapping_without_table_refs( - make_snapshot: t.Callable, mocker: MockerFixture -): - """Rendering a property expression with no table references shouldn't build the full - snapshot -> table-name mapping at all (https://github.com/SQLMesh/sqlmesh/issues/6017).""" - import sqlmesh.core.snapshot as snapshot_module - - model = load_sql_based_model( - d.parse( - """ - MODEL ( - name test_schema.test_model, - virtual_properties ( - labels = [('team', 'data')] - ), - session_properties ( - "spark.executor.memory" = '1G' - ), - ); - SELECT a FROM tbl; - """ - ) - ) - - snapshots = {} - for i in range(50): - other = load_sql_based_model(d.parse(f"MODEL (name other_{i}); SELECT 1 AS c")) - other_snapshot = make_snapshot(other) - other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) - snapshots[f'"other_{i}"'] = other_snapshot - - to_table_mapping_spy = mocker.spy(snapshot_module, "to_table_mapping") - - assert model.render_virtual_properties(snapshots=snapshots) == { - "labels": exp.maybe_parse("[('team', 'data')]") - } - assert model.render_session_properties(snapshots=snapshots) == { - "spark.executor.memory": "1G", - } - - # `this_model` resolution may still make a narrow, single-snapshot (or empty) call, but the - # full N-snapshot mapping build in `_resolve_tables` must never fire for a table-less property - for call in to_table_mapping_spy.call_args_list: - assert len(call.args[0]) <= 1 - - def test_resolve_table_cross_dialect_fqn_mismatch(make_snapshot: t.Callable): """`_resolve_table`'s narrowed lookup keys `snapshots` by the caller's already-normalized `table_name` string. That string is built with the *referencing* model's own dialect @@ -9962,33 +9900,6 @@ def resolve_named(evaluator, name): ) -def test_resolve_table_table_mapping_only_no_snapshots(make_snapshot: t.Callable): - """A `table_mapping` entry with no corresponding `snapshots` entry should still be honored - by the narrowed lookup in `_resolve_table` (mirrors the override case in - `test_resolve_table_large_environment`, but with `snapshots=None`/empty entirely, to make - sure the narrowed code path doesn't assume `snapshots` is non-empty before consulting - `table_mapping`).""" - - @macro() - def resolve_named(evaluator, name): - return evaluator.resolve_table(name.name) - - child = load_sql_based_model( - d.parse( - """ - MODEL (name child); - SELECT 1 AS c; - @resolve_named('parent') - """ - ) - ) - - post_statements = child.render_post_statements( - snapshots=None, table_mapping={'"parent"': "explicit_physical_table"} - ) - assert post_statements[0].sql() == '"explicit_physical_table"' - - def test_resolve_table_non_string_expr_path(make_snapshot: t.Callable): """When `table_name` is an `exp.Expr` (not a `str`), `_resolve_table` falls back to building the full snapshot mapping (the `else` branch of the new code). This exercises that branch -- @@ -10021,37 +9932,6 @@ def test_resolve_table_non_string_expr_path(make_snapshot: t.Callable): assert resolved.sql(comments=False) == f'"sqlmesh__default"."parent__{parent_snapshot.version}"' -def test_resolve_tables_table_ref_only_in_string_literal_not_expanded(make_snapshot: t.Callable): - """Adversarial case for the `expression.find(exp.Table)` short-circuit in `_resolve_tables`: - an expression that references a table only inside a string literal (not a parsed `exp.Table` - node) has no `exp.Table` node for `find()` to see, so the mapping build is correctly skipped. - This documents/locks in that the short-circuit is safe because `exp.replace_tables` itself - only ever rewrites `exp.Table` nodes -- it would never have touched a string literal either, - mapping built or not -- so skipping the mapping cannot change behavior here.""" - - parent = load_sql_based_model(d.parse("MODEL (name parent); SELECT 1 AS c")) - parent_snapshot = make_snapshot(parent) - parent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) - - model = load_sql_based_model( - d.parse( - """ - MODEL ( - name test_schema.string_ref_model, - virtual_properties ( - description = 'references parent as a plain string, not a table node' - ), - ); - SELECT a FROM tbl; - """ - ) - ) - - snapshots = {'"parent"': parent_snapshot} - props = model.render_virtual_properties(snapshots=snapshots) - assert props["description"].this == "references parent as a plain string, not a table node" - - def test_resolve_tables_expand_reveals_table_after_find_check(make_snapshot: t.Callable): """Embedded-model expansion (`expand=`) runs as an `expression.transform` *before* the new `expression.find(exp.Table)` short-circuit in `_resolve_tables`, so a table reference that @@ -10223,70 +10103,15 @@ def items(self): assert ItemsCountingDict.items_call_count == 0 -def test_resolve_table_with_view_mapping_uses_single_entry( - make_snapshot: t.Callable, mocker: MockerFixture -): - """During promotion `table_mapping` maps every model in the environment to its view. Resolving - one table against it must not normalize every key in that mapping on every call - (https://github.com/SQLMesh/sqlmesh/issues/6017).""" - from sqlmesh.core.snapshot.definition import to_view_mapping - - @macro() - def resolve_named(evaluator, name): - return evaluator.resolve_table(name.name) - - snapshots = {} - for i in range(50): - other = load_sql_based_model(d.parse(f"MODEL (name db.other_{i}); SELECT 1 AS c")) - other_snapshot = make_snapshot(other) - other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) - snapshots[other.fqn] = other_snapshot - - children = [ - load_sql_based_model( - d.parse( - f""" - MODEL (name db.child_{i}); - SELECT 1 AS c; - @resolve_named('db.other_{i}') - """ - ) - ) - for i in range(3) - ] - for child in children: - child_snapshot = make_snapshot(child) - child_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) - snapshots[child.fqn] = child_snapshot - - table_mapping = to_view_mapping(snapshots.values(), EnvironmentNamingInfo(name="dev")) - spy = mocker.spy(exp, "replace_tables") - - for i, child in enumerate(children): - rendered = child.render_post_statements(snapshots=snapshots, table_mapping=table_mapping) - assert rendered[0].sql() == f'"db__dev"."other_{i}"' - - # One call for `this_model` and one for the resolved table, per child. - assert spy.call_count == 6 - for call in spy.call_args_list: - assert len(call.args[1]) == 1 - - -@pytest.mark.parametrize("dialect", ["duckdb", "snowflake"]) -def test_table_mapping_normalized_keys(dialect: str): +def test_table_mapping_normalized_keys(): table_mapping = TableMapping({'"db"."a"': "view_a", "db.A": "view_a_upper"}) - normalized = table_mapping.normalized_keys(dialect) # Keys that normalize to the same name resolve to the last one, like exp.replace_tables. - if dialect == "snowflake": - assert normalized == {"db.a": '"db"."a"', "DB.A": "db.A"} - else: - assert normalized == {"db.a": "db.A"} - # Normalization happens once per dialect. - assert table_mapping.normalized_keys(dialect) is normalized - - assert isinstance(table_mapping.copy(), TableMapping) - assert table_mapping.copy() == table_mapping + duckdb_keys = table_mapping.normalized_keys("duckdb") + assert duckdb_keys == {"db.a": "db.A"} + # Normalization happens once per dialect, and each dialect gets its own normalization. + assert table_mapping.normalized_keys("duckdb") is duckdb_keys + assert table_mapping.normalized_keys("snowflake") == {"db.a": '"db"."a"', "DB.A": "db.A"} # Every mutation invalidates the cache. table_mapping["db.b"] = "view_b" From 8c6284df016627d9a8d7b33c0cdce5783a0c00c7 Mon Sep 17 00:00:00 2001 From: mday-io Date: Thu, 24 Sep 2026 19:24:19 +0000 Subject: [PATCH 10/11] refactor: accept only table name strings in _resolve_table Every caller of BaseExpressionRenderer._resolve_table passes a normalized model name string: this_model resolution passes the model's FQN, and the resolve_table macro normalizes its argument with normalize_model_name first. The exp.Expr branch was reachable only from a test that called the private method directly, so narrow the signature to str and delete that test. Signed-off-by: mday-io --- sqlmesh/core/renderer.py | 4 ++-- tests/core/test_model.py | 32 -------------------------------- 2 files changed, 2 insertions(+), 34 deletions(-) diff --git a/sqlmesh/core/renderer.py b/sqlmesh/core/renderer.py index ffaf616357..a2660a435e 100644 --- a/sqlmesh/core/renderer.py +++ b/sqlmesh/core/renderer.py @@ -384,7 +384,7 @@ def update_cache(self, expression: t.Optional[exp.Expr]) -> None: def _resolve_table( self, - table_name: str | exp.Expr, + table_name: str, snapshots: t.Optional[t.Dict[str, Snapshot]] = None, table_mapping: t.Optional[t.Dict[str, str]] = None, deployability_index: t.Optional[DeployabilityIndex] = None, @@ -405,7 +405,7 @@ def _resolve_table( if not mapping and snapshots: # An exact FQN match avoids scanning unrelated snapshots. - snapshot = snapshots.get(table_name) if isinstance(table_name, str) else None + snapshot = snapshots.get(table_name) # Keys normalized under different dialects may differ in casing or quoting. # Fall back to the full mapping so exp.replace_tables can reconcile them. mapping = self._to_table_mapping( diff --git a/tests/core/test_model.py b/tests/core/test_model.py index 7eb21c2478..c5d1a71bee 100644 --- a/tests/core/test_model.py +++ b/tests/core/test_model.py @@ -9900,38 +9900,6 @@ def resolve_named(evaluator, name): ) -def test_resolve_table_non_string_expr_path(make_snapshot: t.Callable): - """When `table_name` is an `exp.Expr` (not a `str`), `_resolve_table` falls back to building - the full snapshot mapping (the `else` branch of the new code). This exercises that branch -- - which the `this_model`/`resolve_table` macro call sites never hit, since they always pass a - pre-normalized string -- directly at the renderer level, to make sure it's still reachable - and correct, and not dead code that silently bit-rots.""" - - from sqlmesh.core.renderer import ExpressionRenderer - - parent = load_sql_based_model(d.parse("MODEL (name parent); SELECT 1 AS c")) - parent_snapshot = make_snapshot(parent) - parent_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) - - other = load_sql_based_model(d.parse("MODEL (name other); SELECT 1 AS c")) - other_snapshot = make_snapshot(other) - other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) - - expr_renderer = ExpressionRenderer( - exp.select("*"), - dialect="", - macro_definitions=[], - path=Path("."), - ) - - table_expr = exp.to_table('"parent"') - resolved = expr_renderer._resolve_table( - table_expr, - snapshots={'"parent"': parent_snapshot, '"other"': other_snapshot}, - ) - assert resolved.sql(comments=False) == f'"sqlmesh__default"."parent__{parent_snapshot.version}"' - - def test_resolve_tables_expand_reveals_table_after_find_check(make_snapshot: t.Callable): """Embedded-model expansion (`expand=`) runs as an `expression.transform` *before* the new `expression.find(exp.Table)` short-circuit in `_resolve_tables`, so a table reference that From d98d81767be6036cbb1ccc8c8e20ab414972dff7 Mon Sep 17 00:00:00 2001 From: mday-io Date: Thu, 24 Sep 2026 20:01:18 +0000 Subject: [PATCH 11/11] test: parse promotion test models with the SQLMesh dialect parser sqlglot's parse returns list[Expr | None], which mypy rejects as an argument to load_sql_based_model. Use d.parse, which is typed list[Expr]. Signed-off-by: mday-io --- tests/core/test_snapshot_evaluator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/core/test_snapshot_evaluator.py b/tests/core/test_snapshot_evaluator.py index 78c78ea8db..a9ca86496a 100644 --- a/tests/core/test_snapshot_evaluator.py +++ b/tests/core/test_snapshot_evaluator.py @@ -5680,13 +5680,13 @@ def local_or_missing(evaluator, name): evaluator = SnapshotEvaluator(adapter_mock) upstream = load_sql_based_model( - parse("MODEL (name test_schema.upstream, kind FULL); SELECT 1 AS a") + d.parse("MODEL (name test_schema.upstream, kind FULL); SELECT 1 AS a") ) upstream_snapshot = make_snapshot(upstream) upstream_snapshot.categorize_as(SnapshotChangeCategory.BREAKING) model = load_sql_based_model( - parse( + d.parse( """ MODEL ( name test_schema.test_model, @@ -5735,7 +5735,7 @@ def test_promote_resolves_this_model_with_single_mapping_entry( snapshots = {} for i in range(20): model = load_sql_based_model( - parse( + d.parse( f""" MODEL ( name test_schema.model_{i},