From 949cbd2f087f1374acba4606198f70ff729f0c11 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Wed, 12 Aug 2026 16:40:35 +0200 Subject: [PATCH 1/2] fix(DEV-1779): formula measure referencing a sibling measure emits valid SQL regardless of order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A saved formula (`habit_score = order_count / unique_customers`) inline-expands at parse time to leaf colon refs (`id:count / customer:count_distinct`), so a formula measure enriched BEFORE a referenced sibling froze the sibling's canonical alias (`orders.id_count`) into its expression SQL; the sibling's later direct selection renamed the base-CTE column to `orders.order_count`, leaving the frozen reference dangling — invalid SQL on Postgres, silently-NULL on SQLite. The DEV-1444 provenance-merge only reconciled the forward order. Make the rename atomic via one `_repoint_alias(prev, new)` helper called at BOTH rename sites (local-agg + cross-model-intercept): it sweeps every `known_aliases` value, the `measure_canonical_key_to_alias` index, and the already-frozen carriers `EnrichedExpression.sql` (exact quoted-token replace) and `EnrichedTransform.measure_alias` (so `cumsum` / `change_pct` follow too). Defense-in-depth: the SQL generator's CTE-layering post-loop now raises a precise ValueError for any unresolved expression AND all transform types, instead of emitting invalid SQL / silently dropping an unresolved self-join. --- DECISIONS.md | 1 + slayer/engine/enrichment.py | 52 ++- slayer/sql/generator.py | 21 +- ...est_formula_referencing_measure_dev1779.py | 401 ++++++++++++++++++ tests/test_nested_dag_cross_stage_refs.py | 52 +++ 5 files changed, 514 insertions(+), 13 deletions(-) create mode 100644 tests/test_formula_referencing_measure_dev1779.py diff --git a/DECISIONS.md b/DECISIONS.md index 496c8b6f..0e460bd3 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -69,3 +69,4 @@ implementation detail. Include issue refs when known. - 2026-08-03 — Optional blocks + Cube JS/FILTER_PARAMS import (DEV-1730 / #270): a Mode-A-only `{? ... ?}` block renders its content parenthesised when every inner `{var}` is supplied, else collapses to the neutral `(1=1)` — the SLayer form of a Cube `FILTER_PARAMS` optional pushdown. Blocks live in the same `substitute_variables` (escape="sql") scanner as `{var}`/`{{`/`}}`, must contain ≥1 var, do not nest, and are rejected in Mode-B. A block-bearing model runs substitution even on a zero-variable call so its blocks collapse (the `_substitute_model_sql_surfaces` fast-path now checks for `{?` too); a block-free, required-only model with zero variables is still left untouched (the documented DEV-1625 raw-brace-literal boundary). `extract_model_variables(model)` derives required (bare, no default) vs optional (in-block or defaulted) from the four Mode-A surfaces — structural, nothing persisted, surfaced additively in the inspect skeleton `Variables:` line. The Cube importer gains a **JavaScript front-end** (esprima ESTree parser, a new core dep) that parses `cube()`/`view()` into the same `CubeCube`/`CubeView` shapes as YAML (dynamic values → report + skip member). FILTER_PARAMS refs are carried JS→converter as structured `CubeFilterParamRef` on the transient `CubeCube` (sentinels in the surface text; no arrow-body re-parse, sidestepping the `{var}`-vs-`{FILTER_PARAMS…}` brace clash); the converter resolves sentinels AFTER `translate_cube_refs` so the introduced `{var}` are never eaten. Requiredness (bare vs block) is decided in the converter alone via `honor_required_meta` (default on; CLI `--ignore-required-meta`) AND the member's `meta.required`; with the flag off a scalar-position arrow collapses to Cube's own `(1=1)::TIMESTAMP` booby-trap, faithfully. Cross-cube refs, unknown members, and generated-name collisions (`d`→`d_from` clashing member `d_from`) drop the cube (`filter_params_unsupported`); each logical variable is reported once (`filter_params_variable`) and stashed in `meta.cube_variables`. `render_probe_text` (blocks→`(1=1)`, bare vars→`0`) is the single import-time validation renderer, matching runtime collapse. - 2026-08-04 — Dialect-aware / complete escaping for Mode-A `{variable}` substitution (DEV-1727), hardening DEV-1625. `substitute_variables(..., escape="sql")` is now **dialect-aware** and **fail-closed**: it gained a required keyword-only `backslash_escapes` signal (`bool | None`, raises if `None` in sql mode) so a caller rendering raw SQL can never silently under-escape. On backslash-escaping dialects (MySQL/ClickHouse/Snowflake/Redshift/BigQuery/Databricks/Spark) it doubles the backslash before escaping the single quote; on standard dialects it keeps the `''` quote-doubling. The double quote is deliberately left untouched — inside a single-quoted literal `\"` is NOT a recognised escape on 6 of the 7 backslash dialects (only MySQL), so escaping it would corrupt the value. The regime is DERIVED from sqlglot's own tokenizer via `SqlDialect.backslash_escapes_strings` (= `"\\" in tokenizer.STRING_ESCAPES`, guarded + 14-dialect pinned) so our escaping can never drift from the parser that reads the substituted SQL. `escape="python"` (Mode-B) additionally encodes the full C0 control range (`\t`/`\n`/`\r` named, rest `\xNN`) so raw newlines/NUL no longer break `ast.parse`. Engine fail-closed: `_substitute_model_sql_surfaces` / `_render_probe_model` require a `dialect`, threaded from the resolved datasource — no bare bool to forget. Assumes MySQL's default `sql_mode` (backslash escapes on); `NO_BACKSLASH_ESCAPES` servers are a sqlglot-layer-wide limitation, documented not fixed. The SQLite backslash end-to-end gap stays a pinned strict-xfail (pre-existing, out of scope). Bound parameters rejected (don't fit substitute-into-raw-SQL). Nested/join/cross-model lineages remain DEV-1678. - 2026-08-04 — Declared list-valued `{variable}` coercion (DEV-1730 follow-up): a scalar supplied for a variable the model declares `list_valued` is wrapped into a one-element list before Mode-A substitution, so an importer-generated `col IN ({var})` renders `IN ('US')` rather than the unquoted `IN (US)`. The generic scalar rule (author writes the quotes, so `{var}` also works in numeric/fragment positions like `amount >= {floor}` and `{d}::TIMESTAMP`) is CORRECT and unchanged — it just presumes an author who can see the SQL position, which a machine-generated fixed template does not have; the caller cannot supply per-element quotes through parentheses the importer wrote. Silent-wrong-answer risk drove the fix over a raise: `region IN (US)` parses as a column reference, so it fails at the database with a confusing message, or resolves against a real column and returns wrong rows. Opt-in is a front-end-NEUTRAL flag: the Cube converter writes `list_valued: ref.kind == "string"` into each `meta.cube_variables` entry (arrow forms splice pre-quoted scalars and stay `False`), and the engine reads only that flag — never Cube's `kind` taxonomy — so a future list-shaped front-end opts in the same way. Coercion lives at the single Mode-A choke point `_substitute_model_sql_surfaces` (execution and the `_render_probe_model` type-probe both route through it, so it cannot be bypassed) via `coerce_declared_list_variables` / `list_valued_variable_names` in `slayer/core/query.py`. Scope is deliberately narrow: only `str`/`int`/`float`/`bool` are wrapped; `list`/`tuple` pass through (the **empty list still raises** — "no filter" belongs to an optional block or a sentinel default); `None`/`dict` are left for `_render_variable_value` to reject with its own naming error; hand-written models declare nothing and are untouched. Follow-on from the same review: `declares_variables(model)` (any non-empty `meta.cube_variables`) now also defeats the DEV-1625 zero-variable fast path, via the shared `_model_needs_substitution_pass` predicate used by both `_substitute_model_sql_surfaces` and `_render_probe_model`. This closes the fast-path hole for a GENERATED model whose pushdowns are all required (no `{? ?}` block to force the pass): such a model used to emit a bare `{var}` into the SQL on a zero-variable call instead of raising the documented missing-variable error. The hole stays open — deliberately — for hand-written models, which declare nothing and keep the raw-brace-literal protection (`'{1,2,3}'`). The `list_valued` flag is matched with `is True`, not truthiness, since `meta` is user-extensible and a stray `1` or the string `"false"` must not switch substitution semantics. The bag is also SELF-IDENTIFYING — an entry counts only with a string `member` (the shape every importer writes) — so a hand-written `meta` that reuses the `cube_variables` key is not mistaken for generated SQL and silently stripped of its brace-literal protection. +- 2026-08-12 — Formula measure referencing a sibling saved measure now emits valid SQL regardless of measure order (DEV-1779). A saved formula (`habit_score = order_count / unique_customers`) inline-expands at parse time to leaf colon refs (`id:count / customer:count_distinct`), so when the formula measure is enriched BEFORE a referenced sibling, its expression SQL freezes the sibling's canonical alias (`orders.id_count`); the later direct selection of that sibling renames the base-CTE column to the declared name (`orders.order_count`) and the frozen reference dangled — invalid SQL on Postgres, silently-NULL on SQLite (double-quote-as-string-literal). The DEV-1444 provenance-merge only reconciled the forward order (sibling declared first). Fix makes the rename atomic via one `_repoint_alias(prev, new)` helper called at BOTH rename sites (local-agg and cross-model-intercept): it sweeps every `known_aliases` value, the `measure_canonical_key_to_alias` provenance index, and — the new part — the already-frozen carriers `EnrichedExpression.sql` (exact quoted-token replace; the closing quote makes `"orders.id_count"` never match `"orders.id_count_2"`) and `EnrichedTransform.measure_alias` (so `cumsum(order_count)` and `change_pct` desugaring follow the rename too). Quoted-token string replacement is SQL-token-blind but safe here because arithmetic expression SQL is compiler-produced and never embeds a single-quoted literal containing a double-quoted alias — same invariant `_resolve_sql` already relies on. Defense-in-depth: the SQL generator's CTE-layering loop previously emitted an unresolved expression (and silently DROPPED an unresolved self-join `time_shift`) when it stalled, so a regression of this class reached the DB as broken SQL; it now raises a precise `ValueError` naming the computed column / transform and the missing alias for expressions AND all transform types. `_deps_available` gates in-loop addition, so anything still pending is genuinely unresolved — no false-positive raise. diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index 59ec3d47..cc5c9542 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -345,6 +345,35 @@ def _mark_user_declared(alias: str) -> bool: return True return False + def _repoint_alias(prev_alias: str, new_alias: str) -> None: + """DEV-1779: repoint every reference to ``prev_alias`` onto ``new_alias``. + + A formula/transform enriched before the sibling measure it references + freezes that sibling's canonical alias (``orders.id_count``) into its + expression SQL / transform input. When the sibling is later renamed to + its declared name (``orders.order_count``), follow the rename in every + carrier: the alias resolver, the provenance-merge index, and the + already-frozen ``EnrichedExpression.sql`` / ``EnrichedTransform``. + """ + if prev_alias == new_alias: + return + for k, v in known_aliases.items(): + if v == prev_alias: + known_aliases[k] = new_alias + for k, v in list(measure_canonical_key_to_alias.items()): + if v == prev_alias: + measure_canonical_key_to_alias[k] = new_alias + # Aliases are emitted only as whole quoted identifiers, so matching the + # closing quote is exact: ``"orders.id_count"`` never matches the + # prefix of ``"orders.id_count_2"``. + quoted_prev, quoted_new = f'"{prev_alias}"', f'"{new_alias}"' + for e in enriched_expressions: + if quoted_prev in e.sql: + e.sql = e.sql.replace(quoted_prev, quoted_new) + for t in enriched_transforms: + if t.measure_alias == prev_alias: + t.measure_alias = new_alias + async def _ensure_aggregated_measure( alias_key: str, measure_name: str, @@ -1490,12 +1519,11 @@ def _mangled_formula(formula: str) -> str: break known_aliases[target_name] = target_alias known_aliases[canonical_name] = target_alias - # DEV-1444 provenance merge: any canonical key - # currently pointing at the pre-rename alias must - # follow the rename. - for k, v in list(measure_canonical_key_to_alias.items()): - if v == prev_alias: - measure_canonical_key_to_alias[k] = target_alias + # DEV-1444 provenance merge + DEV-1779 frozen-carrier + # rewrite: repoint resolver / provenance entries AND + # any expression/transform that already froze the + # pre-rename intercept alias onto the new alias. + _repoint_alias(prev_alias, target_alias) # canonical_to_user_name only fires when the # user explicitly renamed via qfield.name; the # auto-rename to cross-model canonical doesn't @@ -1655,12 +1683,12 @@ def _mangled_formula(formula: str) -> str: break known_aliases[qfield.name] = user_alias known_aliases[canonical_name] = user_alias - # DEV-1444 provenance merge: any canonical key currently - # pointing at the pre-rename alias must follow the rename - # so later auto-extracted refs collapse onto the new alias. - for k, v in list(measure_canonical_key_to_alias.items()): - if v == prev_alias: - measure_canonical_key_to_alias[k] = user_alias + # DEV-1444 provenance merge + DEV-1779 frozen-carrier rewrite: + # any resolver / provenance entry pointing at the pre-rename + # alias must follow the rename, AND any expression/transform + # that already froze the pre-rename alias must be rewritten so + # a formula enriched before this measure doesn't dangle. + _repoint_alias(prev_alias, user_alias) # DEV-1443: record the canonical → user-name mapping so # query filters and ORDER BY items referencing the raw # ``col:agg`` formula can be remapped to the user alias diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index 82dab6b4..3b095ed2 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -1698,13 +1698,33 @@ def _generate_with_computed(self, enriched: EnrichedQuery, final_parts = [self._q(a) for a in sorted(available_aliases)] # Add any remaining expressions/transforms that couldn't be layered. + # DEV-1779: a remaining item whose dependencies never became available + # can only reference a column no CTE projects — emitting it produces + # invalid SQL that fails (or silently NULLs, on SQLite) at the DB. + # Fail loudly here with the offending aliases instead. `_deps_available` + # gates in-loop addition, so anything still pending is genuinely + # unresolved (not a false positive). # DEV-1571 Bug 3 follow-up: re-emit each expression through the # active dialect so ANSI-quoted aliases from enrichment become # MySQL backticks / T-SQL brackets. for expr in pending_expressions: + if not self._deps_available(expr.sql, available_aliases): + missing = sorted(set(re.findall(r'"([^"]+)"', expr.sql)) - available_aliases) + raise ValueError( + f"Computed column {expr.alias!r} references column(s) " + f"{missing} that no CTE layer projects — the query could " + f"not be lowered to valid SQL (internal alias-resolution error)." + ) expr_sql = self._parse(expr.sql, dialect="postgres").sql(dialect=self.dialect) final_parts.append(f'{expr_sql} AS {self._q(expr.alias)}') for t in pending_transforms: + if t.measure_alias not in available_aliases: + raise ValueError( + f"Transform {t.alias!r} references measure alias " + f"{t.measure_alias!r} that no CTE layer projects — the query " + f"could not be lowered to valid SQL (internal " + f"alias-resolution error)." + ) if t.transform in _SELF_JOIN_TRANSFORMS: continue # Should not happen — self-joins are always materialized if t.transform == "consecutive_periods": @@ -1723,7 +1743,6 @@ def _generate_with_computed(self, enriched: EnrichedQuery, # pagination, so LIMIT/OFFSET operate on the filtered result. post_filters = [f for f in enriched.filters if f.is_post_filter] if post_filters: - import re model = enriched.model_name conditions = [] for f in post_filters: diff --git a/tests/test_formula_referencing_measure_dev1779.py b/tests/test_formula_referencing_measure_dev1779.py new file mode 100644 index 00000000..dfd875b4 --- /dev/null +++ b/tests/test_formula_referencing_measure_dev1779.py @@ -0,0 +1,401 @@ +"""DEV-1779: a formula measure that references sibling saved measures must +emit valid SQL regardless of the order the measures appear in the query. + +Root cause was an ordering asymmetry in the provenance-merge: a formula +enriched *before* the sibling measure it references froze that sibling's +canonical alias (``orders.id_count``) into its expression SQL / transform +input, and the later direct selection renamed the base-CTE column to the +declared name (``orders.order_count``) without following the frozen +reference — so the outer SELECT referenced a column no CTE projected. + +Test groups: + A string-shape invariant over the measure-ordering matrix (no DB) + B end-to-end execution over the same matrix (temp-file SQLite) + C the same defect through a transform-wrapped reference (cumsum) + D full reported scenario: joined dimension + ORDER BY the formula + E generator defense-in-depth guard (raises instead of emitting bad SQL) + +Only the formula-first / formula-middle orderings reproduce the bug; the +forward-order, single-ref, and no-ref cases are non-regression controls that +already pass on the pre-fix code (they assert the invariant is preserved). +""" + +from __future__ import annotations + +import re +import sqlite3 + +import pytest + +from slayer.core.enums import DataType, TimeGranularity +from slayer.core.models import ( + Column, + DatasourceConfig, + ModelJoin, + ModelMeasure, + SlayerModel, +) +from slayer.core.query import SlayerQuery +from slayer.engine.enriched import ( + EnrichedExpression, + EnrichedMeasure, + EnrichedQuery, + EnrichedTimeDimension, + EnrichedTransform, +) +from slayer.engine.enrichment import enrich_query +from slayer.engine.query_engine import SlayerQueryEngine +from slayer.sql.generator import SQLGenerator +from slayer.storage.yaml_storage import YAMLStorage + +# Canonical auto-aliases of the two aggregates the formula expands to. If +# either shows up in the SQL *referenced but not declared*, the bug is live. +_CANON_ORDER = "orders.id_count" +_CANON_UNIQUE = "orders.customer_count_distinct" + + +def _habit_measures() -> list[ModelMeasure]: + """order_count / unique_customers, plus the formula that divides them.""" + return [ + ModelMeasure(name="order_count", formula="id:count"), + ModelMeasure(name="unique_customers", formula="customer:count_distinct"), + ModelMeasure(name="total_revenue", formula="revenue:sum"), + ModelMeasure(name="habit_score", formula="order_count / unique_customers"), + ] + + +def _orders_model(measures: list[ModelMeasure] | None = None) -> SlayerModel: + return SlayerModel( + name="orders", + sql_table="orders", + data_source="test", + default_time_dimension="created_at", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="customer", sql="customer", type=DataType.TEXT), + Column(name="revenue", sql="amount", type=DataType.DOUBLE), + Column(name="store_id", sql="store_id", type=DataType.DOUBLE), + Column(name="created_at", sql="created_at", type=DataType.TIMESTAMP), + ], + joins=[ModelJoin(target_model="stores", join_pairs=[["store_id", "id"]])], + measures=_habit_measures() if measures is None else measures, + ) + + +def _stores_model() -> SlayerModel: + return SlayerModel( + name="stores", + sql_table="stores", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="name", sql="name", type=DataType.TEXT), + ], + ) + + +# --------------------------------------------------------------------------- +# SQL-shape invariant helper +# --------------------------------------------------------------------------- + + +def _referenced_but_undeclared(sql: str) -> set[str]: + """Generated aliases referenced in ``sql`` but never declared with ``AS``. + + Every ``"model.col"`` alias a SELECT/CTE references must be projected + (declared ``AS "model.col"``) by some layer below it. A non-empty result + means the SQL references a column no CTE produces — exactly the DEV-1779 + failure (``"orders.id_count"`` referenced, only ``"orders.order_count"`` + declared). Restricted to *dotted* quoted identifiers: SLayer aliases are + always ``model.col`` (a dot), while base-table columns / physical + identifiers are bare, so the dot filter avoids false-failing on a quoted + physical name. This is a heuristic backstop; the execution tests are the + authoritative check that the SQL is valid end to end. + """ + declared = set(re.findall(r'AS "([^"]+)"', sql)) + referenced = {ref for ref in re.findall(r'"([^"]+)"', sql) if "." in ref} + return referenced - declared + + +# --------------------------------------------------------------------------- +# Group A — string-shape invariant over the measure-ordering matrix (no DB) +# --------------------------------------------------------------------------- + + +async def _noop_async(**_kw): + return None + + +async def _gen_single_model_sql(measures: list[str]) -> str: + """Enrich + generate an orders-only query (no join resolution needed).""" + model = _orders_model() + query = SlayerQuery(source_model="orders", measures=measures) + enriched = await enrich_query( + query=query, + model=model, + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + return SQLGenerator(dialect="postgres").generate(enriched=enriched) + + +# (id, label, reproduces_bug, both_refs_selected) +_ORDERINGS = [ + ("formula_first", ["habit_score", "order_count", "unique_customers"], True, True), + ("formula_middle", ["order_count", "habit_score", "unique_customers"], True, True), + ("formula_last", ["order_count", "unique_customers", "total_revenue", "habit_score"], False, True), + ("one_ref_selected", ["order_count", "habit_score"], False, False), + ("no_ref_selected", ["habit_score"], False, False), +] + + +@pytest.mark.parametrize( + "label,measures,_bug,both_refs", _ORDERINGS, ids=[c[0] for c in _ORDERINGS] +) +async def test_formula_ref_sql_has_no_dangling_alias( + label: str, measures: list[str], _bug: bool, both_refs: bool +) -> None: + sql = await _gen_single_model_sql(measures) + assert _referenced_but_undeclared(sql) == set(), sql + if both_refs: + # When both siblings are selected they are both renamed, so neither + # canonical auto-alias may survive anywhere in the SQL. + assert f'"{_CANON_ORDER}"' not in sql, sql + assert f'"{_CANON_UNIQUE}"' not in sql, sql + + +# --------------------------------------------------------------------------- +# Group B/C/D — execution + join scenarios (temp-file SQLite) +# --------------------------------------------------------------------------- + + +async def _make_engine(tmp_path, seed: bool) -> SlayerQueryEngine: + db_file = tmp_path / "slayer_test.db" + if seed: + conn = sqlite3.connect(db_file) + conn.executescript( + """ + CREATE TABLE stores (id INTEGER PRIMARY KEY, name TEXT); + INSERT INTO stores VALUES (1, 'North'), (2, 'South'); + CREATE TABLE orders ( + id INTEGER PRIMARY KEY, customer TEXT, amount REAL, + store_id INTEGER, created_at TEXT + ); + -- North: 6 orders, 2 distinct customers → habit = 3 + INSERT INTO orders VALUES + (1, 'A', 10, 1, '2026-01-01'), + (2, 'A', 20, 1, '2026-01-02'), + (3, 'A', 30, 1, '2026-01-03'), + (4, 'B', 40, 1, '2026-02-01'), + (5, 'B', 50, 1, '2026-02-02'), + (6, 'B', 60, 1, '2026-02-03'), + -- South: 2 orders, 2 distinct customers → habit = 1 + (7, 'C', 70, 2, '2026-01-01'), + (8, 'D', 80, 2, '2026-02-01'); + -- Ungrouped: 8 orders, 4 distinct customers → habit = 2 (exact) + """ + ) + conn.commit() + conn.close() + + storage = YAMLStorage(base_dir=str(tmp_path / "store")) + await storage.save_datasource( + DatasourceConfig(name="test", type="sqlite", database=str(db_file)) + ) + await storage.save_model(_stores_model()) + await storage.save_model(_orders_model()) + return SlayerQueryEngine(storage=storage) + + +@pytest.mark.parametrize( + "label,measures,_bug,_both", _ORDERINGS, ids=[c[0] for c in _ORDERINGS] +) +async def test_formula_ref_executes( + tmp_path, label: str, measures: list[str], _bug: bool, _both: bool +) -> None: + engine = await _make_engine(tmp_path, seed=True) + query = SlayerQuery(source_model="orders", measures=measures) + resp = await engine.execute(query=query) # runs the real SQL — must not raise + assert resp.data, resp.sql + row = resp.data[0] + # Single ungrouped bucket: order_count=8, unique_customers=4 → habit=2. + # unique_customers is not always projected (hidden inside the formula for + # one_ref/no_ref), so assert against the formula result directly. + assert row["orders.habit_score"] == 2 + if "orders.order_count" in row: + assert row["orders.order_count"] == 8 + + +async def test_transform_wrapped_reference_follows_rename() -> None: + """Group C: cumsum(order_count) with order_count selected AFTER — the + hidden transform's input alias must follow the rename (not orphan).""" + model = _orders_model( + measures=[ + ModelMeasure(name="order_count", formula="id:count"), + ModelMeasure(name="running_orders", formula="cumsum(order_count)"), + ] + ) + query = SlayerQuery( + source_model="orders", + measures=["running_orders", "order_count"], + time_dimensions=[{"dimension": "created_at", "granularity": "month"}], + ) + enriched = await enrich_query( + query=query, + model=model, + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + sql = SQLGenerator(dialect="postgres").generate(enriched=enriched) + assert _referenced_but_undeclared(sql) == set(), sql + assert f'"{_CANON_ORDER}"' not in sql, sql + + +async def test_change_pct_desugar_reference_follows_rename() -> None: + """Group C (desugaring): change_pct(order_count) desugars to an + expression + a hidden time_shift, both referencing the inner measure. + With order_count selected AFTER, both frozen carriers must follow the + rename.""" + model = _orders_model( + measures=[ + ModelMeasure(name="order_count", formula="id:count"), + ModelMeasure(name="mom_orders", formula="change_pct(order_count)"), + ] + ) + query = SlayerQuery( + source_model="orders", + measures=["mom_orders", "order_count"], + time_dimensions=[{"dimension": "created_at", "granularity": "month"}], + ) + enriched = await enrich_query( + query=query, + model=model, + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + sql = SQLGenerator(dialect="postgres").generate(enriched=enriched) + assert _referenced_but_undeclared(sql) == set(), sql + assert f'"{_CANON_ORDER}"' not in sql, sql + + +async def test_full_reported_scenario(tmp_path) -> None: + """Group D: the exact shape from the ticket — joined ``stores.name`` + dimension, formula measure listed first, and ORDER BY the formula.""" + engine = await _make_engine(tmp_path, seed=True) + query = SlayerQuery( + source_model="orders", + measures=["habit_score", "order_count", "unique_customers", "total_revenue"], + dimensions=["stores.name"], + order=[{"column": "habit_score", "direction": "desc"}], + limit=100, + ) + dry = await engine.execute(query=query, dry_run=True) + assert dry.sql is not None + assert _referenced_but_undeclared(dry.sql) == set(), dry.sql + + resp = await engine.execute(query=query) # must execute cleanly + assert [r["orders.stores.name"] for r in resp.data] == ["North", "South"] + for r in resp.data: + assert r["orders.habit_score"] * r["orders.unique_customers"] == ( + r["orders.order_count"] + ) + assert resp.data[0]["orders.habit_score"] == 3 # North: 6 orders / 2 customers + assert resp.data[1]["orders.habit_score"] == 1 # South: 2 orders / 2 customers + + +# --------------------------------------------------------------------------- +# Group E — generator defense-in-depth guard +# --------------------------------------------------------------------------- + + +def _measure(alias: str, *, sql: str = "id", agg: str = "count") -> EnrichedMeasure: + return EnrichedMeasure( + name=alias.split(".", 1)[-1], sql=sql, aggregation=agg, + alias=alias, model_name="orders", + ) + + +def _time_dim() -> EnrichedTimeDimension: + """A projected time dimension so a transform's ``time_alias`` is available + — isolating the guard on the missing ``measure_alias``.""" + return EnrichedTimeDimension( + name="created_at", + sql="created_at", + granularity=TimeGranularity.MONTH, + date_range=None, + alias="orders.created_at", + model_name="orders", + ) + + +def test_generator_raises_on_expression_with_unknown_alias() -> None: + enriched = EnrichedQuery( + model_name="orders", + sql_table="orders", + measures=[_measure("orders.order_count")], + expressions=[ + EnrichedExpression( + name="habit_score", + sql=f'"{_CANON_ORDER}" / "{_CANON_UNIQUE}"', + alias="orders.habit_score", + ) + ], + ) + with pytest.raises(ValueError) as exc: + SQLGenerator(dialect="postgres").generate(enriched=enriched) + msg = str(exc.value) + assert "orders.habit_score" in msg + # The guard must report *all* missing inputs, not just the first. + assert _CANON_ORDER in msg + assert _CANON_UNIQUE in msg + + +def test_generator_raises_on_window_transform_with_unknown_alias() -> None: + enriched = EnrichedQuery( + model_name="orders", + sql_table="orders", + measures=[_measure("orders.order_count")], + time_dimensions=[_time_dim()], # time_alias available; measure_alias is not + transforms=[ + EnrichedTransform( + name="running", + transform="cumsum", + measure_alias="orders.id_count", + alias="orders.running", + offset=1, + time_alias="orders.created_at", + ) + ], + ) + with pytest.raises(ValueError) as exc: + SQLGenerator(dialect="postgres").generate(enriched=enriched) + msg = str(exc.value) + assert "orders.running" in msg + assert "orders.id_count" in msg + + +def test_generator_raises_on_self_join_transform_with_unknown_alias() -> None: + enriched = EnrichedQuery( + model_name="orders", + sql_table="orders", + measures=[_measure("orders.order_count")], + time_dimensions=[_time_dim()], # time_alias available; measure_alias is not + transforms=[ + EnrichedTransform( + name="shifted", + transform="time_shift", + measure_alias="orders.id_count", + alias="orders.shifted", + offset=-1, + time_alias="orders.created_at", + ) + ], + ) + with pytest.raises(ValueError) as exc: + SQLGenerator(dialect="postgres").generate(enriched=enriched) + msg = str(exc.value) + assert "orders.shifted" in msg + assert "orders.id_count" in msg diff --git a/tests/test_nested_dag_cross_stage_refs.py b/tests/test_nested_dag_cross_stage_refs.py index 99b940ab..7b8eaf9d 100644 --- a/tests/test_nested_dag_cross_stage_refs.py +++ b/tests/test_nested_dag_cross_stage_refs.py @@ -1826,3 +1826,55 @@ async def test_intercepted_rename_colliding_with_other_canonical_raises( # integration site for cross-stage dotted refs and is covered by tests # in `TestCrossStageFilter` (#5). # =========================================================================== + + +# =========================================================================== +# DEV-1779 — the cross-model-INTERCEPT rename site. +# +# A downstream-stage formula that references an intercepted cross-model +# aggregate BEFORE that same aggregate is selected+renamed must not orphan +# the frozen reference. This exercises the second rename site of the fix +# (the intercept branch), distinct from the local-agg site covered in +# tests/test_formula_referencing_measure_dev1779.py. +# =========================================================================== + + +def _dev1779_undeclared(sql: str) -> set[str]: + """Dotted quoted aliases referenced but never declared with ``AS`` — a + non-empty result means the SQL references a column no CTE projects.""" + import re + + declared = set(re.findall(r'AS "([^"]+)"', sql)) + referenced = {ref for ref in re.findall(r'"([^"]+)"', sql) if "." in ref} + return referenced - declared + + +class TestDev1779InterceptRename: + async def test_formula_before_renamed_intercept_ref(self, tmp_path) -> None: + """Outer stage lists a formula over ``customers.revenue:sum`` FIRST + (freezing the intercept alias), then selects the same cross-model + aggregate with an explicit rename. Before the fix the frozen formula + reference is orphaned when the intercept measure is renamed.""" + engine = await _engine_with_real_sqlite(tmp_path) + inner = SlayerQuery( + name="s1", + source_model="orders", + dimensions=["customers.regions.name"], + measures=[{"formula": "customers.revenue:sum"}], + ) + outer = SlayerQuery( + source_model="s1", + measures=[ + {"formula": "customers.revenue:sum / *:count", "name": "avg_rev"}, + {"formula": "customers.revenue:sum", "name": "cust_rev"}, + ], + ) + dry = await engine.execute(query=[inner, outer], dry_run=True) + assert dry.sql is not None + assert _dev1779_undeclared(dry.sql) == set(), dry.sql + + resp = await engine.execute(query=[inner, outer]) # must execute + row = resp.data[0] + # 3 region groups, total revenue 1500 → cust_rev=1500, avg_rev=1500/3. + assert row["s1.cust_rev"] == pytest.approx(1500.0), row + assert row["s1.avg_rev"] == pytest.approx(500.0), row From 0b46cc4f69c3a476e1adc7b639228ee47d7d8253 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Wed, 12 Aug 2026 17:14:38 +0200 Subject: [PATCH 2/2] chore(DEV-1779): address SonarCloud findings (S7504, S5778) - Drop the unnecessary list() wrapper in _repoint_alias (the loop only reassigns existing keys' values; matches the known_aliases loop above). - Hoist SQLGenerator construction out of the pytest.raises blocks in the three generator-guard tests so each has one throwing invocation. --- slayer/engine/enrichment.py | 2 +- tests/test_formula_referencing_measure_dev1779.py | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index cc5c9542..4bba7db9 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -360,7 +360,7 @@ def _repoint_alias(prev_alias: str, new_alias: str) -> None: for k, v in known_aliases.items(): if v == prev_alias: known_aliases[k] = new_alias - for k, v in list(measure_canonical_key_to_alias.items()): + for k, v in measure_canonical_key_to_alias.items(): if v == prev_alias: measure_canonical_key_to_alias[k] = new_alias # Aliases are emitted only as whole quoted identifiers, so matching the diff --git a/tests/test_formula_referencing_measure_dev1779.py b/tests/test_formula_referencing_measure_dev1779.py index dfd875b4..3b35ff66 100644 --- a/tests/test_formula_referencing_measure_dev1779.py +++ b/tests/test_formula_referencing_measure_dev1779.py @@ -344,8 +344,9 @@ def test_generator_raises_on_expression_with_unknown_alias() -> None: ) ], ) + generator = SQLGenerator(dialect="postgres") with pytest.raises(ValueError) as exc: - SQLGenerator(dialect="postgres").generate(enriched=enriched) + generator.generate(enriched=enriched) msg = str(exc.value) assert "orders.habit_score" in msg # The guard must report *all* missing inputs, not just the first. @@ -370,8 +371,9 @@ def test_generator_raises_on_window_transform_with_unknown_alias() -> None: ) ], ) + generator = SQLGenerator(dialect="postgres") with pytest.raises(ValueError) as exc: - SQLGenerator(dialect="postgres").generate(enriched=enriched) + generator.generate(enriched=enriched) msg = str(exc.value) assert "orders.running" in msg assert "orders.id_count" in msg @@ -394,8 +396,9 @@ def test_generator_raises_on_self_join_transform_with_unknown_alias() -> None: ) ], ) + generator = SQLGenerator(dialect="postgres") with pytest.raises(ValueError) as exc: - SQLGenerator(dialect="postgres").generate(enriched=enriched) + generator.generate(enriched=enriched) msg = str(exc.value) assert "orders.shifted" in msg assert "orders.id_count" in msg