From 949cbd2f087f1374acba4606198f70ff729f0c11 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Wed, 12 Aug 2026 16:40:35 +0200 Subject: [PATCH 1/4] 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 2ec324d38d0234f4cd6d70d8ad1d8cb890f6eb0d Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Wed, 12 Aug 2026 17:11:28 +0200 Subject: [PATCH 2/4] fix(DEV-1780): bind or reject dotted dimension join paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dotted dimension/time-dimension path only resolves when every hop is a direct join. A non-direct hop previously fell through leniently — the dim kept its A__B alias in SELECT/GROUP BY but no join was emitted, shipping invalid SQL (unbound table alias). Filters and cross-model measures already rejected such paths; only dimensions/time-dimensions had the hole. A short-form ref (target model only, e.g. Consumer.name) with a unique route now auto-resolves to the full routed path (result key = full path). Ambiguous, unreachable, and broken-explicit-chain refs reject with a new UnresolvableDimensionJoinError carrying a route-aware suggestion (short form when the target is uniquely reachable, else the shortest full path). The rewrite is applied to matching ORDER BY and main_time_dimension. Routing is datasource-scoped and deferred when named-query stages are in scope. A post-_resolve_joins guard guarantees enrich_query never returns an unbound dimension alias; the re-rooted cross-model CTE opts out via enforce_join_binding=False. --- DECISIONS.md | 1 + slayer/core/errors.py | 45 ++ slayer/engine/enrichment.py | 20 + slayer/engine/join_graph.py | 55 +++ slayer/engine/query_engine.py | 143 +++++- tests/test_dev1780_missing_join_path.py | 625 ++++++++++++++++++++++++ 6 files changed, 888 insertions(+), 1 deletion(-) create mode 100644 tests/test_dev1780_missing_join_path.py diff --git a/DECISIONS.md b/DECISIONS.md index 496c8b6f..81ef28e8 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 — Dotted dimension join-path binding (DEV-1780): a dotted dimension/time-dimension path resolves only when every hop is a direct join. Previously a hop that was not a direct join fell through leniently — the enriched dim kept its `A__B` alias in SELECT/GROUP BY but `_resolve_joins` emitted no join, shipping invalid SQL (unbound table alias). Filters and cross-model measures already rejected such paths; only dimensions/time-dimensions had the hole (the shared `_resolve_dotted_dim_with_stage_fallback` lenient branch). Fix is an engine routing pre-pass (`SlayerQueryEngine._route_dotted_dimension_refs`, run in `_enrich` before `enrich_query`, gated on `enforce_join_binding and source_model_origin is None`): it normalizes root-prefixes via `strip_source_model_prefix`, then for each dotted ref tries the explicit direct-join walk and, on `_NoJoinError`, routes via a datasource-scoped `JoinGraph`. A SHORT FORM (one model segment, e.g. `Consumer.name`) with exactly ONE route to the target auto-resolves — the ref is rewritten to the full routed path, so the result key is the full path (`root.Subscription.Customer.Consumer.name`), consistent with "joined dims keep the full path". Ambiguous (≥2 routes), unreachable (0), and explicit multi-hop chains with a broken hop are REJECTED with `UnresolvableDimensionJoinError(SlayerError, ValueError)` (mirrors the DEV-1645 `UnresolvableOrderColumnError` reject-don't-emit-invalid-SQL doctrine); the message suggests the short form when the target is uniquely reachable, else the shortest deterministic full path (`JoinGraph.shortest_path`), else nothing. `JoinGraph.count_simple_paths(root, target, cap=2)` classifies routes — it counts ALL simple paths (a 2-hop + 3-hop route is genuinely ambiguous; auto-picking the shorter would silently change join semantics), reverse-reachability-pruned and cycle-guarded. The rewrite map is also applied to matching `OrderItem.column` refs and `main_time_dimension` so dependent references stay consistent. Deliberate limits (conservative, prefer reject over a wrong route): routing runs only within a single datasource (`model.data_source` truthy; the graph is datasource-scoped) and is deferred when named-query stages are in scope (their virtual models aren't in the stored graph) — those refs fall through to the guard. A post-`_resolve_joins` safety-net guard in `enrich_query` (same gate) raises `UnresolvableDimensionJoinError` for any dim/time-dim whose alias is absent from `resolved_joins`, guaranteeing the invariant even for direct `enrich_query` callers; the re-rooted cross-model CTE enrichment passes `enforce_join_binding=False` (it legitimately carries source-local shared dims like `orders.status` that never bind to a base-table join). Out of scope: the multi-stage lenient cross-stage fall-through (`test_unresolvable_dotted_ref_falls_through`, where distinguishing a genuine error from a re-rooting artifact is unsolved) and leaf-column-missing-on-a-valid-path (the alias IS bound there — a different failure class). diff --git a/slayer/core/errors.py b/slayer/core/errors.py index c02f38c1..2ceb73ba 100644 --- a/slayer/core/errors.py +++ b/slayer/core/errors.py @@ -213,3 +213,48 @@ def __init__(self, *, column: str, qualifier: str) -> None: f"Project it (add to dimensions/measures), reference it in a filter, or " f"order by a projected field instead." ) + + +class UnresolvableDimensionJoinError(SlayerError, ValueError): + """A dimension / time-dimension dotted path that is not a valid direct-join + chain and cannot be uniquely routed to its target model (DEV-1780). + + A dotted path resolves only when every hop is a direct join. A short form + (``Consumer.name`` — target model only) auto-resolves when exactly one route + reaches the target; otherwise (ambiguous route, unreachable target, or an + explicit multi-hop chain with a broken hop) the reference is rejected here + rather than emitting SQL that references an unbound table alias. + + Multi-inherits ``ValueError`` (like ``UnresolvableOrderColumnError``) so + existing ``except ValueError`` sites keep working. ``__str__`` is computed + from the fields so ``suggested_path`` set after construction is reflected. + """ + + def __init__( + self, + *, + reference: str, + root_model: str, + reason: str | None = None, + available_joins: "list[str] | None" = None, + suggested_path: str | None = None, + ) -> None: + self.reference = reference + self.root_model = root_model + self.reason = reason + self.available_joins = available_joins + self.suggested_path = suggested_path + super().__init__() + + def __str__(self) -> str: + msg = ( + f"Cannot resolve dimension '{self.reference}': not a valid join path " + f"from '{self.root_model}'." + ) + if self.reason: + msg += f" {self.reason}" + if self.available_joins is not None: + msg += f" Available joins from '{self.root_model}': {sorted(self.available_joins)}." + if self.suggested_path: + msg += f" Did you mean '{self.suggested_path}'?" + return msg diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index 59ec3d47..4a3fab22 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -37,6 +37,7 @@ parse_filter, parse_formula, ) +from slayer.core.errors import UnresolvableDimensionJoinError from slayer.core.models import Column, SlayerModel from slayer.core.query import OrderItem, SlayerQuery, substitute_variables from slayer.core.refs import DOTTED_IDENT_REF_RE as _DOTTED_IDENT_REF_RE @@ -180,6 +181,7 @@ async def enrich_query( resolve_model=None, dialect: str = "postgres", drop_unreachable_filters: bool = False, + enforce_join_binding: bool = True, ) -> EnrichedQuery: """Resolve a SlayerQuery against model definitions into an EnrichedQuery. @@ -1878,6 +1880,24 @@ def _mangled_formula(formula: str) -> str: dialect=dialect, ) + # DEV-1780 safety net: never return a dim/time-dim whose join-path alias is + # absent from resolved_joins (it would render an unbound ``A__B`` reference). + # Skipped for virtual stages and the re-rooted CTE (enforce_join_binding=False). + if enforce_join_binding and model.source_model_origin is None: + _bound_aliases = {rj[1] for rj in resolved_joins} + _root_prefix = f"{model_name_str}." + for _bound_check in list(dimensions) + list(time_dimensions): + _mn = _bound_check.model_name + if _mn != model_name_str and _mn not in _bound_aliases: + _reference = _bound_check.alias + if _reference.startswith(_root_prefix): + _reference = _reference[len(_root_prefix):] + raise UnresolvableDimensionJoinError( + reference=_reference, + root_model=model_name_str, + available_joins=[j.target_model for j in model.joins], + ) + # Names that resolve at the query level (named measures, transforms, # expressions) — pass through as legitimate filter targets even though # they are not Columns / ModelMeasures on the source model. diff --git a/slayer/engine/join_graph.py b/slayer/engine/join_graph.py index 8f628962..2708d163 100644 --- a/slayer/engine/join_graph.py +++ b/slayer/engine/join_graph.py @@ -60,6 +60,61 @@ def reachable_from(self, root: str) -> set[str]: frontier.append(nbr) return seen + def _reverse_reachable_to(self, target: str) -> set[str]: + """Nodes that can reach ``target`` via directed edges (incl. ``target``). + Used to prune ``count_simple_paths`` to the relevant subgraph.""" + reverse: dict[str, set[str]] = {} + for src, nbrs in self._adj.items(): + for nbr in nbrs: + reverse.setdefault(nbr, set()).add(src) + seen: set[str] = {target} + frontier: deque[str] = deque([target]) + while frontier: + node = frontier.popleft() + for pred in reverse.get(node, ()): # noqa: SIM118 — .get default + if pred not in seen: + seen.add(pred) + frontier.append(pred) + return seen + + def count_simple_paths(self, root: str, target: str, *, cap: int = 2) -> int: + """Number of distinct simple (acyclic) directed paths ``root → target``, + capped at ``cap`` with early-stop. + + ``0`` = unreachable, ``1`` = unique route, ``>= cap`` = ambiguous. Counts + ALL simple paths, not just shortest ones: a 2-hop plus a 3-hop route to + the same target is genuinely ambiguous (auto-picking the shorter would + silently change join semantics). The DFS is confined to nodes that can + still reach ``target`` (reverse-reachability prune) and iterates + adjacency in sorted order; the visited set keeps it finite on cyclic + (symmetric INNER) graphs. ``root == target`` returns ``1`` (trivial + empty route).""" + if root == target: + return 1 + relevant = self._reverse_reachable_to(target) + if root not in relevant: + return 0 + + count = 0 + visited: set[str] = {root} + + def dfs(node: str) -> None: + nonlocal count + for nbr in sorted(self._adj.get(node, ())): # noqa: SIM118 — .get default + if count >= cap: + return + if nbr == target: + count += 1 + continue + if nbr in visited or nbr not in relevant: + continue + visited.add(nbr) + dfs(nbr) + visited.discard(nbr) + + dfs(root) + return min(count, cap) + def shortest_path(self, root: str, target: str) -> list[str] | None: """Return the hop-name sequence from ``root`` to ``target`` (excluding ``root``), or ``None`` if unreachable. diff --git a/slayer/engine/query_engine.py b/slayer/engine/query_engine.py index 8f2c0fc9..cfca3e39 100644 --- a/slayer/engine/query_engine.py +++ b/slayer/engine/query_engine.py @@ -20,7 +20,11 @@ from sqlglot import exp from slayer.core.enums import DEFAULT_AGGREGATIONS_BY_TYPE, DataType -from slayer.core.errors import AmbiguousModelError, ForcedFilterError +from slayer.core.errors import ( + AmbiguousModelError, + ForcedFilterError, + UnresolvableDimensionJoinError, +) from slayer.core.policy import JoinFilterRuleset, SessionPolicy from slayer.core.format import NumberFormat, NumberFormatType, format_number from slayer.core.models import ( @@ -2923,6 +2927,7 @@ async def _enrich( # NOSONAR S3776 — orchestrates resolve-callback closures + dialect: str | None = None, *, drop_unreachable_filters: bool = False, + enforce_join_binding: bool = True, ) -> EnrichedQuery: """Resolve a SlayerQuery against model definitions into an EnrichedQuery. @@ -2945,6 +2950,13 @@ async def _enrich( # NOSONAR S3776 — orchestrates resolve-callback closures + except Exception: # noqa: BLE001 — diagnostics only; never block enrichment pass + # DEV-1780: normalize + route dotted dimension / time-dimension refs + # before enrichment. Skipped for the re-rooted CTE and virtual stages. + if enforce_join_binding and model.source_model_origin is None: + query = await self._route_dotted_dimension_refs( + query=query, model=model, named_queries=named_queries or {}, + ) + async def _resolve_join_target(target_model_name, named_queries): nq = named_queries or {} if target_model_name in nq: @@ -3035,6 +3047,7 @@ async def _resolve_model_for_expansion(model_name, named_queries): resolve_model=_resolve_model_for_expansion, dialect=dialect, drop_unreachable_filters=drop_unreachable_filters, + enforce_join_binding=enforce_join_binding, ) # Post-process: build re-rooted enriched queries for cross-model measures @@ -3388,6 +3401,133 @@ async def _resolve_dimension_with_terminal( return None return col, terminal_model + async def _route_dotted_dimension_refs( + self, + *, + query: SlayerQuery, + model: SlayerModel, + named_queries: dict, + ) -> SlayerQuery: + """DEV-1780: normalize and route dotted dimension / time-dimension refs. + + A dotted path resolves only when every hop is a direct join. A SHORT + FORM (target model only, e.g. ``Consumer.name``) with exactly one route + to the target is rewritten to the full routed path (result key = full + path). Ambiguous / unreachable short forms and explicit chains with a + broken hop are rejected with a route-aware ``UnresolvableDimensionJoinError``. + + Routing only runs within a single datasource and is deferred when named- + query stages are in scope (their virtual models are not in the stored + graph); such refs fall through to enrichment's binding guard. Only + ``_NoJoinError`` triggers routing — pre-existing circular / missing-model + ``ValueError``s keep their own diagnostics. + """ + # Strip redundant source-model prefixes (``Invoice.status`` -> local, + # ``Invoice.A.col`` -> ``A.col``) so routing sees canonical refs. + query = query.strip_source_model_prefix() + if not (query.dimensions or query.time_dimensions): + return query + + can_route = bool(model.data_source) and not named_queries + graph: "JoinGraph | None" = None + rewrite: dict[tuple[str, str], str] = {} + + async def _route_ref(ref: ColumnRef) -> ColumnRef: + nonlocal graph + if ref.model is None: + return ref + segments = ref.model.split(".") + try: + await self._walk_join_chain( + source_model=model, hop_names=segments, + named_queries=named_queries, strict_missing_join=False, + ) + return ref # valid direct-join chain + except _NoJoinError: + pass + if not can_route: + return ref # defer to enrichment's binding guard + if graph is None: + graph = JoinGraph.build_from_models( + await self._load_candidate_models(data_source=model.data_source) + ) + target = segments[-1] + n_routes = graph.count_simple_paths(model.name, target) + route = graph.shortest_path(model.name, target) + if len(segments) == 1 and n_routes == 1 and route: + new_model = ".".join(route) + rewrite[(ref.model, ref.name)] = new_model + return ref.model_copy(update={"model": new_model}) + raise UnresolvableDimensionJoinError( + reference=f"{ref.model}.{ref.name}", + root_model=model.name, + reason=self._unresolvable_reason(target=target, n_routes=n_routes), + available_joins=[j.target_model for j in model.joins], + suggested_path=self._suggested_path( + target=target, leaf=ref.name, n_routes=n_routes, route=route, + ), + ) + + updates: dict[str, Any] = {} + if query.dimensions: + routed = [await _route_ref(d) for d in query.dimensions] + if any(a is not b for a, b in zip(routed, query.dimensions)): + updates["dimensions"] = routed + if query.time_dimensions: + new_tds, changed = [], False + for td in query.time_dimensions: + routed_dim = await _route_ref(td.dimension) + if routed_dim is not td.dimension: + new_tds.append(td.model_copy(update={"dimension": routed_dim})) + changed = True + else: + new_tds.append(td) + if changed: + updates["time_dimensions"] = new_tds + + # Keep dependent references consistent with the rewritten dimensions. + if rewrite: + if query.order: + new_order, order_changed = [], False + for item in query.order: + new_model = rewrite.get((item.column.model, item.column.name)) + if new_model is not None: + new_order.append(item.model_copy( + update={"column": item.column.model_copy(update={"model": new_model})} + )) + order_changed = True + else: + new_order.append(item) + if order_changed: + updates["order"] = new_order + if query.main_time_dimension and "." in query.main_time_dimension: + mtd_model, _, mtd_leaf = query.main_time_dimension.rpartition(".") + new_model = rewrite.get((mtd_model, mtd_leaf)) + if new_model is not None: + updates["main_time_dimension"] = f"{new_model}.{mtd_leaf}" + + return query.model_copy(update=updates) if updates else query + + @staticmethod + def _unresolvable_reason(*, target: str, n_routes: int) -> str | None: + if n_routes >= 2: + return f"'{target}' is reachable by multiple join paths." + if n_routes == 0: + return f"'{target}' is not reachable by any join." + return None + + @staticmethod + def _suggested_path( + *, target: str, leaf: str, n_routes: int, route: "list[str] | None" + ) -> str | None: + """Short form when the target is uniquely reachable; the shortest + deterministic full path when reachable by several routes; else none.""" + if n_routes == 1 and route: + return f"{target}.{leaf}" + if n_routes >= 2 and route: + return ".".join(route) + f".{leaf}" + return None + async def _walk_join_chain( self, *, @@ -3797,6 +3937,7 @@ async def _build_rerooted_enriched( model=target_model, named_queries=named_queries, drop_unreachable_filters=True, + enforce_join_binding=False, ) # --- Fix aliases to match main query's expectations --- diff --git a/tests/test_dev1780_missing_join_path.py b/tests/test_dev1780_missing_join_path.py new file mode 100644 index 00000000..ed51b05c --- /dev/null +++ b/tests/test_dev1780_missing_join_path.py @@ -0,0 +1,625 @@ +"""DEV-1780 — a dotted dimension/time-dimension whose hops are not all direct +joins must never emit invalid SQL (an unbound ``A__B`` alias in SELECT/GROUP BY +with no matching join). The engine now: + +* auto-resolves a SHORT-FORM ref (``Consumer.name`` — target model only) when + exactly one route reaches the target (result key = full routed path); +* rejects an ambiguous short form, an unreachable target, or an explicit + multi-hop chain with a broken hop, raising ``UnresolvableDimensionJoinError`` + with a route-aware suggestion; +* keeps a post-``_resolve_joins`` safety-net guard so ``enrich_query`` can never + return an EnrichedQuery with an unbound dimension alias. + +Scope: dimensions + time-dimensions only (filters / cross-model measures already +reject). Out of scope: multi-stage lenient fall-through; leaf-column-missing-on- +a-valid-path. +""" + +from __future__ import annotations + +import pytest +import sqlglot + +from slayer.core.enums import DataType, TimeGranularity +from slayer.core.errors import UnresolvableDimensionJoinError +from slayer.core.models import Column, DatasourceConfig, ModelJoin, SlayerModel +from slayer.core.query import ColumnRef, OrderItem, SlayerQuery, TimeDimension +from slayer.engine.enrichment import enrich_query +from slayer.engine.join_graph import JoinGraph +from slayer.engine.query_engine import SlayerQueryEngine +from slayer.sql.generator import SQLGenerator +from slayer.storage.yaml_storage import YAMLStorage + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _norm(s: str) -> str: + return " ".join(s.split()) + + +def _pk() -> Column: + return Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True) + + +def _d(name: str) -> Column: + return Column(name=name, sql=name, type=DataType.DOUBLE) + + +def _t(name: str) -> Column: + return Column(name=name, sql=name, type=DataType.TEXT) + + +async def _noop_async(**kw): # NOSONAR(S7503) — resolver-callback contract is async + return None + + +async def _save_chain( + storage: YAMLStorage, + *, + direct_customer: bool = False, + drop_customer_consumer: bool = False, +) -> SlayerModel: + """Invoice -> Subscription -> Customer -> Consumer. + + * ``direct_customer`` adds Invoice -> Customer, giving TWO routes to Consumer + (2-hop Customer.Consumer and 3-hop Subscription.Customer.Consumer). + * ``drop_customer_consumer`` removes Customer -> Consumer, leaving Consumer + unreachable from Invoice. + Returns the Invoice (root) model. + """ + await storage.save_model(SlayerModel( + name="Consumer", sql_table="Consumer", data_source="test", + columns=[_pk(), _t("name"), _t("email"), + Column(name="signup_at", sql="signup_at", type=DataType.TIMESTAMP)], + )) + customer_joins = ( + [] if drop_customer_consumer + else [ModelJoin(target_model="Consumer", join_pairs=[["consumerId", "id"]])] + ) + await storage.save_model(SlayerModel( + name="Customer", sql_table="Customer", data_source="test", + columns=[_pk(), _d("consumerId")], joins=customer_joins, + )) + await storage.save_model(SlayerModel( + name="Subscription", sql_table="Subscription", data_source="test", + columns=[_pk(), _d("customerId")], + joins=[ModelJoin(target_model="Customer", join_pairs=[["customerId", "id"]])], + )) + invoice_joins = [ModelJoin(target_model="Subscription", join_pairs=[["subscriptionId", "id"]])] + if direct_customer: + invoice_joins.append(ModelJoin(target_model="Customer", join_pairs=[["customerId", "id"]])) + invoice = SlayerModel( + name="Invoice", sql_table="Invoice", data_source="test", + columns=[_pk(), _d("subscriptionId"), _d("customerId"), _d("amount"), _t("status"), + Column(name="issued_at", sql="issued_at", type=DataType.TIMESTAMP)], + joins=invoice_joins, + ) + await storage.save_model(invoice) + return invoice + + +async def _engine(tmp_path, **knobs) -> tuple[SlayerQueryEngine, SlayerModel]: + storage = YAMLStorage(base_dir=str(tmp_path)) + invoice = await _save_chain(storage, **knobs) + return SlayerQueryEngine(storage=storage), invoice + + +async def _sql(engine: SlayerQueryEngine, query: SlayerQuery, model: SlayerModel) -> str: + enriched = await engine._enrich(query=query, model=model) + return SQLGenerator(dialect="postgres").generate(enriched=enriched) + + +def _amount_query(**kw) -> dict: + return dict(source_model="Invoice", measures=[{"formula": "amount:sum", "name": "amt"}], **kw) + + +# =========================================================================== +# Short-form auto-routing (unique) +# =========================================================================== + +class TestShortFormUniqueRoute: + async def test_short_form_unique_resolves_and_emits_all_joins(self, tmp_path) -> None: + """``Consumer.name`` with a single route resolves; every JOIN on the + routed chain is emitted and the SQL parses on Postgres.""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + )) + sql = _norm(await _sql(engine, query, invoice)) + assert "LEFT JOIN \"Subscription\" AS Subscription " in sql + " " + assert "AS Subscription__Customer " in sql + assert "AS Subscription__Customer__Consumer " in sql + # No unbound alias: the projected table alias is joined. + assert "Subscription__Customer__Consumer.name" in sql + sqlglot.parse_one(sql, dialect="postgres") + + async def test_short_form_unique_result_key_is_full_routed_path(self, tmp_path) -> None: + """Approved: the result column key for a resolved short form is the + FULL routed path, not the short form the user typed.""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + )) + enriched = await engine._enrich(query=query, model=invoice) + dim = enriched.dimensions[0] + assert dim.alias == "Invoice.Subscription.Customer.Consumer.name" + assert dim.model_name == "Subscription__Customer__Consumer" + + async def test_short_form_time_dimension_unique_resolves(self, tmp_path) -> None: + """A short-form TIME dimension resolves via its unique route too.""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery( + source_model="Invoice", + measures=[{"formula": "amount:sum", "name": "amt"}], + time_dimensions=[TimeDimension( + dimension=ColumnRef(name="signup_at", model="Consumer"), + granularity=TimeGranularity.MONTH, + )], + ) + sql = _norm(await _sql(engine, query, invoice)) + assert "AS Subscription__Customer__Consumer " in sql + sqlglot.parse_one(sql, dialect="postgres") + + +# =========================================================================== +# Rejections (ambiguous / unreachable / broken explicit chain) +# =========================================================================== + +class TestRejections: + async def test_short_form_ambiguous_rejects_and_suggests_shortest(self, tmp_path) -> None: + """Two routes reach Consumer → the short form is ambiguous → reject and + suggest the shortest deterministic full path (``Customer.Consumer.name``).""" + engine, invoice = await _engine(tmp_path, direct_customer=True) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + )) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await engine._enrich(query=query, model=invoice) + err = ei.value + assert err.suggested_path == "Customer.Consumer.name" + assert "multiple" in str(err).lower() + + async def test_short_form_unreachable_rejects_without_suggestion(self, tmp_path) -> None: + """Target not reachable by any join → reject with no suggestion.""" + engine, invoice = await _engine(tmp_path, drop_customer_consumer=True) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + )) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await engine._enrich(query=query, model=invoice) + err = ei.value + assert err.suggested_path is None + assert "Did you mean" not in str(err) + + async def test_ticket_shape_explicit_broken_chain_suggests_short_form(self, tmp_path) -> None: + """The DEV-1780 repro: ``Customer.Consumer.name`` where Invoice has no + direct Customer join. The explicit chain is broken → reject (never + auto-fixed); Consumer is uniquely reachable → suggest the short form.""" + engine, invoice = await _engine(tmp_path) # no direct Customer join + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Customer.Consumer")], + )) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await engine._enrich(query=query, model=invoice) + err = ei.value + assert err.reference == "Customer.Consumer.name" + assert err.suggested_path == "Consumer.name" + assert "Did you mean 'Consumer.name'" in str(err) + + async def test_explicit_broken_chain_ambiguous_target_suggests_shortest_full_path( + self, tmp_path + ) -> None: + """Broken explicit chain (``Subscription.Consumer`` — Subscription has no + direct Consumer join) whose target Consumer is reachable by >=2 routes → + reject and suggest the shortest full path.""" + engine, invoice = await _engine(tmp_path, direct_customer=True) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Subscription.Consumer")], + )) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await engine._enrich(query=query, model=invoice) + assert ei.value.suggested_path == "Customer.Consumer.name" + + async def test_broken_time_dimension_chain_rejects(self, tmp_path) -> None: + """The invalid-SQL hole applies to time dimensions too: an explicit + broken chain time-dim rejects (uniquely-reachable target → short-form + suggestion).""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery( + source_model="Invoice", + measures=[{"formula": "amount:sum", "name": "amt"}], + time_dimensions=[TimeDimension( + dimension=ColumnRef(name="signup_at", model="Customer.Consumer"), + granularity=TimeGranularity.MONTH, + )], + ) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await engine._enrich(query=query, model=invoice) + assert ei.value.suggested_path == "Consumer.signup_at" + + async def test_error_message_lists_available_root_joins(self, tmp_path) -> None: + engine, invoice = await _engine(tmp_path) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Customer.Consumer")], + )) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await engine._enrich(query=query, model=invoice) + # Root Invoice's own joins are surfaced as a hint. + assert "Subscription" in str(ei.value) + + +# =========================================================================== +# Order-by / main_time_dimension consistency (Codex High #1) +# =========================================================================== + +class TestOrderAndMainTimeConsistency: + async def test_short_form_with_matching_order_by_resolves(self, tmp_path) -> None: + """Projecting a short-form dim AND ordering by the same short form must + stay consistent (the order ref is rewritten with the dim) — the ORDER BY + binds to the full routed projection key, not the unbound short form.""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + order=[OrderItem(column=ColumnRef(name="name", model="Consumer"), direction="desc")], + )) + sql = _norm(await _sql(engine, query, invoice)) + order_tail = sql.split("ORDER BY", 1)[1] + assert '"Invoice.Subscription.Customer.Consumer.name"' in order_tail + sqlglot.parse_one(sql, dialect="postgres") + + async def test_short_form_main_time_dimension_is_rewritten(self, tmp_path) -> None: + """A routed short-form time dimension selected via ``main_time_dimension`` + must have that reference rewritten to the full routed path so the + resolved time axis matches the enriched time-dimension alias.""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery( + source_model="Invoice", + measures=[{"formula": "cumsum(amount:sum)", "name": "cs"}], + time_dimensions=[ + TimeDimension(dimension=ColumnRef(name="issued_at"), + granularity=TimeGranularity.MONTH), + TimeDimension(dimension=ColumnRef(name="signup_at", model="Consumer"), + granularity=TimeGranularity.MONTH), + ], + main_time_dimension="Consumer.signup_at", + ) + enriched = await engine._enrich(query=query, model=invoice) + # The cumsum transform's time axis resolves to the routed dim's alias — + # proving main_time_dimension was rewritten (else it would be the unbound + # "Invoice.Consumer.signup_at"). + assert enriched.transforms[0].time_alias == "Invoice.Subscription.Customer.Consumer.signup_at" + + +# =========================================================================== +# Regressions — valid paths must be untouched +# =========================================================================== + +class TestValidPathsUnchanged: + async def test_valid_explicit_two_hop_unchanged(self, tmp_path) -> None: + """A fully-direct explicit two-hop chain resolves exactly as before.""" + engine, invoice = await _engine(tmp_path, direct_customer=True) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Customer.Consumer")], + )) + enriched = await engine._enrich(query=query, model=invoice) + dim = enriched.dimensions[0] + assert dim.alias == "Invoice.Customer.Consumer.name" + assert dim.model_name == "Customer__Consumer" + sql = _norm(SQLGenerator(dialect="postgres").generate(enriched=enriched)) + assert "AS Customer__Consumer " in sql + sqlglot.parse_one(sql, dialect="postgres") + + async def test_missing_terminal_column_on_valid_path_unchanged(self, tmp_path) -> None: + """A missing leaf column on an otherwise-valid path is a DIFFERENT issue + (the join alias IS bound). The pre-existing lenient behavior is + unchanged: enrichment succeeds with the bound alias — no + UnresolvableDimensionJoinError.""" + engine, invoice = await _engine(tmp_path, direct_customer=True) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="does_not_exist", model="Customer.Consumer")], + )) + enriched = await engine._enrich(query=query, model=invoice) # must not raise + assert enriched.dimensions[0].model_name == "Customer__Consumer" + + async def test_self_qualified_root_col_is_local(self, tmp_path) -> None: + """A self-qualified ``Invoice.status`` normalizes to a local ref (no + circular-join error, no routing).""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="status", model="Invoice")], + )) + enriched = await engine._enrich(query=query, model=invoice) + assert enriched.dimensions[0].model_name == "Invoice" + + async def test_root_prefixed_explicit_chain_normalized(self, tmp_path) -> None: + """``Invoice.Customer.Consumer.name`` (root-prefixed) normalizes to the + valid ``Customer.Consumer`` chain.""" + engine, invoice = await _engine(tmp_path, direct_customer=True) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Invoice.Customer.Consumer")], + )) + enriched = await engine._enrich(query=query, model=invoice) + assert enriched.dimensions[0].model_name == "Customer__Consumer" + + +# =========================================================================== +# Internal enrichments unaffected (re-rooting / stage) +# =========================================================================== + +class TestInternalEnrichmentsUnaffected: + async def test_cross_model_rerooting_still_enriches(self, tmp_path) -> None: + """A cross-model measure with a shared source-local dim (the re-rooting + shape) must still enrich without raising — the re-rooted CTE carries + ``orders.status`` which never binds to a base-table join.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + await storage.save_model(SlayerModel( + name="customers", sql_table="customers", data_source="test", + columns=[_pk(), _d("revenue")], + )) + orders = SlayerModel( + name="orders", sql_table="orders", data_source="test", + columns=[_pk(), _d("customer_id"), _t("status")], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) + await storage.save_model(orders) + engine = SlayerQueryEngine(storage=storage) + query = SlayerQuery( + source_model="orders", + dimensions=[ColumnRef(name="status")], + measures=[{"formula": "customers.revenue:sum", "name": "cust_rev"}], + ) + enriched = await engine._enrich(query=query, model=orders) # must not raise + assert enriched.cross_model_measures + + async def test_multi_stage_unresolved_ref_still_falls_through(self, tmp_path) -> None: + """An outer stage referencing a dotted dim the inner stage did not + project stays a lenient fall-through (virtual-stage exclusion) — no + UnresolvableDimensionJoinError.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + await storage.save_datasource(DatasourceConfig(name="test", type="sqlite", database=":memory:")) + await _save_chain(storage) # Invoice -> Subscription -> Customer -> Consumer + engine = SlayerQueryEngine(storage=storage) + inner = SlayerQuery( + name="s1", source_model="Invoice", + dimensions=[ColumnRef(name="name", model="Subscription.Customer.Consumer")], + measures=[{"formula": "*:count"}], + ) + outer = SlayerQuery( + source_model="s1", + dimensions=[ColumnRef(name="email", model="Subscription.Customer.Consumer")], + measures=[{"formula": "*:count"}], + ) + resp = await engine.execute(query=[inner, outer], dry_run=True) # must not raise our error + assert resp.sql is not None + + +# =========================================================================== +# Enrichment safety-net guard (direct enrich_query caller) +# =========================================================================== + +class TestEnrichmentGuard: + @staticmethod + def _ghost_model() -> SlayerModel: + return SlayerModel( + name="Invoice", sql_table="Invoice", data_source="test", + columns=[_pk(), _d("amount"), + Column(name="issued_at", sql="issued_at", type=DataType.TIMESTAMP)], + ) + + async def test_direct_enrich_query_rejects_unbound_dim_alias(self) -> None: + """Called directly (bypassing the engine routing pre-pass), enrich_query + must never return an EnrichedQuery with an unbound dim alias. The base + error names the user's reference (not the internal alias) and carries no + route (enrich_query has no graph).""" + query = SlayerQuery( + source_model="Invoice", + measures=[{"formula": "amount:sum", "name": "amt"}], + dimensions=[ColumnRef(name="x", model="Ghost")], + ) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await enrich_query( + query=query, model=self._ghost_model(), + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + err = ei.value + assert err.reference == "Ghost.x" + assert err.root_model == "Invoice" + assert err.suggested_path is None + assert "Did you mean" not in str(err) + + async def test_guard_covers_time_dimensions(self) -> None: + query = SlayerQuery( + source_model="Invoice", + measures=[{"formula": "amount:sum", "name": "amt"}], + time_dimensions=[TimeDimension( + dimension=ColumnRef(name="issued_at", model="Ghost"), + granularity=TimeGranularity.MONTH, + )], + ) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await enrich_query( + query=query, model=self._ghost_model(), + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + assert ei.value.reference == "Ghost.issued_at" + + async def test_guard_reports_first_unbound_dimension(self) -> None: + query = SlayerQuery( + source_model="Invoice", + measures=[{"formula": "amount:sum", "name": "amt"}], + dimensions=[ColumnRef(name="a", model="Ghost1"), + ColumnRef(name="b", model="Ghost2")], + ) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await enrich_query( + query=query, model=self._ghost_model(), + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + assert ei.value.reference == "Ghost1.a" + + async def test_guard_suppressed_when_enforce_join_binding_false(self) -> None: + """The guard is off for the internal re-rooted path (enforce_join_binding + =False) so deliberately-carried unbound shared dims survive.""" + query = SlayerQuery( + source_model="Invoice", + measures=[{"formula": "amount:sum", "name": "amt"}], + dimensions=[ColumnRef(name="x", model="Ghost")], + ) + enriched = await enrich_query( + query=query, model=self._ghost_model(), + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + enforce_join_binding=False, + ) + assert enriched.dimensions[0].model_name == "Ghost" + + +# =========================================================================== +# Routing gates / safety limits +# =========================================================================== + +class TestRoutingGates: + async def test_no_auto_routing_when_named_queries_present(self, tmp_path) -> None: + """Short-form auto-routing is disabled when named-query stages are in + scope (Option A — deferred). The ref falls to the binding guard and is + rejected rather than routed.""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + )) + named = {"sibling": SlayerQuery(source_model="Invoice", measures=[{"formula": "*:count"}])} + with pytest.raises(UnresolvableDimensionJoinError): + await engine._enrich(query=query, model=invoice, named_queries=named) + + async def test_routing_is_datasource_scoped(self, tmp_path) -> None: + """Routing only considers models in the root's datasource. A target that + lives in a DIFFERENT datasource is not a candidate route (cross-datasource + joins aren't executable) — the short form is rejected as unreachable.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + # Consumer lives in a different datasource, so the "test" graph can't + # reach it even though Customer declares a join to it. + await storage.save_model(SlayerModel( + name="Consumer", sql_table="Consumer", data_source="other", + columns=[_pk(), _t("name")], + )) + await storage.save_model(SlayerModel( + name="Customer", sql_table="Customer", data_source="test", + columns=[_pk(), _d("consumerId")], + joins=[ModelJoin(target_model="Consumer", join_pairs=[["consumerId", "id"]])], + )) + invoice = SlayerModel( + name="Invoice", sql_table="Invoice", data_source="test", + columns=[_pk(), _d("customerId"), _d("amount")], + joins=[ModelJoin(target_model="Customer", join_pairs=[["customerId", "id"]])], + ) + await storage.save_model(invoice) + engine = SlayerQueryEngine(storage=storage) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + )) + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await engine._enrich(query=query, model=invoice) + assert ei.value.suggested_path is None + + +class TestPrePassPurity: + async def test_pre_pass_does_not_mutate_input_query(self, tmp_path) -> None: + """Routing rewrites a COPY — the caller's query keeps the short forms.""" + engine, invoice = await _engine(tmp_path) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + order=[OrderItem(column=ColumnRef(name="name", model="Consumer"), direction="asc")], + )) + await engine._enrich(query=query, model=invoice) + assert query.dimensions[0].model == "Consumer" + assert query.order[0].column.model == "Consumer" + + +class TestDiagnosticsPreserved: + async def test_repeated_hop_keeps_circular_error(self, tmp_path) -> None: + """A repeated-hop path is a pre-existing circular-join error; it must keep + its own diagnostic and NOT be converted into + UnresolvableDimensionJoinError (the pre-pass catches only _NoJoinError).""" + engine, invoice = await _engine(tmp_path, direct_customer=True) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Customer.Customer")], + )) + with pytest.raises(ValueError) as ei: + await engine._enrich(query=query, model=invoice) + assert not isinstance(ei.value, UnresolvableDimensionJoinError) + assert "ircular" in str(ei.value) + + +# =========================================================================== +# Error class contract +# =========================================================================== + +class TestErrorContract: + def test_is_value_error_and_exposes_fields(self) -> None: + err = UnresolvableDimensionJoinError( + reference="Customer.Consumer.name", + root_model="Invoice", + reason="'Consumer' is reachable by multiple join paths.", + available_joins=["Subscription"], + suggested_path="Consumer.name", + ) + assert isinstance(err, ValueError) + assert err.reference == "Customer.Consumer.name" + assert err.root_model == "Invoice" + assert err.suggested_path == "Consumer.name" + assert "Did you mean 'Consumer.name'" in str(err) + + async def test_raised_error_caught_as_value_error(self, tmp_path) -> None: + engine, invoice = await _engine(tmp_path, drop_customer_consumer=True) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + )) + with pytest.raises(ValueError): + await engine._enrich(query=query, model=invoice) + + +# =========================================================================== +# JoinGraph.count_simple_paths unit +# =========================================================================== + +class TestCountSimplePaths: + def test_unique(self) -> None: + g = JoinGraph({"A": {"B"}, "B": {"C"}, "C": set()}) + assert g.count_simple_paths("A", "C") == 1 + + def test_diamond_is_ambiguous(self) -> None: + g = JoinGraph({"A": {"B", "C"}, "B": {"D"}, "C": {"D"}, "D": set()}) + assert g.count_simple_paths("A", "D") == 2 + + def test_two_hop_plus_three_hop_is_ambiguous(self) -> None: + # A->D direct-ish (via B) and A->C->... both reach D + g = JoinGraph({"A": {"B", "C"}, "B": {"D"}, "C": {"B"}, "D": set()}) + assert g.count_simple_paths("A", "D") == 2 + + def test_unreachable(self) -> None: + g = JoinGraph({"A": {"B"}, "B": set(), "C": set()}) + assert g.count_simple_paths("A", "C") == 0 + + def test_root_equals_target(self) -> None: + g = JoinGraph({"A": {"B"}, "B": set()}) + # A path from A to itself is the single trivial empty route. + assert g.count_simple_paths("A", "A") == 1 + + def test_symmetric_cycle_is_finite_and_counts_one_route(self) -> None: + # Symmetric INNER edges A<->B, B<->C: exactly one simple route A->C. + g = JoinGraph({"A": {"B"}, "B": {"A", "C"}, "C": {"B"}}) + assert g.count_simple_paths("A", "C") == 1 + + def test_cap_limits_work(self) -> None: + # Many parallel routes A->{B1,B2,B3}->D: capped at 2. + g = JoinGraph({"A": {"B1", "B2", "B3"}, "B1": {"D"}, "B2": {"D"}, "B3": {"D"}, "D": set()}) + assert g.count_simple_paths("A", "D", cap=2) == 2 From 0b46cc4f69c3a476e1adc7b639228ee47d7d8253 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Wed, 12 Aug 2026 17:14:38 +0200 Subject: [PATCH 3/4] 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 From 9be039d7f00dfbe2c1d5e02dbd5aa622ec81cff4 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Wed, 12 Aug 2026 17:34:06 +0200 Subject: [PATCH 4/4] fix(DEV-1780): address Codex + Sonar review feedback - Route dependent references by model, not (model, leaf), so an order / main_time_dimension ref to any column on a routed model stays consistent with the rewritten dimension (Codex). - Build the routing graph with the passed root substituting its stored namesake, so inline / ModelExtension joins are honored and a uniquely reachable short form is not wrongly rejected (Codex). - Split _route_dotted_dimension_refs into _route_one_ref / _route_time_dimension_list / _graph_models / _rewrite_dependent_refs to drop cognitive complexity below threshold (Sonar S3776). - Hoist _ghost_model() out of the pytest.raises blocks so each has a single throwing invocation (Sonar S5778). --- slayer/engine/query_engine.py | 176 ++++++++++++++---------- tests/test_dev1780_missing_join_path.py | 39 +++++- 2 files changed, 141 insertions(+), 74 deletions(-) diff --git a/slayer/engine/query_engine.py b/slayer/engine/query_engine.py index cfca3e39..c44d06f8 100644 --- a/slayer/engine/query_engine.py +++ b/slayer/engine/query_engine.py @@ -3428,86 +3428,120 @@ async def _route_dotted_dimension_refs( if not (query.dimensions or query.time_dimensions): return query - can_route = bool(model.data_source) and not named_queries - graph: "JoinGraph | None" = None - rewrite: dict[tuple[str, str], str] = {} - - async def _route_ref(ref: ColumnRef) -> ColumnRef: - nonlocal graph - if ref.model is None: - return ref - segments = ref.model.split(".") - try: - await self._walk_join_chain( - source_model=model, hop_names=segments, - named_queries=named_queries, strict_missing_join=False, - ) - return ref # valid direct-join chain - except _NoJoinError: - pass - if not can_route: - return ref # defer to enrichment's binding guard - if graph is None: - graph = JoinGraph.build_from_models( - await self._load_candidate_models(data_source=model.data_source) - ) - target = segments[-1] - n_routes = graph.count_simple_paths(model.name, target) - route = graph.shortest_path(model.name, target) - if len(segments) == 1 and n_routes == 1 and route: - new_model = ".".join(route) - rewrite[(ref.model, ref.name)] = new_model - return ref.model_copy(update={"model": new_model}) - raise UnresolvableDimensionJoinError( - reference=f"{ref.model}.{ref.name}", - root_model=model.name, - reason=self._unresolvable_reason(target=target, n_routes=n_routes), - available_joins=[j.target_model for j in model.joins], - suggested_path=self._suggested_path( - target=target, leaf=ref.name, n_routes=n_routes, route=route, - ), - ) - + rewrite: dict[str, str] = {} # short model -> full routed model + kw = { + "can_route": bool(model.data_source) and not named_queries, + "rewrite": rewrite, + "graph_cache": {}, # lazily holds the datasource JoinGraph + } updates: dict[str, Any] = {} if query.dimensions: - routed = [await _route_ref(d) for d in query.dimensions] + routed = [await self._route_one_ref(d, model, named_queries, **kw) for d in query.dimensions] if any(a is not b for a, b in zip(routed, query.dimensions)): updates["dimensions"] = routed if query.time_dimensions: - new_tds, changed = [], False - for td in query.time_dimensions: - routed_dim = await _route_ref(td.dimension) - if routed_dim is not td.dimension: - new_tds.append(td.model_copy(update={"dimension": routed_dim})) - changed = True - else: - new_tds.append(td) - if changed: + new_tds = await self._route_time_dimension_list( + query.time_dimensions, model, named_queries, **kw + ) + if new_tds is not None: updates["time_dimensions"] = new_tds - - # Keep dependent references consistent with the rewritten dimensions. if rewrite: - if query.order: - new_order, order_changed = [], False - for item in query.order: - new_model = rewrite.get((item.column.model, item.column.name)) - if new_model is not None: - new_order.append(item.model_copy( - update={"column": item.column.model_copy(update={"model": new_model})} - )) - order_changed = True - else: - new_order.append(item) - if order_changed: - updates["order"] = new_order - if query.main_time_dimension and "." in query.main_time_dimension: - mtd_model, _, mtd_leaf = query.main_time_dimension.rpartition(".") - new_model = rewrite.get((mtd_model, mtd_leaf)) - if new_model is not None: - updates["main_time_dimension"] = f"{new_model}.{mtd_leaf}" - + updates.update(self._rewrite_dependent_refs(query=query, rewrite=rewrite)) return query.model_copy(update=updates) if updates else query + async def _route_one_ref( + self, + ref: ColumnRef, + model: SlayerModel, + named_queries: dict, + *, + can_route: bool, + rewrite: dict, + graph_cache: dict, + ) -> ColumnRef: + """Route one dotted ref: return it unchanged (valid chain / deferred), + rewrite a uniquely-routed short form to its full path, or raise.""" + if ref.model is None: + return ref + segments = ref.model.split(".") + try: + await self._walk_join_chain( + source_model=model, hop_names=segments, + named_queries=named_queries, strict_missing_join=False, + ) + return ref # valid direct-join chain + except _NoJoinError: + pass + if not can_route: + return ref # defer to enrichment's binding guard + graph = graph_cache.get("graph") + if graph is None: + graph = graph_cache["graph"] = JoinGraph.build_from_models( + await self._graph_models(model) + ) + target = segments[-1] + n_routes = graph.count_simple_paths(model.name, target) + route = graph.shortest_path(model.name, target) + if len(segments) == 1 and n_routes == 1 and route: + new_model = ".".join(route) + rewrite[ref.model] = new_model + return ref.model_copy(update={"model": new_model}) + raise UnresolvableDimensionJoinError( + reference=f"{ref.model}.{ref.name}", + root_model=model.name, + reason=self._unresolvable_reason(target=target, n_routes=n_routes), + available_joins=[j.target_model for j in model.joins], + suggested_path=self._suggested_path( + target=target, leaf=ref.name, n_routes=n_routes, route=route, + ), + ) + + async def _route_time_dimension_list( + self, time_dims: list, model: SlayerModel, named_queries: dict, **kw + ) -> "list | None": + """Route each time-dim's ColumnRef; return the new list or ``None`` if + nothing changed.""" + out, changed = [], False + for td in time_dims: + routed = await self._route_one_ref(td.dimension, model, named_queries, **kw) + if routed is not td.dimension: + out.append(td.model_copy(update={"dimension": routed})) + changed = True + else: + out.append(td) + return out if changed else None + + async def _graph_models(self, model: SlayerModel) -> list: + """Routing candidates: stored models in the datasource, with the passed + root substituting its stored namesake so inline / ModelExtension joins + are honored (a stored graph node would miss them).""" + stored = await self._load_candidate_models(data_source=model.data_source) + return [m for m in stored if m.name != model.name] + [model] + + def _rewrite_dependent_refs(self, *, query: SlayerQuery, rewrite: dict) -> dict: + """Propagate short->full model rewrites to matching order items and + ``main_time_dimension`` so dependent references stay consistent.""" + updates: dict[str, Any] = {} + if query.order: + new_order, changed = [], False + for item in query.order: + new_model = rewrite.get(item.column.model) + if new_model is not None: + new_order.append(item.model_copy( + update={"column": item.column.model_copy(update={"model": new_model})} + )) + changed = True + else: + new_order.append(item) + if changed: + updates["order"] = new_order + if query.main_time_dimension and "." in query.main_time_dimension: + mtd_model, _, mtd_leaf = query.main_time_dimension.rpartition(".") + new_model = rewrite.get(mtd_model) + if new_model is not None: + updates["main_time_dimension"] = f"{new_model}.{mtd_leaf}" + return updates + @staticmethod def _unresolvable_reason(*, target: str, n_routes: int) -> str | None: if n_routes >= 2: diff --git a/tests/test_dev1780_missing_join_path.py b/tests/test_dev1780_missing_join_path.py index ed51b05c..b24a8ff5 100644 --- a/tests/test_dev1780_missing_join_path.py +++ b/tests/test_dev1780_missing_join_path.py @@ -418,9 +418,10 @@ async def test_direct_enrich_query_rejects_unbound_dim_alias(self) -> None: measures=[{"formula": "amount:sum", "name": "amt"}], dimensions=[ColumnRef(name="x", model="Ghost")], ) + model = self._ghost_model() with pytest.raises(UnresolvableDimensionJoinError) as ei: await enrich_query( - query=query, model=self._ghost_model(), + query=query, model=model, resolve_dimension_via_joins=_noop_async, resolve_cross_model_measure=_noop_async, resolve_join_target=_noop_async, @@ -440,9 +441,10 @@ async def test_guard_covers_time_dimensions(self) -> None: granularity=TimeGranularity.MONTH, )], ) + model = self._ghost_model() with pytest.raises(UnresolvableDimensionJoinError) as ei: await enrich_query( - query=query, model=self._ghost_model(), + query=query, model=model, resolve_dimension_via_joins=_noop_async, resolve_cross_model_measure=_noop_async, resolve_join_target=_noop_async, @@ -456,9 +458,10 @@ async def test_guard_reports_first_unbound_dimension(self) -> None: dimensions=[ColumnRef(name="a", model="Ghost1"), ColumnRef(name="b", model="Ghost2")], ) + model = self._ghost_model() with pytest.raises(UnresolvableDimensionJoinError) as ei: await enrich_query( - query=query, model=self._ghost_model(), + query=query, model=model, resolve_dimension_via_joins=_noop_async, resolve_cross_model_measure=_noop_async, resolve_join_target=_noop_async, @@ -531,6 +534,36 @@ async def test_routing_is_datasource_scoped(self, tmp_path) -> None: assert ei.value.suggested_path is None +class TestInlineModelJoins: + async def test_routing_honors_passed_root_joins_not_stored(self, tmp_path) -> None: + """The routing graph must use the passed root's joins (which may include + inline / ModelExtension joins absent from storage). Here the stored graph + has no Invoice node; the inline Invoice's join makes Consumer uniquely + reachable, so the short form resolves instead of being wrongly rejected.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + await storage.save_model(SlayerModel( + name="Consumer", sql_table="Consumer", data_source="test", + columns=[_pk(), _t("name")], + )) + await storage.save_model(SlayerModel( + name="Customer", sql_table="Customer", data_source="test", + columns=[_pk(), _d("consumerId")], + joins=[ModelJoin(target_model="Consumer", join_pairs=[["consumerId", "id"]])], + )) + # Invoice is NOT saved — it only exists as the inline model passed in. + invoice = SlayerModel( + name="Invoice", sql_table="Invoice", data_source="test", + columns=[_pk(), _d("customerId"), _d("amount")], + joins=[ModelJoin(target_model="Customer", join_pairs=[["customerId", "id"]])], + ) + engine = SlayerQueryEngine(storage=storage) + query = SlayerQuery(**_amount_query( + dimensions=[ColumnRef(name="name", model="Consumer")], + )) + enriched = await engine._enrich(query=query, model=invoice) # must not raise + assert enriched.dimensions[0].model_name == "Customer__Consumer" + + class TestPrePassPurity: async def test_pre_pass_does_not_mutate_input_query(self, tmp_path) -> None: """Routing rewrites a COPY — the caller's query keeps the short forms."""