Conversation
_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 SQLMesh#6017 (one of three sub-issues split out of SQLMesh#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>
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DzEtt434q32KhtoGDtD424 Signed-off-by: Michael Day <mdaytn@gmail.com>
ee4b41e to
a5ee072
Compare
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 <mdaytn@gmail.com>
mday-io
left a comment
There was a problem hiding this comment.
Two findings: override-precedence regression and failing formatter check.
Signed-off-by: mday-io <mdaytn@gmail.com>
Signed-off-by: mday-io <mdaytn@gmail.com>
Signed-off-by: mday-io <mdaytn@gmail.com>
|
I tested the change against our production-backed monorepo project to understand whether the improvement translated beyond the focused unit tests. The targeted benchmark confirms the optimization works: repeated resolution against 3,132 production snapshots was substantially faster, and the rendered SQL remained identical. However, a real environment promotion showed no measurable improvement in the virtual-layer stage:
A broader render-only run across 1,775 promoted models produced the same conclusion. Most properties in the real project are plain strings and do not exercise the expensive expression-resolution path often enough to affect overall plan performance. The change appears correct, but the practical benefit is too narrow to justify carrying the additional logic without a real workload demonstrating an end-to-end improvement. I’m going to close this PR for now. We can revisit it if we find a project where table-containing property expressions materially slow promotion. |
Description
Resolving one table during virtual-layer updates previously built and normalized a mapping for every snapshot.
_resolve_tablenow maps only the matching snapshot on an exact lookup, retaining the full-mapping fallback for dialect-dependent name mismatches._resolve_tablesskips snapshot scans and expansion setup when an expression contains no table reference.During promotion, each target model can receive the broader snapshot context because its expressions and macros may reference other models. Property values go through the same expression renderer as SQL statements. Previously, even a literal virtual property such as
description = 'Customer orders'triggered construction of the full snapshot mapping when snapshots were supplied, despite containing no table reference.This work repeats per rendered property or statement, not just once per plan. For illustration, promoting 1,000 models with three literal virtual properties each and a 1,000-snapshot context would cause 3,000 mapping builds, each visiting 1,000 snapshots. This is a count of avoidable work for that hypothetical workload, not a measured benchmark or timing claim. Empty property collections produce no calls, and calls without snapshots, overrides, or expansion already returned early.
The change targets that unnecessary setup. Expressions containing table references still use the broader mapping, while single-table resolution narrows the snapshot lookup when an exact match is available.
Explicit
table_mappingentries retain their dialect-aware matching and precedence, including when an exact snapshot match exists or a later equivalent override follows an exact-key override. The fast path avoids scanning unrelated snapshots; processing explicit overrides remains proportional to the size of that mapping.Fixes #6017.
Test Plan
make style: passed (Ruff, formatting, mypy, migration validation).07e2ddf4: DCO/Commits Check passed; the full CI run is still in progress.Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO