Skip to content

Commit 3cc4daa

Browse files
mday-ioclaude
andcommitted
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyFdP5xRu9D368mjGcYDLn Signed-off-by: mday-io <mdaytn@gmail.com>
1 parent 8d0b4de commit 3cc4daa

2 files changed

Lines changed: 151 additions & 12 deletions

File tree

‎sqlmesh/core/renderer.py‎

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -330,12 +330,27 @@ def _resolve_table(
330330
table_mapping: t.Optional[t.Dict[str, str]] = None,
331331
deployability_index: t.Optional[DeployabilityIndex] = None,
332332
) -> exp.Table:
333+
table_mapping = table_mapping or {}
334+
if isinstance(table_name, str):
335+
# table_name arrives here already normalized to a model FQN (see the `resolve_table`
336+
# closure below and the `this_model` call site), the same key format `snapshots` and
337+
# `table_mapping` use. Only the one relevant snapshot needs mapping, not the whole
338+
# environment - building the full mapping made this call O(N) in the number of
339+
# snapshots in the environment for every table resolved.
340+
snapshot = snapshots.get(table_name) if snapshots else None
341+
mapping = {
342+
**self._to_table_mapping([snapshot] if snapshot else [], deployability_index),
343+
**({table_name: table_mapping[table_name]} if table_name in table_mapping else {}),
344+
}
345+
else:
346+
mapping = {
347+
**self._to_table_mapping((snapshots or {}).values(), deployability_index),
348+
**table_mapping,
349+
}
350+
333351
table = exp.replace_tables(
334352
t.cast(exp.Table, exp.maybe_parse(table_name, into=exp.Table, dialect=self._dialect)),
335-
{
336-
**self._to_table_mapping((snapshots or {}).values(), deployability_index),
337-
**(table_mapping or {}),
338-
},
353+
mapping,
339354
dialect=self._dialect,
340355
copy=False,
341356
)
@@ -365,10 +380,6 @@ def _resolve_tables(
365380
with self._normalize_and_quote(expression) as expression:
366381
snapshots = snapshots or {}
367382
table_mapping = table_mapping or {}
368-
mapping = {
369-
**self._to_table_mapping(snapshots.values(), deployability_index),
370-
**table_mapping,
371-
}
372383
expand = set(expand) | {
373384
name for name, snapshot in snapshots.items() if snapshot.is_embedded
374385
}
@@ -410,10 +421,22 @@ def _expand(node: exp.Expr) -> exp.Expr:
410421

411422
expression = expression.transform(_expand, copy=False) # type: ignore
412423

413-
if mapping:
414-
expression = exp.replace_tables(
415-
expression, mapping, dialect=self._dialect, copy=False
416-
)
424+
# Building the full snapshot -> table-name mapping and normalizing it in
425+
# exp.replace_tables is O(N) in the number of snapshots in the environment; skip it
426+
# entirely for expressions that don't reference any table at all (e.g. session/
427+
# virtual properties), since there's nothing for the mapping to replace.
428+
if expression.find(exp.Table):
429+
# mypy loses the `snapshots`/`table_mapping` narrowing above because they're
430+
# captured by the `_expand` closure defined earlier in this block.
431+
assert snapshots is not None and table_mapping is not None
432+
mapping = {
433+
**self._to_table_mapping(snapshots.values(), deployability_index),
434+
**table_mapping,
435+
}
436+
if mapping:
437+
expression = exp.replace_tables(
438+
expression, mapping, dialect=self._dialect, copy=False
439+
)
417440

418441
return expression
419442

‎tests/core/test_model.py‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9748,6 +9748,122 @@ def resolve_parent(evaluator, name):
97489748
assert post_statements[0].sql() == f'"main"."sqlmesh__schema"."schema__parent__{version}"'
97499749

97509750

9751+
def test_resolve_table_large_environment(make_snapshot: t.Callable, mocker: MockerFixture):
9752+
"""`_resolve_table` should only build a mapping for the one table being resolved, not the
9753+
entire environment (https://github.com/SQLMesh/sqlmesh/issues/6017)."""
9754+
9755+
@macro()
9756+
def resolve_named(evaluator, name):
9757+
return evaluator.resolve_table(name.name)
9758+
9759+
target = load_sql_based_model(d.parse("MODEL (name target); SELECT 1 AS c"))
9760+
target_snapshot = make_snapshot(target)
9761+
target_snapshot.categorize_as(SnapshotChangeCategory.BREAKING)
9762+
9763+
snapshots = {'"target"': target_snapshot}
9764+
for i in range(50):
9765+
other = load_sql_based_model(d.parse(f"MODEL (name other_{i}); SELECT 1 AS c"))
9766+
other_snapshot = make_snapshot(other)
9767+
other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING)
9768+
snapshots[f'"other_{i}"'] = other_snapshot
9769+
9770+
child = load_sql_based_model(
9771+
d.parse(
9772+
"""
9773+
MODEL (name child);
9774+
SELECT c FROM target;
9775+
@resolve_named('target')
9776+
"""
9777+
)
9778+
)
9779+
9780+
spy = mocker.spy(exp, "replace_tables")
9781+
9782+
post_statements = child.render_post_statements(snapshots=snapshots)
9783+
assert len(post_statements) == 1
9784+
assert post_statements[0].sql() == f'"sqlmesh__default"."target__{target_snapshot.version}"'
9785+
9786+
# every replace_tables call made while resolving the single `target` reference should only
9787+
# ever see that one mapping entry, not all 51 snapshots in the environment
9788+
for call in spy.call_args_list:
9789+
assert len(call.args[1]) <= 1
9790+
9791+
# an explicit table_mapping entry takes precedence over the snapshot-derived one (rendered
9792+
# via a separate model instance so the statement-render cache doesn't return the earlier result)
9793+
child_for_override = load_sql_based_model(
9794+
d.parse(
9795+
"""
9796+
MODEL (name child_override);
9797+
SELECT c FROM target;
9798+
@resolve_named('target')
9799+
"""
9800+
)
9801+
)
9802+
override = child_for_override.render_post_statements(
9803+
snapshots=snapshots, table_mapping={'"target"': "overridden_table"}
9804+
)
9805+
assert override[0].sql() == '"overridden_table"'
9806+
9807+
# a name absent from both snapshots and table_mapping resolves unchanged
9808+
unmapped = load_sql_based_model(
9809+
d.parse(
9810+
"""
9811+
MODEL (name unmapped_child);
9812+
SELECT 1 AS c;
9813+
@resolve_named('does_not_exist')
9814+
"""
9815+
)
9816+
)
9817+
unmapped_result = unmapped.render_post_statements(snapshots=snapshots)
9818+
assert unmapped_result[0].sql() == '"does_not_exist"'
9819+
9820+
9821+
def test_render_virtual_properties_skips_mapping_without_table_refs(
9822+
make_snapshot: t.Callable, mocker: MockerFixture
9823+
):
9824+
"""Rendering a property expression with no table references shouldn't build the full
9825+
snapshot -> table-name mapping at all (https://github.com/SQLMesh/sqlmesh/issues/6017)."""
9826+
import sqlmesh.core.snapshot as snapshot_module
9827+
9828+
model = load_sql_based_model(
9829+
d.parse(
9830+
"""
9831+
MODEL (
9832+
name test_schema.test_model,
9833+
virtual_properties (
9834+
labels = [('team', 'data')]
9835+
),
9836+
session_properties (
9837+
"spark.executor.memory" = '1G'
9838+
),
9839+
);
9840+
SELECT a FROM tbl;
9841+
"""
9842+
)
9843+
)
9844+
9845+
snapshots = {}
9846+
for i in range(50):
9847+
other = load_sql_based_model(d.parse(f"MODEL (name other_{i}); SELECT 1 AS c"))
9848+
other_snapshot = make_snapshot(other)
9849+
other_snapshot.categorize_as(SnapshotChangeCategory.BREAKING)
9850+
snapshots[f'"other_{i}"'] = other_snapshot
9851+
9852+
to_table_mapping_spy = mocker.spy(snapshot_module, "to_table_mapping")
9853+
9854+
assert model.render_virtual_properties(snapshots=snapshots) == {
9855+
"labels": exp.maybe_parse("[('team', 'data')]")
9856+
}
9857+
assert model.render_session_properties(snapshots=snapshots) == {
9858+
"spark.executor.memory": "1G",
9859+
}
9860+
9861+
# `this_model` resolution may still make a narrow, single-snapshot (or empty) call, but the
9862+
# full N-snapshot mapping build in `_resolve_tables` must never fire for a table-less property
9863+
for call in to_table_mapping_spy.call_args_list:
9864+
assert len(call.args[0]) <= 1
9865+
9866+
97519867
def test_cluster_with_complex_expression():
97529868
expressions = d.parse(
97539869
"""

0 commit comments

Comments
 (0)