diff --git a/DECISIONS.md b/DECISIONS.md index 8daf6350..4b37b87d 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -73,3 +73,5 @@ implementation detail. Include issue refs when known. - 2026-08-06 — Dialect-aware identifier-length fitting (DEV-1756): every dialect declares a conservative universal budget as `SqlDialect.max_identifier_bytes` (postgres 63, mysql 64, redshift 127, oracle/tsql 128, snowflake 255, duckdb 256, bigquery 300, `None` = unbounded for sqlite/clickhouse/trino/presto/databricks/spark), and an over-limit identifier is shortened at emission to `__` via `slayer/sql/dialects/_identifier_fit.py`. Postgres is the binding case and the reason this is a correctness fix rather than cosmetics: it truncates over-length identifiers **silently** (a NOTICE, never an error), so two 3-hop aliases sharing a 63-byte prefix either blow up as `AmbiguousColumnError` under the DEV-1444 outer wrap or — with no sibling to collide with — quietly return the column under a name the engine never looks up. Shorten **only when over the limit**, never uniformly: `decode_result_keys` restores the canonical dotted alias, so the dialect-dependence the issue worried about is invisible to consumers and the 99% case keeps byte-identical SQL (pinned by pre-change goldens, not merely by idempotence). Both ends of the alias are kept because the reported colliding pair differs ONLY in its final segment — a head-only truncation would render the two indistinguishable in `dry_run` output. The digest is sha256 of the FULL original, which is what makes `fit_identifier` a pure function of the name and lets the read side rebuild the emitted→canonical map by re-running it, with no map threaded through generation (the alternative the issue sketched). Write side is an EXACT-match replacement over the query's own alias set (`all_projection_aliases`, unfiltered — hidden ORDER-BY hoists and `_inner_*`/`_ft*`/`_ts*` entries are projected in the inner SELECT and truncate identically), never a length regex over arbitrary SQL, so a long quoted-looking span inside a string literal can never be corrupted; substitution is two-phase (canonical→sentinel→final) as defence-in-depth, though today's key set (over-limit) and value set (within-limit) are provably disjoint. BigQuery/T-SQL compose by running their existing dot-mangle regex AFTER the base length pass: an under-limit alias makes the length pass a genuine no-op so their output is unchanged, and an over-limit one arrives still-dotted and gets mangled by the same regex — no double-encoding — with the budget sized against `encode_alias` so the post-mangle form still fits. Scope is the three OUTPUT-name surfaces: projection aliases, CTE names (`_cte_name_from_alias` fits the whole result, prefix included, allocated through a per-statement collision-checked `SQLGenerator._cte_name`), and `_query_as_model` virtual-model shorts. Join-path TABLE aliases are deferred to DEV-1743, whose plan already owns the `__` path-alias allocator; their failure mode (silent wrong joins) is worse but the fix requires decoupling `EnrichedDimension.model_name` from the emitted qualifier across ~8 sites that split it back on `__`. Collisions raise `IdentifierCollisionError` rather than emitting ambiguous SQL — the check covers identity entries too, since an already-short alias equal to another's fitted form is a duplicate no hash width can prevent. Fixed alongside, in the same code path: `_query_as_model` no longer decides its short alias's quoting with a hand-written predicate. The contract is AGREEMENT between the emitted `AS ` and the downstream `Column(sql=short)` reference — they must resolve to the same column — so the short is now run through the same two mechanisms the reference side uses: `SQLGenerator._maybe_quote_ident` (DEV-1645: quote iff the name contains an uppercase letter) followed by `Identifier.sql(dialect=...)`, which lets sqlglot add quotes for words reserved IN THE TARGET DIALECT and for names that are not safe bare identifiers. Emitted bare, a mixed-case short was case-folded by Postgres while the outer stage referenced it quoted, making any query-backed model with a mixed-case join path unqueryable (`UndefinedColumnError`, reproduced on a live server). Two wrong answers were tried and rejected on the way: quoting UNCONDITIONALLY defines a case-sensitive `"status"` on upper-folding backends (Snowflake, Oracle) while the bare reference still resolves as `STATUS`, trading a Postgres bug for a Snowflake one; and quoting on `SLAYER_RESERVED_KEYWORDS` alone misses words reserved only NATIVELY (`index`, `int`, `rows` on MySQL; `rows` on BigQuery), where the reference side quotes but a bare `AS index` is a syntax error — `install_reserved_keywords` unions our set INTO each generator's, so only the generator knows the full set. Shape matters too: `Column.name` forbids only `.` and `:`, so `1abc` or `foo bar` needs quoting on form alone. Both axes must therefore be dialect-driven, and the rule is pinned by a 12-dialect × 7-name test asserting the two spellings are byte-identical. Because an all-lowercase short stays bare and still shares a case-folded namespace, the short-uniqueness check remains case-insensitive. - 2026-08-06 — `mcp` capped at `>=1.0,<2` (DEV-1757). mcp 2.0.0 renamed `mcp.server.fastmcp` → `mcp.server.mcpserver` (`FastMCP` → `MCPServer`), so the unbounded `mcp = ">=1.0"` let every lockfile-free install (`pip install motley-slayer`, `uv tool install`) resolve a major `slayer/mcp/server.py` cannot import — a broken MCP server, SLayer's primary agent-facing interface, on every fresh install. `poetry.lock` pinned a 1.x, so CI was green throughout and only users saw it; the guard added here (`tests/test_mcp_dependency_pin.py`) is therefore **declaration-level** — it parses the pyproject constraint and asserts 2.0.0 falls outside it, since no in-repo test can execute an mcp major the environment does not have. Two consequences accepted deliberately: users needing mcp≥2 for another package in the same environment now hit a resolver conflict instead of a runtime crash, and other core deps were left unbounded — capping the rest was considered and rejected (caps rot and produce unresolvable trees downstream), which is the scope of this change and NOT a standing no-caps policy. The import failure now diagnoses itself: absent package, wrong major, and a 1.x that failed to import for some other reason are three distinct messages, all offering `pip install 'mcp>=1.0,<2'` first and "upgrade SLayer" second — the old "Reinstall SLayer: pip install motley-slayer" text told users to do the one thing that reproduced the failure, and leading with the upgrade hint would recreate that for anyone already on the latest release. Separately, `serverInfo.version` now reports SLayer's own version: FastMCP 1.x exposes no `version` kwarg and never forwards one to the lowlevel `Server`, which falls back to `pkg_version("mcp")`, so SLayer was announcing the SDK's version as its own. The stamp writes the private `_mcp_server.version` (the only route in 1.x; the file already sets `_slayer_engine` on the same object per DEV-1656) and tolerates both a missing attribute and a read-only one, so a future SDK cannot abort server construction over a cosmetic field. Migrating to the 2.x `MCPServer` API — which would retire that private write via its public `version=` kwarg, and also pulls in `Context` injection, worker-thread sync handlers, snake_case `mcp.types`, and an httpx→httpx2 / pydantic≥2.12 / opentelemetry dependency shift — is deferred, as is removing the deprecated `inspect_model` tool. - 2026-08-06 — Recognised ELT/migration housekeeping tables ingest hidden (DEV-1759). An unfiltered ingest modelled `_dlt_loads`/`_dlt_pipeline_state`/`_dlt_version` as first-class models; the model list is the menu handed to an agent over MCP, so junk entries burn tokens every session, invite an agent to aggregate or join them (agents lack the human instinct that a `_`-prefixed table is off-limits), and dump kilobytes of serialized state into context on one exploratory query. **Hidden, not skipped** — the model exists, stays queryable by name and remains a valid join target, so DEV-1741's no-silent-omissions principle holds and the deliberate use the issue names (`_dlt_loads` answering "when did this last load?") keeps working; all it loses is presence in `models_summary`, `models list`, the REST listing, semantic search, and the BI catalogs, each of which already gated on `model.hidden`. Rules live in a pure, DB-free `slayer/engine/internal_tables.py`; matching is case-insensitive (Liquibase upper-cases, EF Core writes `__EFMigrationsHistory`, Sequelize `SequelizeMeta`) and runs on the **live object name**, never the model name, since `_assign_model_names` collapses `__` runs and matching a derived string is matching something the database never had. PREFIX rules are admitted only where the namespace is reserved by contract in every warehouse the vendor loads into, so the rule needs no dialect: `_dlt_` and `_airbyte_` (which is also how `_airbyte_raw_*`'s arbitrary stream suffix is covered); everything else is an exact name. No `_fivetran_`/`_sdc_` prefix rule: both vendors' real surface is *columns* on real tables (`_fivetran_synced`, `_sdc_batched_at`), so a table-level prefix would match nothing and imply coverage we do not have — Fivetran's only destination-schema tables are the two audit ones, listed exactly. **No `sqlite_` prefix rule either**, for the same reason plus a worse one, and this took a second pass to see: SQLite genuinely reserves the namespace (`CREATE TABLE sqlite_foo` is a hard error), but the objects it reserves it FOR never reach the scan, because SQLAlchemy's SQLite inspector defaults to `sqlite_include_internal=False` and filters `sqlite_sequence`/`sqlite_stat1`…`sqlite_stat4` out of `get_table_names()`. So the rule was unreachable on the one dialect that justified it, and its only reachable effect was on a NON-SQLite datasource — where nothing reserves the prefix and `sqlite_backup` is an ordinary table — i.e. hiding real user data. Dialect-scoping it was considered and rejected: it would have left machinery gating a rule that then fires nowhere. Both halves of that argument are pinned by tests (the inspector really does drop a live `sqlite_sequence`; SQLite really does reject the `CREATE TABLE`), since deleting a rule on the strength of upstream behaviour is only safe while that behaviour holds. PostGIS and `pg_stat_statements` are deliberately excluded: bookkeeping, but not engine-reserved namespaces, and admitting them opens "which extensions count?" with no principled stopping point. One accepted risk remains: `schema_version` (legacy Flyway ≤4) is the most collision-prone exact entry — recoverable precisely because hiding is not omitting. **Creation-only**: `hidden`/`meta` are absent from `_additive_merge_existing`'s update dict, so an un-hide sticks forever and a pre-existing visible model is never retro-hidden; no migration heals old stores, because a "was this ever edited?" heuristic cannot tell a deliberate un-hide from a pristine model and would re-hide it on every run — the exact fight hidden-not-skipped exists to avoid. Flag is `--surface-internals`, not the issue's `--include-internals`, which would read as "add internals to the `--include` list" next to two flags that genuinely control inclusion; `--include _dlt_loads` therefore still hides it, keeping CLI and REST (which has no flags) identical. **Reporting is split by path, and this is the subtle part**: `IngestionScanReport.hidden_internals` is what the scan CONSTRUCTED (correct for `datasources create --ingest`, which never reads storage), while `IdempotentIngestResult.hidden_internals` is EFFECTIVE post-merge state, re-derived by re-reading each matched model and keeping only those actually hidden — looked up by `model_name`, not `table_name`, or every `__`-sanitized model silently vanishes from the report. Forwarding the scan's verdict lies in both directions: it reports a user-un-hidden model as hidden, and it prints nothing for a still-hidden model on the very run where `--surface-internals` was passed to see it. Candidates are re-derived from `scan.models` rather than reused from `scan.hidden_internals` for that second reason (the latter is empty under the flag by construction) and, being keyed on successfully-built models, exclude skipped objects for free. The `HiddenInternal` entry is appended only AFTER `_build_one_model` returns, so a name collision or a construction failure lands in `skipped` alone — never two contradictory verdicts on one table. Exit code is unchanged (hiding is the intended outcome; a skip is a capability failure with `--exclude` as its remedy) and REST never 422s on it, matching the `skipped` contract. `datasources create --ingest` switched from `ingest_datasource` to `ingest_datasource_report` so it stops being silent about both internals and skips; that forced `_print_ingest_drift_and_errors` to read every field through `getattr`, since an `IngestionScanReport` has no `to_delete` and no `errors` at all. **MCP was the last silent surface, and the one that mattered most**: `ingest_datasource_models` renders through its own `_render_ingest_result` rather than `_print_ingest_drift_and_errors`, and that renderer read only `additions`/`to_delete`/`errors` — so the agent-facing tool this feature exists for was the one place that never said what it hid (nor, since DEV-1741, what it skipped). Both sections were added there in the CLI's order. The subtle half is the EARLY RETURN, not the sections: a steady-state re-ingest produces no additions, no drift and no errors, so the "already in sync" branch would have swallowed both on every run after the first — the guard therefore has to test `skipped` and `hidden_internals` too, while still leaving the empty-schema probe reachable for a genuinely empty schema. The hint is `edit_model(name, hidden=false)` rather than `--surface-internals`, which is a CLI flag this tool does not accept; likewise the skip line names no `--exclude`. `surface_internals` is deliberately NOT exposed as an MCP tool argument: handing the agent a knob to un-hide the junk the feature exists to hide it from inverts the point, and `edit_model` already covers the deliberate single-model case. That un-hide hint is DATASOURCE-QUALIFIED on every surface (shared `_unhide_hint`), because internals are the one model class that collides across datasources *by construction* rather than by accident — `_dlt_loads` exists verbatim in every dlt-loaded database, so a bare `edit_model("_dlt_loads", hidden=false)` resolves by priority list and either raises `AmbiguousModelError` or silently un-hides the other pipeline's model, precisely in the multi-pipeline setup this feature targets. The parameter stays optional because neither result shape carries the datasource — it has to come from the caller, and all four in-tree ones pass it. Column-level internals (`_dlt_id`, `_dlt_load_id`, `_fivetran_synced`, `_sdc_*`) are deferred — `_fivetran_deleted` is a soft-delete flag an agent must SEE or it silently returns deleted rows, which needs its own design pass rather than a rider. Glob `--exclude`, internal schemas (`airbyte_internal`, `fivetran_metadata`), and Airbyte V2's `airbyte_internal._raw__stream_` naming are out of scope. +- 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. +- 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 26f63fef..ab1a07c5 100644 --- a/slayer/core/errors.py +++ b/slayer/core/errors.py @@ -245,3 +245,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..bae49204 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. @@ -345,6 +347,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 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 +1521,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 +1685,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 @@ -1878,6 +1908,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 9c5866e4..42828fc3 100644 --- a/slayer/engine/query_engine.py +++ b/slayer/engine/query_engine.py @@ -24,6 +24,7 @@ AmbiguousModelError, ForcedFilterError, IdentifierCollisionError, + UnresolvableDimensionJoinError, ) from slayer.core.policy import JoinFilterRuleset, SessionPolicy from slayer.core.format import NumberFormat, NumberFormatType, format_number @@ -2938,6 +2939,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. @@ -2960,6 +2962,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: @@ -3050,6 +3059,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 @@ -3423,6 +3433,167 @@ 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 + + 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 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 = await self._route_time_dimension_list( + query.time_dimensions, model, named_queries, **kw + ) + if new_tds is not None: + updates["time_dimensions"] = new_tds + if rewrite: + 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: + 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, *, @@ -3832,6 +4003,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/slayer/sql/generator.py b/slayer/sql/generator.py index 8489e099..38bf0c12 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -1732,13 +1732,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": @@ -1757,7 +1777,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_dev1780_missing_join_path.py b/tests/test_dev1780_missing_join_path.py new file mode 100644 index 00000000..b24a8ff5 --- /dev/null +++ b/tests/test_dev1780_missing_join_path.py @@ -0,0 +1,658 @@ +"""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")], + ) + model = self._ghost_model() + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await enrich_query( + query=query, model=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, + )], + ) + model = self._ghost_model() + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await enrich_query( + query=query, model=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")], + ) + model = self._ghost_model() + with pytest.raises(UnresolvableDimensionJoinError) as ei: + await enrich_query( + query=query, model=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 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.""" + 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 diff --git a/tests/test_formula_referencing_measure_dev1779.py b/tests/test_formula_referencing_measure_dev1779.py new file mode 100644 index 00000000..3b35ff66 --- /dev/null +++ b/tests/test_formula_referencing_measure_dev1779.py @@ -0,0 +1,404 @@ +"""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", + ) + ], + ) + generator = SQLGenerator(dialect="postgres") + with pytest.raises(ValueError) as exc: + 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. + 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", + ) + ], + ) + generator = SQLGenerator(dialect="postgres") + with pytest.raises(ValueError) as exc: + generator.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", + ) + ], + ) + generator = SQLGenerator(dialect="postgres") + with pytest.raises(ValueError) as exc: + generator.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