From b1c964063e8849c0906af4507f9581f3086a5e36 Mon Sep 17 00:00:00 2001 From: whimo Date: Mon, 20 Jul 2026 17:41:00 +0400 Subject: [PATCH 1/5] Fix duplicate CTE name when a query has multiple arithmetic-wrapped transforms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A query with two or more `time_shift()` calls embedded in arithmetic formulas failed at SQL generation with `Parser Error: Duplicate CTE name "shifted__t0"`. `_parse_mixed_arithmetic` mints the placeholder names for transforms hoisted out of an arithmetic formula from a counter that restarts on every formula parse, so every such measure claimed the name `_t0`. That name becomes both the self-join CTE name (`shifted_{name}`) and the projection alias, so two measures collided on both. Qualify each placeholder with the owning measure's `field_name` in the `MixedArithmeticField` branch of `_flatten_spec`, mirroring the `_ts_{field_name}` scheme the change/change_pct desugar already uses. `field_name` is unique per query (duplicate measure names are rejected), so the hoisted names are too. The duplicate CTE name was the visible half of the bug: both expressions also resolved to the same alias, so the second measure read the first one's value. That made the same collision silently wrong — rather than a parser error — for the CTE-less window transforms (`cumsum`, `lag`, `lead`) wrapped in arithmetic, which generated valid SQL with duplicate aliases. Both are fixed here. Fixes DEV-1692. Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 1 + slayer/engine/enrichment.py | 21 +++++- tests/integration/test_integration.py | 46 +++++++++++++ tests/test_sql_generator.py | 94 +++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f246be43..3ae604df 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -72,6 +72,7 @@ poetry run ruff check slayer/ tests/ - **Result column naming**: `revenue:sum` → `orders.revenue_sum` (colon becomes underscore). `*:count` → `orders._count` — the `*` is dropped but the underscore is kept as a leading marker so the alias never collides with a user-defined column literally named `count`. When converting queries to models (`create_model_from_query`), the same colon-to-underscore mapping applies. An explicit `name` on the measure spec overrides the canonical form for **both** simple aggregations and arithmetic/transform formulas — `{"formula": "amount:sum", "name": "rev"}` surfaces as `orders.rev`. This matters most for inner stages of multi-stage `source_queries`, where downstream stages reference inner-stage outputs by the chosen name. **DEV-1443**: when a query measure is renamed via `name`, filters and ORDER BY entries in the same node may reference EITHER the raw colon form (`amount:sum > 100`) OR the user alias (`rev > 100`) — both resolve to the user alias, and a colon-form filter classifies as HAVING. Renaming never changes the legal filter/order form. Two enrichment-time validations on renames: (1) a query measure `name` colliding with a source column on the source model is rejected (the alias-form filter would otherwise silently bind to the source column instead of the aggregate); (2) a rename whose canonical alias (e.g. `revenue_sum`) literally shadows a source column on the same model is also rejected — the colon-form filter would otherwise be ambiguous between the renamed aggregate and the source column. **DEV-1448**: the user-supplied `name` on a *cross-model* aggregated measure (`{"formula": "customers.revenue:sum", "name": "cust_rev"}`) is now honored for the projection alias and the downstream-stage virtual model column. Only the canonical **leaf** of the dotted path is swapped to the user name; the hop path is preserved — same dot-syntax shape as every other multi-hop caller-facing key. So `{"formula": "customers.revenue:sum", "name": "cust_rev"}` surfaces as `orders.customers.cust_rev`, and `{"formula": "customers.regions.population:sum", "name": "region_pop"}` surfaces as `orders.customers.regions.region_pop`. Two caller-facing surfaces: (a) the top-level result-column key uses the hop-preserved form (`orders.customers.cust_rev`); (b) the downstream-stage virtual model column built by `_query_as_model` uses the BARE user name (`cust_rev`) — a special-case in the cross-model loop short-circuits the `__`-flattening of `_alias_to_short` whenever `cm.name` is a bare identifier, so a stage-2 reference like `cust_rev:max` resolves directly. The inner `CrossModelMeasure.measure` (used to build the CTE aggregate expression) intentionally retains the canonical form — only the outer handle is renamed. The canonical-collision guard from DEV-1443 was lifted into a pre-pass so it fires for both local AND cross-model renames symmetrically. Covers `customers.*:count` and `customers.col:count_distinct` cross-model variants. Same-stage **filter** referencing a renamed cross-model measure (`filters=["cust_rev > 100"]` or the colon form `"customers.revenue:sum > 100"`) is NOT auto-resolved — both raise at strict resolution; the SQL generator has no path to route the bare user alias to the cross-model CTE's output column for filter classification, so admitting the filter would emit broken SQL (`WHERE orders.cust_rev > 100` against a column that doesn't exist on the base table). Filter remap stays DEV-1445 territory. **ORDER BY** referencing the bare user alias (`order=[{"column": "cust_rev"}]`) DOES resolve via `SQLGenerator._resolve_order_column`'s `alias_lookup[cm.name] = cm.alias` mapping and emits `ORDER BY "orders.customers.cust_rev"` against the cross-model CTE's output column. Workaround until DEV-1445 lands: restructure as a multi-stage `source_queries` so the cross-model measure becomes a local measure in the downstream stage. See companion tickets DEV-1445 (cross-model filter remap) and DEV-1446 (transform-wrapped inner-ref dedup). - **Response attributes**: `SlayerResponse.attributes` is a `ResponseAttributes` with `.dimensions` and `.measures` dicts, each mapping column alias → `FieldMetadata(label, format)`. Split by type so consumers can distinguish dimension metadata from measure metadata. - Available formula transforms: cumsum, time_shift, change, change_pct, rank, percent_rank, dense_rank, ntile, first (FIRST_VALUE window ASC), last (FIRST_VALUE window DESC), lag, lead, consecutive_periods. time_shift uses a self-join CTE where the shifted sub-query has the time column expression offset by INTERVAL (calendar-based, gap-safe). change and change_pct are desugared at enrichment time into a hidden time_shift + arithmetic expression. lag/lead use LAG/LEAD window functions directly (more efficient but produce NULLs at edges). Non-transform SQL function calls (`nullif`, `coalesce`, `ln`, `sqrt`, etc.) may also wrap aggregated refs inside arithmetic expressions, e.g. `"*:count / nullif(revenue:max, 0)"` — the call passes through to emitted SQL while the inner refs resolve to their measure aliases +- **Hidden-transform naming** (DEV-1692): a transform wrapped in arithmetic (`revenue:sum - time_shift(revenue:sum, -1, 'month')`) is hoisted out of the formula under a hidden name. `slayer/core/formula.py::_parse_mixed_arithmetic` mints per-formula placeholders `_t0`, `_t1`, … from a counter that **restarts on every formula parse**, so the `MixedArithmeticField` branch of `_flatten_spec` (`slayer/engine/enrichment.py`) qualifies each one with the owning measure's `field_name` — `_t0_growth`, `_t1_growth_pct` — before handing it to `_add_transform`. `field_name` is unique per query (duplicate measure names are rejected), so the hoisted names are too. This matters because the name becomes both the self-join CTE name (`shifted_{name}` / `sjoin_{name}` in `slayer/sql/generator.py`) and the projection alias: unqualified, two measures each wrapping a transform both became `_t0`, which surfaced as a duplicate-CTE parser error for `time_shift`/`change` and, for the CTE-less window transforms (`cumsum`, `lag`, …), as *silently wrong values* (the second measure read the first's). The `change`/`change_pct` desugar uses the same scheme (`_ts_{field_name}`) and composes — `change(x) * 2` yields `_ts__t0_{field_name}`. Filter-extracted transforms use a separate `_ft{n}` namespace off a query-wide shared counter, so they never collide either. Extending: any new hoisting path must derive its name from `field_name`, never from a formula-local counter alone - **Rank-family transforms** (DEV-1353): `rank`, `percent_rank`, `dense_rank`, and `ntile` are timeless window-function transforms emitted as `RANK() / PERCENT_RANK() / DENSE_RANK() / NTILE(n) OVER (... ORDER BY DESC)`. They default to **no `PARTITION BY`** (rank across the entire result set, unlike cumsum/lag/lead which auto-partition by query dimensions), and accept an optional `partition_by=col` or `partition_by=[col1, col2]` kwarg to opt into per-partition ranking; the columns referenced must be query dimensions or time dimensions. `ntile` additionally requires `n=`. Standard SQL across SQLite (≥3.25), Postgres, DuckDB, MySQL, and ClickHouse — no UDFs needed. - Filters can reference computed field names or contain inline transform expressions (e.g., `"change(revenue:sum) > 0"`, `"last(change(revenue:sum)) < 0"`). These are auto-extracted as hidden fields and applied as post-filters on the outer query - **Window functions in filters**: filter strings and `ModelMeasure.formula` cannot contain raw `OVER (...)` SQL — SLayer's formula parser is Python-AST-based and rejects with an actionable error pointing at the `rank()` / `first()` / `last()` / `lag()` / `lead()` transforms. Filtering on a `Column` whose `sql` contains a window function is also rejected (DEV-1369; the prior auto-promotion escape hatch from DEV-1336 is removed). For top-N use the inline `rank() <= N` transform (or `dense_rank` / `percent_rank` / `ntile(n=)`); for non-standard window expressions, factor them into an earlier stage of a multi-stage `source_queries` model. diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index 025c1c35..e575f598 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -882,13 +882,30 @@ async def _flatten_spec(spec, field_name: str) -> str: elif isinstance(spec, MixedArithmeticField): for mname in spec.measure_names: await _ensure_measure_from_spec(mname, spec.agg_refs) + # DEV-1692: the ``_t{n}`` placeholder counter restarts on every + # formula parse, so two measures that each wrap a transform in + # arithmetic would both flatten under the name ``_t0`` — colliding + # on the self-join CTE names (``shifted__t0``) and, worse, on the + # expression alias, silently making the second measure read the + # first one's value. Qualify with the owning measure's field_name + # (unique per query), mirroring the ``_ts_{field_name}`` naming the + # change/change_pct desugar already uses. + placeholder_aliases: list[tuple[str, str]] = [] for placeholder, sub_transform in spec.sub_transforms: - await _flatten_spec(sub_transform, placeholder) + sub_alias = await _flatten_spec( + sub_transform, f"{placeholder}_{field_name}" + ) + placeholder_aliases.append((placeholder, sub_alias)) + for placeholder, sub_alias in placeholder_aliases: + known_aliases[placeholder] = sub_alias + resolved_sql = _resolve_sql(spec.sql) + for placeholder, _ in placeholder_aliases: + known_aliases.pop(placeholder, None) alias = f"{model_name_str}.{field_name}" enriched_expressions.append( EnrichedExpression( name=field_name, - sql=_resolve_sql(spec.sql), + sql=resolved_sql, alias=alias, ) ) diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index 0b35d4d9..e35b8038 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -531,6 +531,52 @@ async def test_time_shift_calendar_based(integration_env): assert response.data[2]["orders.prev_month"] == pytest.approx(125.0) +async def test_multiple_time_shifts_in_one_query(integration_env): + """DEV-1692: two arithmetic-wrapped time_shifts in one query. + + Used to fail outright with a duplicate-CTE error; the offsets are kept + distinct here (-1 vs -2) so a regression that re-collapses them onto one + shared CTE shows up as wrong values rather than just a parser error. + """ + engine = integration_env + + query = SlayerQuery( + source_model="orders", + time_dimensions=[TimeDimension( + dimension=ColumnRef(name="created_at"), + granularity=TimeGranularity.MONTH, + )], + measures=[ + ModelMeasure(formula="total_amount:sum"), + ModelMeasure( + formula="total_amount:sum - time_shift(total_amount:sum, -1, 'month')", + name="growth_1m", + ), + ModelMeasure( + formula="total_amount:sum - time_shift(total_amount:sum, -2, 'month')", + name="growth_2m", + ), + ], + order=[OrderItem(column=ColumnRef(name="created_at"), direction="asc")], + ) + response = await engine.execute(query) + + # 3 months: Jan(300), Feb(125), Mar(325) + assert response.row_count == 3 + + # No month two back from Jan/Feb, and none one back from Jan → NULL + assert response.data[0]["orders.growth_1m"] is None + assert response.data[0]["orders.growth_2m"] is None + assert response.data[1]["orders.growth_2m"] is None + + # Feb vs Jan + assert response.data[1]["orders.growth_1m"] == pytest.approx(125.0 - 300.0) + # Mar vs Feb (-1) and Mar vs Jan (-2) must differ — same-CTE collapse + # would make both read the -1 shift. + assert response.data[2]["orders.growth_1m"] == pytest.approx(325.0 - 125.0) + assert response.data[2]["orders.growth_2m"] == pytest.approx(325.0 - 300.0) + + async def test_time_shift_with_date_range(integration_env): """time_shift with date_range should fetch shifted data from outside the filtered range.""" engine = integration_env diff --git a/tests/test_sql_generator.py b/tests/test_sql_generator.py index dfd3d247..e8631101 100644 --- a/tests/test_sql_generator.py +++ b/tests/test_sql_generator.py @@ -1326,6 +1326,100 @@ async def test_quarter_date_shift(self, generator: SQLGenerator, orders_model: S assert "MONTH" in sql assert "shifted_" in sql + async def test_multiple_time_shifts_in_arithmetic_unique_ctes( + self, generator: SQLGenerator, orders_model: SlayerModel + ) -> None: + """DEV-1692: two arithmetic-wrapped time_shifts must not share a CTE name. + + The `_t{n}` placeholder counter restarts per formula, so both measures + used to flatten as `_t0` — emitting `shifted__t0` twice (a duplicate-CTE + parser error) and silently aliasing the second shift onto the first. + """ + orders_model.default_time_dimension = "created_at" + query = SlayerQuery( + source_model="orders", + time_dimensions=[ + TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH) + ], + measures=[ + ModelMeasure(formula="revenue:sum - time_shift(revenue:sum, -1, 'month')", name="growth"), + ModelMeasure(formula="revenue:sum - time_shift(revenue:sum, -2, 'month')", name="growth_2m"), + ], + ) + sql = await _generate(generator, query, orders_model) + + ctes = _re.findall(r'(?:WITH|,)\s*"?(\w+)"?\s+AS\s*\(', sql) + assert len(ctes) == len(set(ctes)), f"duplicate CTE names: {ctes}" + assert len([c for c in ctes if c.startswith("shifted_")]) == 2 + + async def test_multiple_time_shifts_resolve_to_distinct_aliases( + self, orders_model: SlayerModel + ) -> None: + """DEV-1692: each shift keeps its own offset — no silent alias sharing. + + Guards the corruption the duplicate name masked: both expressions + previously resolved to `orders._t0`, so `growth_2m` would have read + `growth`'s -1 month shift. + """ + orders_model.default_time_dimension = "created_at" + query = SlayerQuery( + source_model="orders", + time_dimensions=[ + TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH) + ], + measures=[ + ModelMeasure(formula="revenue:sum - time_shift(revenue:sum, -1, 'month')", name="growth"), + ModelMeasure(formula="revenue:sum - time_shift(revenue:sum, -2, 'month')", name="growth_2m"), + ], + ) + enriched = await enrich_query( + query=query, + model=orders_model, + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + + shifts = [t for t in enriched.transforms if t.transform == "time_shift"] + assert len({t.alias for t in shifts}) == len(shifts) + assert {t.offset for t in shifts} == {-1, -2} + + by_name = {e.name: e.sql for e in enriched.expressions} + assert by_name["growth"] != by_name["growth_2m"] + + async def test_dev_1692_repro_shape( + self, generator: SQLGenerator, orders_model: SlayerModel + ) -> None: + """DEV-1692: the reported four-measure period-over-period shape. + + `growth_pct` alone carries two time_shift calls, so uniquification has + to hold within a single formula as well as across measures. + """ + orders_model.default_time_dimension = "created_at" + query = SlayerQuery( + source_model="orders", + dimensions=[ColumnRef(name="status")], + time_dimensions=[ + TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH) + ], + measures=[ + ModelMeasure(formula="revenue:sum", name="total_revenue"), + ModelMeasure(formula="time_shift(revenue:sum, -1, 'month')", name="prev_revenue"), + ModelMeasure(formula="revenue:sum - time_shift(revenue:sum, -1, 'month')", name="growth"), + ModelMeasure( + formula=( + "(revenue:sum - time_shift(revenue:sum, -1, 'month')) " + "/ time_shift(revenue:sum, -1, 'month')" + ), + name="growth_pct", + ), + ], + ) + sql = await _generate(generator, query, orders_model) + + ctes = _re.findall(r'(?:WITH|,)\s*"?(\w+)"?\s+AS\s*\(', sql) + assert len(ctes) == len(set(ctes)), f"duplicate CTE names: {ctes}" + async def test_nested_self_join_raises(self, generator: SQLGenerator, orders_model: SlayerModel) -> None: """Nesting self-join transforms (e.g., change(time_shift(x))) should raise.""" orders_model.default_time_dimension = "created_at" From 81c01abbfb4d82f94399f91966179c2a6448fd13 Mon Sep 17 00:00:00 2001 From: whimo Date: Mon, 20 Jul 2026 18:56:26 +0400 Subject: [PATCH 2/5] Reword change/change_pct/time_shift transform docs (DEV-1693) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "diff from previous row" gloss on change/change_pct read as naive row-diffing, steering users toward time_shift for grouped period-over-period queries — exactly the case where change_pct is the right tool. Reword the MCP query docstring, formulas.md, the help.transforms memory, terminology.md, and the query skill to state the actual behavior: calendar-aware self-join matched on all non-time dimensions, partition-safe, resets per group. Add intent recipes (month-over-month growth -> change_pct). Also correct two stale claims that granularity-less time_shift(x, -1) compiles to LAG — verified via dry-run that it emits the calendar self-join at the query's time grain. --- .claude/skills/slayer-query.md | 2 ++ docs/concepts/formulas.md | 14 ++++++++++--- docs/concepts/terminology.md | 2 +- docs/examples/04_time/time_nb.ipynb | 2 +- slayer/mcp/server.py | 14 ++++++++++--- slayer/memories/help_content/04_transforms.md | 21 ++++++++++++++++--- tests/integration/test_integration.py | 2 +- 7 files changed, 45 insertions(+), 12 deletions(-) diff --git a/.claude/skills/slayer-query.md b/.claude/skills/slayer-query.md index 68e5a4c5..e1fb5171 100644 --- a/.claude/skills/slayer-query.md +++ b/.claude/skills/slayer-query.md @@ -48,6 +48,8 @@ Each entry in `measures` is either a bare formula string or a `{"formula": ..., Built-in aggregations: `sum`, `avg`, `min`, `max`, `count`, `count_distinct`, `count_distinct_approx`, `first`, `last`, `weighted_avg`, `median`, `percentile`, `stddev_samp`, `stddev_pop`, `var_samp`, `var_pop`, `corr`, `covar_samp`, `covar_pop`. `count_distinct_approx` is dialect-aware (native approximate-distinct where available, exact `COUNT(DISTINCT)` fallback otherwise). Two-column `corr`/`covar_samp`/`covar_pop` take the second column as a named param: `price:corr(other=quantity)`. `sum` and `avg` accept an optional trailing-window: `revenue:sum(window='30d')`. +For month-over-month / period-over-period growth use `change_pct(x)` (absolute delta: `change(x)`) — both are calendar-aware and partition-safe (the underlying self-join matches on all non-time dimensions, so per-group series reset cleanly). Reach for `time_shift` only when you need the shifted value itself as a term in custom arithmetic or at a different grain (`time_shift(revenue:sum, -1, 'year')` for year-over-year). + `*:count` is always available — no column definition needed. `col:count` counts non-nulls. Saved named formulas (`SlayerModel.measures`) can be referenced by bare name in any formula context: `{"formula": "aov"}`. diff --git a/docs/concepts/formulas.md b/docs/concepts/formulas.md index 40e7c626..3ca9bd3b 100644 --- a/docs/concepts/formulas.md +++ b/docs/concepts/formulas.md @@ -131,12 +131,12 @@ Functions apply window operations to measures: | Function | Description | SQL Generated | |----------|-------------|---------------| | `cumsum(x)` | Running total over time | `SUM(x) OVER (PARTITION BY dims ORDER BY time)` | -| `time_shift(x, n)` | Value N periods back/ahead | Self-join CTE with INTERVAL offset | +| `time_shift(x, n)` | Value N time buckets back/ahead (calendar-aware) | Self-join CTE with INTERVAL offset | | `time_shift(x, offset, gran)` | Value from a different time bucket | Self-join CTE with INTERVAL offset | | `lag(x, n)` | Value N rows back (window function) | `LAG(x, n) OVER (PARTITION BY dims ORDER BY time)` | | `lead(x, n)` | Value N rows ahead (window function) | `LEAD(x, n) OVER (PARTITION BY dims ORDER BY time)` | -| `change(x)` | Difference from previous period | Desugars to `x - time_shift(x, -1)` | -| `change_pct(x)` | Percentage change from previous | Desugars to `(x - ts) / ts` where `ts = time_shift(x, -1)` | +| `change(x)` | Period-over-period difference (partition-safe, resets per group) | Desugars to `x - time_shift(x, -1)` | +| `change_pct(x)` | Period-over-period % change, e.g. month-over-month growth (partition-safe, resets per group) | Desugars to `(x - ts) / ts` where `ts = time_shift(x, -1)` | | `consecutive_periods(predicate)` | Current trailing run length where predicate is true | Staged window CTEs with reset groups | | `rank(x[, partition_by=...])` | Ranking by value (descending) | `RANK() OVER ([PARTITION BY ...] ORDER BY x DESC)` | | `percent_rank(x[, partition_by=...])` | Relative rank in [0, 1] (descending) | `PERCENT_RANK() OVER ([PARTITION BY ...] ORDER BY x DESC)` | @@ -155,6 +155,14 @@ total per status, not one running total across the whole result set. `time_shift` uses a **self-join CTE** with an INTERVAL-shifted time column. `change` and `change_pct` are desugared into a hidden `time_shift` + arithmetic expression at query enrichment time. The shifted sub-query applies the time offset everywhere (WHERE, GROUP BY, SELECT), so it can reach outside the current result set — no edge NULLs when the database has the data, and correct handling of gaps in time series. +The self-join matches on **all non-time dimensions as well as the shifted time column** (e.g. `ON base.month = shifted.month AND base.store = shifted.store`), so these transforms are partition-safe: each group's series is compared only against itself, and per-group series reset cleanly. One store's first month is never diffed against another store's last month. + +**Intent recipes:** + +- Month-over-month / period-over-period growth → `change_pct(revenue:sum)` with a `time_dimensions` entry at the desired granularity. Prefer this over hand-building the ratio from `time_shift`. +- Absolute period-over-period delta → `change(revenue:sum)`. +- Comparing against a *different* grain than the query's (e.g. year-over-year on a monthly series), or using the shifted value as a term in custom arithmetic → `time_shift(revenue:sum, -1, 'year')`. + `lag(x, n)` and `lead(x, n)` use SQL `LAG`/`LEAD` window functions directly. They are more efficient but have two trade-offs: - **Edge NULLs**: the first/last N rows always return NULL since window functions can only see rows within the current result set. diff --git a/docs/concepts/terminology.md b/docs/concepts/terminology.md index 0e01dd0d..8950b173 100644 --- a/docs/concepts/terminology.md +++ b/docs/concepts/terminology.md @@ -52,7 +52,7 @@ Key terms used throughout SLayer documentation and code. **Self-join vs window-function transforms:** -- `time_shift`, `change`, and `change_pct` use self-join CTEs — they can reach outside the current result set (no edge NULLs) and handle gaps in data correctly. `time_shift(revenue, -1, 'year')` (with granularity) joins on calendar date arithmetic for comparisons like year-over-year. +- `time_shift`, `change`, and `change_pct` use self-join CTEs — they can reach outside the current result set (no edge NULLs) and handle gaps in data correctly. The join matches on all non-time dimensions as well as the shifted time column, so grouped comparisons are partition-safe and reset per group. `time_shift(revenue, -1, 'year')` (with granularity) joins on calendar date arithmetic for comparisons like year-over-year. - `lag(revenue, 1)` / `lead(revenue, 1)` use SQL `LAG`/`LEAD` window functions directly — more efficient, but produce NULLs at the edges and are sensitive to gaps in data. **Nesting** — Formulas can be nested: `change(cumsum(revenue))` applies `change` to the result of `cumsum`. Each level of nesting generates an additional CTE layer in the SQL. diff --git a/docs/examples/04_time/time_nb.ipynb b/docs/examples/04_time/time_nb.ipynb index cf3e53ba..1f350c04 100644 --- a/docs/examples/04_time/time_nb.ipynb +++ b/docs/examples/04_time/time_nb.ipynb @@ -261,7 +261,7 @@ "metadata": { "jp-MarkdownHeadingCollapsed": true }, - "source": "### time_shift vs lag\n\nWithout a granularity argument, `time_shift(x, -1)` is equivalent to `lag(x, 1)` — it uses SQL's `LAG()` window function to look at the previous row. This is more efficient but blindly walks rows: if the result has a gap (e.g., a missing month), the previous row is the wrong period.\n\nWith a granularity argument (like `'year'` above), `time_shift` uses a calendar-based self-join: gap-safe (it matches on the actual calendar date, so a missing intermediate month produces NULL rather than the wrong value), and the shift can be any amount (not just a multiple of the query granularity). Both forms still return NULL at the start of the window when there's no prior period in the data to match." + "source": "### time_shift vs lag\n\n`time_shift` always uses a calendar-based self-join, with or without a granularity argument. Without one, `time_shift(x, -1)` shifts by one bucket of the query's own time granularity; with one (like `'year'` above), the shift can be any calendar amount — not just a multiple of the query granularity. Either way it's gap-safe: the previous value is matched on the actual calendar date, so a missing intermediate month produces NULL rather than the wrong value. The join also matches on all non-time dimensions, so grouped series (e.g. per-store) reset cleanly per group.\n\n`lag(x, 1)` / `lead(x, 1)` use SQL's `LAG()`/`LEAD()` window functions instead — more efficient, but they blindly walk rows: if the result has a gap (e.g., a missing month), the previous row is the wrong period. Both approaches return NULL at the start of the window when there's no prior period in the data to match." }, { "cell_type": "markdown", diff --git a/slayer/mcp/server.py b/slayer/mcp/server.py index 036d9b5c..49af018f 100644 --- a/slayer/mcp/server.py +++ b/slayer/mcp/server.py @@ -346,11 +346,19 @@ async def query( # NOSONAR S107 — FastMCP introspects this signature to expos ``{"name": "ad_hoc", "sql_table": "things", "data_source": "test", "columns": [...]}``. measures: Aggregated values to return. Each is a formula: {"formula": "*:count"}, {"formula": "revenue:sum / *:count", "name": "aov"} (arithmetic), - {"formula": "cumsum(revenue:sum)"} (cumulative sum), {"formula": "change(revenue:sum)"} (diff from previous row), - {"formula": "change_pct(revenue:sum)"} (% change), {"formula": "time_shift(revenue:sum, -1)"} (previous period via self-join), - {"formula": "time_shift(revenue:sum, -1, 'year')"} (year-over-year), {"formula": "lag(revenue:sum, 1)"} (previous row via window function), + {"formula": "cumsum(revenue:sum)"} (cumulative sum), + {"formula": "change(revenue:sum)"} (period-over-period difference), + {"formula": "change_pct(revenue:sum)"} (period-over-period % change, e.g. month-over-month growth), + {"formula": "time_shift(revenue:sum, -1)"} (the shifted value itself, one time bucket back), + {"formula": "time_shift(revenue:sum, -1, 'year')"} (value from one year earlier, for custom arithmetic), + {"formula": "lag(revenue:sum, 1)"} (previous row via window function; shifts by row position, NULL at edges), {"formula": "lead(revenue:sum, 1)"} (next row via window function), {"formula": "last(revenue:sum)"} (most recent), {"formula": "rank(revenue:sum)"} (ranking). A bare name like {"formula": "aov"} resolves to a saved ModelMeasure on the model. + change / change_pct / time_shift are calendar-aware and partition-safe: they compare each row against + the same non-time dimension values one time grain earlier (joined on all non-time dimensions), so + per-group series reset cleanly — safe for grouped queries like month-over-month revenue by store. + For period-over-period growth, prefer change_pct (or change for the absolute delta); use time_shift + only when you need the shifted value itself as a term in your own arithmetic. dimensions: List of dimension names to group by, e.g. ["status", "region"]. filters: Filter conditions as formula strings. Examples: "status == 'completed'", "amount > 100", "status in ('a', 'b')", "status is None", diff --git a/slayer/memories/help_content/04_transforms.md b/slayer/memories/help_content/04_transforms.md index d29227f9..dbad001c 100644 --- a/slayer/memories/help_content/04_transforms.md +++ b/slayer/memories/help_content/04_transforms.md @@ -9,10 +9,10 @@ becomes an extra CTE in the generated SQL. | Transform | Purpose | SQL strategy | |-----------|---------|--------------| | `cumsum(x)` | Running total over time | Window: `SUM(x) OVER (PARTITION BY dims ORDER BY time)` | -| `time_shift(x, n)` | Value N periods back/ahead | Self-join CTE with INTERVAL offset | +| `time_shift(x, n)` | Value N time buckets back/ahead (calendar-aware) | Self-join CTE with INTERVAL offset | | `time_shift(x, n, 'year')` | Value at a different granularity offset (e.g. YoY) | Self-join CTE with INTERVAL offset | -| `change(x)` | `x − previous(x)` | Desugars to `x − time_shift(x, -1)` | -| `change_pct(x)` | `(x − previous) / previous` | Desugars to `(x − ts) / ts` where `ts = time_shift(x, -1)` | +| `change(x)` | Period-over-period difference (partition-safe, resets per group) | Desugars to `x − time_shift(x, -1)` | +| `change_pct(x)` | Period-over-period % change, e.g. month-over-month growth (partition-safe) | Desugars to `(x − ts) / ts` where `ts = time_shift(x, -1)` | | `lag(x, n)` / `lead(x, n)` | N rows back / ahead | `LAG` / `LEAD` window fn, partitioned by dimensions | | `consecutive_periods(predicate)` | Current trailing run length where predicate is true | Staged window CTEs with reset groups | | `rank(x[, partition_by=...])` | Rank by x, descending; ties skip ranks | `RANK() OVER ([PARTITION BY ...] ORDER BY x DESC)` | @@ -32,6 +32,10 @@ the previous/next value. Consequences: - No NULLs at the first / last rows when the database actually has the data. - Handles **gaps** in the time series correctly — shifts by calendar, not by row. +- **Partition-safe**: the join matches on all non-time dimensions as well as the + shifted time column (`ON base.month = shifted.month AND base.store = + shifted.store`), so each group's series is compared only against itself and + resets cleanly per group. - Slightly heavier SQL. `lag` and `lead` use SQL `LAG` / `LEAD`: @@ -43,6 +47,17 @@ the previous/next value. Consequences: Use `time_shift`, `change`, `change_pct` unless you have a specific reason to prefer `lag` / `lead`. +**Intent recipes:** + +- Month-over-month / period-over-period growth → `change_pct(revenue:sum)` + with a `time_dimensions` entry at the desired granularity. Prefer this over + hand-building the ratio from `time_shift` — same partition-safe self-join, + cleaner SQL. +- Absolute period-over-period delta → `change(revenue:sum)`. +- Comparing against a *different* grain than the query's (e.g. year-over-year + on a monthly series), or using the shifted value as a term in custom + arithmetic → `time_shift(revenue:sum, -1, 'year')`. + ## Time dimension requirement All time-ordered transforms (`cumsum`, `time_shift`, `change`, `change_pct`, diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index e35b8038..b13a3a20 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -470,7 +470,7 @@ async def test_arithmetic_expression(integration_env): async def test_time_shift_row_based(integration_env): - """time_shift(x, -1) without granularity → LAG (previous row).""" + """time_shift(x, -1) without granularity → calendar self-join at the query's time grain.""" engine = integration_env query = SlayerQuery( From 47faa1eccf01cb1a50fa6956a04c34a5dab5cf16 Mon Sep 17 00:00:00 2001 From: whimo Date: Mon, 20 Jul 2026 21:10:54 +0400 Subject: [PATCH 3/5] Address review: change_pct NULL guard, time_shift offset wording, gap-aware test - Document change_pct's CASE WHEN ts != 0 guard (NULL on zero/missing prior period) in formulas.md and the help.transforms memory. - MCP query docstring: change/change_pct compare against the prior time bucket at the query's granularity; time_shift at its explicit offset/granularity. - Rework the default-granularity time_shift test to use a gap month (completed orders only: Jan and Mar, no Feb) so calendar self-join semantics are distinguished from row-based LAG/LEAD in both directions. --- docs/concepts/formulas.md | 2 +- slayer/mcp/server.py | 8 ++-- slayer/memories/help_content/04_transforms.md | 2 +- tests/integration/test_integration.py | 37 +++++++++++++------ 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/docs/concepts/formulas.md b/docs/concepts/formulas.md index 3ca9bd3b..af4e5113 100644 --- a/docs/concepts/formulas.md +++ b/docs/concepts/formulas.md @@ -136,7 +136,7 @@ Functions apply window operations to measures: | `lag(x, n)` | Value N rows back (window function) | `LAG(x, n) OVER (PARTITION BY dims ORDER BY time)` | | `lead(x, n)` | Value N rows ahead (window function) | `LEAD(x, n) OVER (PARTITION BY dims ORDER BY time)` | | `change(x)` | Period-over-period difference (partition-safe, resets per group) | Desugars to `x - time_shift(x, -1)` | -| `change_pct(x)` | Period-over-period % change, e.g. month-over-month growth (partition-safe, resets per group) | Desugars to `(x - ts) / ts` where `ts = time_shift(x, -1)` | +| `change_pct(x)` | Period-over-period % change, e.g. month-over-month growth (partition-safe, resets per group; NULL when the prior period's value is 0 or missing) | Desugars to `CASE WHEN ts != 0 THEN (x - ts) / ts END` where `ts = time_shift(x, -1)` | | `consecutive_periods(predicate)` | Current trailing run length where predicate is true | Staged window CTEs with reset groups | | `rank(x[, partition_by=...])` | Ranking by value (descending) | `RANK() OVER ([PARTITION BY ...] ORDER BY x DESC)` | | `percent_rank(x[, partition_by=...])` | Relative rank in [0, 1] (descending) | `PERCENT_RANK() OVER ([PARTITION BY ...] ORDER BY x DESC)` | diff --git a/slayer/mcp/server.py b/slayer/mcp/server.py index 49af018f..63d556ae 100644 --- a/slayer/mcp/server.py +++ b/slayer/mcp/server.py @@ -354,9 +354,11 @@ async def query( # NOSONAR S107 — FastMCP introspects this signature to expos {"formula": "lag(revenue:sum, 1)"} (previous row via window function; shifts by row position, NULL at edges), {"formula": "lead(revenue:sum, 1)"} (next row via window function), {"formula": "last(revenue:sum)"} (most recent), {"formula": "rank(revenue:sum)"} (ranking). A bare name like {"formula": "aov"} resolves to a saved ModelMeasure on the model. - change / change_pct / time_shift are calendar-aware and partition-safe: they compare each row against - the same non-time dimension values one time grain earlier (joined on all non-time dimensions), so - per-group series reset cleanly — safe for grouped queries like month-over-month revenue by store. + change / change_pct / time_shift are calendar-aware and partition-safe: change and change_pct compare + each row against the prior time bucket (one step back at the query's own granularity), while time_shift + compares at its explicitly requested offset and granularity. All three join on the same non-time + dimension values, so per-group series reset cleanly — safe for grouped queries like month-over-month + revenue by store. For period-over-period growth, prefer change_pct (or change for the absolute delta); use time_shift only when you need the shifted value itself as a term in your own arithmetic. dimensions: List of dimension names to group by, e.g. ["status", "region"]. diff --git a/slayer/memories/help_content/04_transforms.md b/slayer/memories/help_content/04_transforms.md index dbad001c..e533f89a 100644 --- a/slayer/memories/help_content/04_transforms.md +++ b/slayer/memories/help_content/04_transforms.md @@ -12,7 +12,7 @@ becomes an extra CTE in the generated SQL. | `time_shift(x, n)` | Value N time buckets back/ahead (calendar-aware) | Self-join CTE with INTERVAL offset | | `time_shift(x, n, 'year')` | Value at a different granularity offset (e.g. YoY) | Self-join CTE with INTERVAL offset | | `change(x)` | Period-over-period difference (partition-safe, resets per group) | Desugars to `x − time_shift(x, -1)` | -| `change_pct(x)` | Period-over-period % change, e.g. month-over-month growth (partition-safe) | Desugars to `(x − ts) / ts` where `ts = time_shift(x, -1)` | +| `change_pct(x)` | Period-over-period % change, e.g. month-over-month growth (partition-safe; NULL when the prior period's value is 0 or missing) | Desugars to `CASE WHEN ts != 0 THEN (x − ts) / ts END` where `ts = time_shift(x, -1)` | | `lag(x, n)` / `lead(x, n)` | N rows back / ahead | `LAG` / `LEAD` window fn, partitioned by dimensions | | `consecutive_periods(predicate)` | Current trailing run length where predicate is true | Staged window CTEs with reset groups | | `rank(x[, partition_by=...])` | Rank by x, descending; ties skip ranks | `RANK() OVER ([PARTITION BY ...] ORDER BY x DESC)` | diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index b13a3a20..77163e0d 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -469,8 +469,15 @@ async def test_arithmetic_expression(integration_env): assert response.data[0]["orders.avg_amount"] == pytest.approx(125.0) -async def test_time_shift_row_based(integration_env): - """time_shift(x, -1) without granularity → calendar self-join at the query's time grain.""" +async def test_time_shift_default_granularity_calendar(integration_env): + """time_shift(x, n) without granularity → calendar self-join at the query's time grain. + + Filtering to completed orders leaves a gap: Jan(300) and Mar(300), no + completed orders in Feb. Calendar shifting matches on the actual month, + so the gap yields NULLs where row-based LAG/LEAD would return the + adjacent row's value — and reaches across the gap where LAG(2) on a + two-row result would fall off the edge. + """ engine = integration_env query = SlayerQuery( @@ -482,24 +489,30 @@ async def test_time_shift_row_based(integration_env): measures=[ ModelMeasure(formula="total_amount:sum"), ModelMeasure(formula="time_shift(total_amount:sum, -1)", name="prev"), + ModelMeasure(formula="time_shift(total_amount:sum, -2)", name="prev2"), ModelMeasure(formula="time_shift(total_amount:sum, 1)", name="next"), ], + filters=["status == 'completed'"], order=[OrderItem(column=ColumnRef(name="created_at"), direction="asc")], ) response = await engine.execute(query) - # 3 months: Jan(300), Feb(125), Mar(325) - assert response.row_count == 3 + # 2 months with completed orders: Jan(300), Mar(300) — Feb is a gap + assert response.row_count == 2 + + # Calendar backward shift: Mar - 1 month = Feb, which has no data → NULL + # (row-based LAG(1) would have returned Jan's 300) + assert response.data[0]["orders.prev"] is None # Jan - 1 = Dec, no data + assert response.data[1]["orders.prev"] is None # Mar - 1 = Feb, gap - # Row-based backward shift (LAG): first row has no previous - assert response.data[0]["orders.prev"] is None - assert response.data[1]["orders.prev"] == pytest.approx(300.0) # Feb's prev = Jan - assert response.data[2]["orders.prev"] == pytest.approx(125.0) # Mar's prev = Feb + # Calendar shift reaches across the gap: Mar - 2 months = Jan → 300 + # (row-based LAG(2) on a two-row result would have returned NULL) + assert response.data[1]["orders.prev2"] == pytest.approx(300.0) - # Row-based forward shift (LEAD): last row has no next - assert response.data[0]["orders.next"] == pytest.approx(125.0) # Jan's next = Feb - assert response.data[1]["orders.next"] == pytest.approx(325.0) # Feb's next = Mar - assert response.data[2]["orders.next"] is None + # Calendar forward shift: Jan + 1 month = Feb, gap → NULL + # (row-based LEAD(1) would have returned Mar's 300) + assert response.data[0]["orders.next"] is None + assert response.data[1]["orders.next"] is None # Mar + 1 = Apr, no data async def test_time_shift_calendar_based(integration_env): From e4a402c93dc0fbb7823fbbecdf561811c9f43082 Mon Sep 17 00:00:00 2001 From: whimo Date: Mon, 20 Jul 2026 21:11:28 +0400 Subject: [PATCH 4/5] Collision-check hidden transform name allocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qualifying a hoisted transform's name with the owning measure's field_name keeps hoisted names distinct from each other, but not from user measure names. A query declaring a measure literally named `_t0_growth` alongside an arithmetic-wrapped transform on a measure named `growth` put both on the alias `orders._t0_growth`: the self-join CTE projected two columns under that name and the hoisted reference resolved to the user's measure. Valid SQL, silently wrong numbers — verified against DuckDB, where the February row of the repro returned -125.0 instead of -175.0, degenerating `growth` to `-revenue`. Allocate hidden names through `_allocate_hidden_name`, which checks the query's public measure names, previously allocated hidden names, and `known_aliases`, appending `_2`, `_3`, ... until free. Extract `_public_field_name` so the reserved set and the main measure loop derive public names identically. Also restore rather than drop a shadowed `known_aliases` binding after resolving a formula, since a user measure may itself be named `_t0`. Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 2 +- slayer/engine/enrichment.py | 64 ++++++++++++++++++++++++++++++++----- tests/test_sql_generator.py | 38 ++++++++++++++++++++++ 3 files changed, 95 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3ae604df..02779ef9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -72,7 +72,7 @@ poetry run ruff check slayer/ tests/ - **Result column naming**: `revenue:sum` → `orders.revenue_sum` (colon becomes underscore). `*:count` → `orders._count` — the `*` is dropped but the underscore is kept as a leading marker so the alias never collides with a user-defined column literally named `count`. When converting queries to models (`create_model_from_query`), the same colon-to-underscore mapping applies. An explicit `name` on the measure spec overrides the canonical form for **both** simple aggregations and arithmetic/transform formulas — `{"formula": "amount:sum", "name": "rev"}` surfaces as `orders.rev`. This matters most for inner stages of multi-stage `source_queries`, where downstream stages reference inner-stage outputs by the chosen name. **DEV-1443**: when a query measure is renamed via `name`, filters and ORDER BY entries in the same node may reference EITHER the raw colon form (`amount:sum > 100`) OR the user alias (`rev > 100`) — both resolve to the user alias, and a colon-form filter classifies as HAVING. Renaming never changes the legal filter/order form. Two enrichment-time validations on renames: (1) a query measure `name` colliding with a source column on the source model is rejected (the alias-form filter would otherwise silently bind to the source column instead of the aggregate); (2) a rename whose canonical alias (e.g. `revenue_sum`) literally shadows a source column on the same model is also rejected — the colon-form filter would otherwise be ambiguous between the renamed aggregate and the source column. **DEV-1448**: the user-supplied `name` on a *cross-model* aggregated measure (`{"formula": "customers.revenue:sum", "name": "cust_rev"}`) is now honored for the projection alias and the downstream-stage virtual model column. Only the canonical **leaf** of the dotted path is swapped to the user name; the hop path is preserved — same dot-syntax shape as every other multi-hop caller-facing key. So `{"formula": "customers.revenue:sum", "name": "cust_rev"}` surfaces as `orders.customers.cust_rev`, and `{"formula": "customers.regions.population:sum", "name": "region_pop"}` surfaces as `orders.customers.regions.region_pop`. Two caller-facing surfaces: (a) the top-level result-column key uses the hop-preserved form (`orders.customers.cust_rev`); (b) the downstream-stage virtual model column built by `_query_as_model` uses the BARE user name (`cust_rev`) — a special-case in the cross-model loop short-circuits the `__`-flattening of `_alias_to_short` whenever `cm.name` is a bare identifier, so a stage-2 reference like `cust_rev:max` resolves directly. The inner `CrossModelMeasure.measure` (used to build the CTE aggregate expression) intentionally retains the canonical form — only the outer handle is renamed. The canonical-collision guard from DEV-1443 was lifted into a pre-pass so it fires for both local AND cross-model renames symmetrically. Covers `customers.*:count` and `customers.col:count_distinct` cross-model variants. Same-stage **filter** referencing a renamed cross-model measure (`filters=["cust_rev > 100"]` or the colon form `"customers.revenue:sum > 100"`) is NOT auto-resolved — both raise at strict resolution; the SQL generator has no path to route the bare user alias to the cross-model CTE's output column for filter classification, so admitting the filter would emit broken SQL (`WHERE orders.cust_rev > 100` against a column that doesn't exist on the base table). Filter remap stays DEV-1445 territory. **ORDER BY** referencing the bare user alias (`order=[{"column": "cust_rev"}]`) DOES resolve via `SQLGenerator._resolve_order_column`'s `alias_lookup[cm.name] = cm.alias` mapping and emits `ORDER BY "orders.customers.cust_rev"` against the cross-model CTE's output column. Workaround until DEV-1445 lands: restructure as a multi-stage `source_queries` so the cross-model measure becomes a local measure in the downstream stage. See companion tickets DEV-1445 (cross-model filter remap) and DEV-1446 (transform-wrapped inner-ref dedup). - **Response attributes**: `SlayerResponse.attributes` is a `ResponseAttributes` with `.dimensions` and `.measures` dicts, each mapping column alias → `FieldMetadata(label, format)`. Split by type so consumers can distinguish dimension metadata from measure metadata. - Available formula transforms: cumsum, time_shift, change, change_pct, rank, percent_rank, dense_rank, ntile, first (FIRST_VALUE window ASC), last (FIRST_VALUE window DESC), lag, lead, consecutive_periods. time_shift uses a self-join CTE where the shifted sub-query has the time column expression offset by INTERVAL (calendar-based, gap-safe). change and change_pct are desugared at enrichment time into a hidden time_shift + arithmetic expression. lag/lead use LAG/LEAD window functions directly (more efficient but produce NULLs at edges). Non-transform SQL function calls (`nullif`, `coalesce`, `ln`, `sqrt`, etc.) may also wrap aggregated refs inside arithmetic expressions, e.g. `"*:count / nullif(revenue:max, 0)"` — the call passes through to emitted SQL while the inner refs resolve to their measure aliases -- **Hidden-transform naming** (DEV-1692): a transform wrapped in arithmetic (`revenue:sum - time_shift(revenue:sum, -1, 'month')`) is hoisted out of the formula under a hidden name. `slayer/core/formula.py::_parse_mixed_arithmetic` mints per-formula placeholders `_t0`, `_t1`, … from a counter that **restarts on every formula parse**, so the `MixedArithmeticField` branch of `_flatten_spec` (`slayer/engine/enrichment.py`) qualifies each one with the owning measure's `field_name` — `_t0_growth`, `_t1_growth_pct` — before handing it to `_add_transform`. `field_name` is unique per query (duplicate measure names are rejected), so the hoisted names are too. This matters because the name becomes both the self-join CTE name (`shifted_{name}` / `sjoin_{name}` in `slayer/sql/generator.py`) and the projection alias: unqualified, two measures each wrapping a transform both became `_t0`, which surfaced as a duplicate-CTE parser error for `time_shift`/`change` and, for the CTE-less window transforms (`cumsum`, `lag`, …), as *silently wrong values* (the second measure read the first's). The `change`/`change_pct` desugar uses the same scheme (`_ts_{field_name}`) and composes — `change(x) * 2` yields `_ts__t0_{field_name}`. Filter-extracted transforms use a separate `_ft{n}` namespace off a query-wide shared counter, so they never collide either. Extending: any new hoisting path must derive its name from `field_name`, never from a formula-local counter alone +- **Hidden-transform naming** (DEV-1692): a transform wrapped in arithmetic (`revenue:sum - time_shift(revenue:sum, -1, 'month')`) is hoisted out of the formula under a hidden name. `slayer/core/formula.py::_parse_mixed_arithmetic` mints per-formula placeholders `_t0`, `_t1`, … from a counter that **restarts on every formula parse**, so the `MixedArithmeticField` branch of `_flatten_spec` (`slayer/engine/enrichment.py`) qualifies each one with the owning measure's `field_name` — `_t0_growth`, `_t1_growth_pct` — and then runs it through `_allocate_hidden_name` before handing it to `_add_transform`. The `field_name` qualifier is what separates hoisted names from *each other*, but it is not sufficient on its own: nothing stops a user from also declaring a measure literally named `_t0_growth`, which would claim the same alias and make the hoisted reference silently resolve to that measure (valid SQL, wrong numbers). So allocation is **collision-checked** against the query's public measure names (`_public_field_name` over `query.measures`), previously allocated hidden names, and `known_aliases`, appending `_2`, `_3`, … until free. This matters because the name becomes both the self-join CTE name (`shifted_{name}` / `sjoin_{name}` in `slayer/sql/generator.py`) and the projection alias: unqualified, two measures each wrapping a transform both became `_t0`, which surfaced as a duplicate-CTE parser error for `time_shift`/`change` and, for the CTE-less window transforms (`cumsum`, `lag`, …), as *silently wrong values* (the second measure read the first's). The `change`/`change_pct` desugar uses the same scheme (`_ts_{field_name}`) and composes — `change(x) * 2` yields `_ts__t0_{field_name}`. Filter-extracted transforms use a separate `_ft{n}` namespace off a query-wide shared counter, so they never collide either. Extending: any new hoisting path must derive its name from `field_name`, never from a formula-local counter alone - **Rank-family transforms** (DEV-1353): `rank`, `percent_rank`, `dense_rank`, and `ntile` are timeless window-function transforms emitted as `RANK() / PERCENT_RANK() / DENSE_RANK() / NTILE(n) OVER (... ORDER BY DESC)`. They default to **no `PARTITION BY`** (rank across the entire result set, unlike cumsum/lag/lead which auto-partition by query dimensions), and accept an optional `partition_by=col` or `partition_by=[col1, col2]` kwarg to opt into per-partition ranking; the columns referenced must be query dimensions or time dimensions. `ntile` additionally requires `n=`. Standard SQL across SQLite (≥3.25), Postgres, DuckDB, MySQL, and ClickHouse — no UDFs needed. - Filters can reference computed field names or contain inline transform expressions (e.g., `"change(revenue:sum) > 0"`, `"last(change(revenue:sum)) < 0"`). These are auto-extracted as hidden fields and applied as post-filters on the outer query - **Window functions in filters**: filter strings and `ModelMeasure.formula` cannot contain raw `OVER (...)` SQL — SLayer's formula parser is Python-AST-based and rejects with an actionable error pointing at the `rank()` / `first()` / `last()` / `lag()` / `lead()` transforms. Filtering on a `Column` whose `sql` contains a window function is also rejected (DEV-1369; the prior auto-promotion escape hatch from DEV-1336 is removed). For top-N use the inline `rank() <= N` transform (or `dense_rank` / `percent_rank` / `ntile(n=)`); for non-standard window expressions, factor them into an earlier stage of a multi-stage `source_queries` model. diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index e575f598..f58c69a8 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -157,6 +157,18 @@ async def _collect_reachable_agg_names( return frozenset(names) if names else None +def _public_field_name(qfield: Any) -> str: + """The public name a query measure surfaces under. + + An explicit ``name`` wins; otherwise the formula is mangled into an + identifier. Shared by the main measure loop and the reserved-name set + that hidden-transform allocation checks against, so the two can't drift. + """ + return qfield.name or qfield.formula.replace(" ", "_").replace("/", "_div_").replace(":", "_").replace( + "*", "" + ) + + async def enrich_query( query: SlayerQuery, model: SlayerModel, @@ -596,6 +608,32 @@ def _resolve_rank_partition(transform: str, partition_by: list[str]) -> list[str ) return resolved + # DEV-1692: names for transforms hoisted out of an arithmetic formula are + # derived from the owning measure's field_name, which keeps them distinct + # from one another — but nothing stops a user from *also* declaring a + # measure literally named ``_t0_growth``. Both would then claim the alias + # ``._t0_growth``: the self-join CTE projects two columns under that + # name and the hoisted reference silently resolves to the user's measure + # (no error, wrong numbers). Allocate against everything already spoken + # for instead of trusting field_name uniqueness on its own. + _reserved_public_names: set[str] = { + _public_field_name(qf) for qf in (query.measures or []) + } + _hidden_names: set[str] = set() + + def _allocate_hidden_name(preferred: str) -> str: + candidate = preferred + suffix = 2 + while ( + candidate in _reserved_public_names + or candidate in _hidden_names + or candidate in known_aliases + ): + candidate = f"{preferred}_{suffix}" + suffix += 1 + _hidden_names.add(candidate) + return candidate + def _add_transform( name: str, transform: str, @@ -888,19 +926,31 @@ async def _flatten_spec(spec, field_name: str) -> str: # on the self-join CTE names (``shifted__t0``) and, worse, on the # expression alias, silently making the second measure read the # first one's value. Qualify with the owning measure's field_name - # (unique per query), mirroring the ``_ts_{field_name}`` naming the - # change/change_pct desugar already uses. + # and run it through _allocate_hidden_name so the result can't + # collide with a user measure that happens to share the shape. placeholder_aliases: list[tuple[str, str]] = [] for placeholder, sub_transform in spec.sub_transforms: + hidden_name = _allocate_hidden_name(f"{placeholder}_{field_name}") sub_alias = await _flatten_spec( - sub_transform, f"{placeholder}_{field_name}" + spec=sub_transform, field_name=hidden_name ) placeholder_aliases.append((placeholder, sub_alias)) + # Bind the placeholders just long enough for _resolve_sql to rewrite + # this formula's references, then restore. A user measure may itself + # be named `_t0`, so a shadowed binding is put back rather than + # dropped. + shadowed: list[tuple[str, str | None]] = [ + (placeholder, known_aliases.get(placeholder)) + for placeholder, _ in placeholder_aliases + ] for placeholder, sub_alias in placeholder_aliases: known_aliases[placeholder] = sub_alias resolved_sql = _resolve_sql(spec.sql) - for placeholder, _ in placeholder_aliases: - known_aliases.pop(placeholder, None) + for placeholder, prior in shadowed: + if prior is None: + del known_aliases[placeholder] + else: + known_aliases[placeholder] = prior alias = f"{model_name_str}.{field_name}" enriched_expressions.append( EnrichedExpression( @@ -1285,9 +1335,7 @@ def _mangled_formula(formula: str) -> str: f"ORDER BY would otherwise bind to the source column " f"instead of the renamed aggregate." ) - field_name = qfield.name or qfield.formula.replace(" ", "_").replace("/", "_div_").replace(":", "_").replace( - "*", "" - ) + field_name = _public_field_name(qfield) if isinstance(spec, AggregatedMeasureRef): # New colon syntax: "revenue:sum", "*:count", etc. diff --git a/tests/test_sql_generator.py b/tests/test_sql_generator.py index e8631101..34378ced 100644 --- a/tests/test_sql_generator.py +++ b/tests/test_sql_generator.py @@ -1387,6 +1387,44 @@ async def test_multiple_time_shifts_resolve_to_distinct_aliases( by_name = {e.name: e.sql for e in enriched.expressions} assert by_name["growth"] != by_name["growth_2m"] + async def test_hidden_transform_name_avoids_user_measure_collision( + self, orders_model: SlayerModel + ) -> None: + """DEV-1692: a hoisted transform must not land on a user measure's name. + + Hidden names are built from the owning measure's field_name, so a user + measure literally named `_t0_growth` would otherwise claim the same + alias as the shift hoisted out of `growth` — the self-join CTE projects + both under that name and the shift silently resolves to the user's + measure (valid SQL, wrong numbers). + """ + orders_model.default_time_dimension = "created_at" + query = SlayerQuery( + source_model="orders", + time_dimensions=[ + TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH) + ], + measures=[ + ModelMeasure(formula="revenue:sum - time_shift(revenue:sum, -1, 'month')", name="growth"), + ModelMeasure(formula="revenue:sum * 2", name="_t0_growth"), + ], + ) + enriched = await enrich_query( + query=query, + model=orders_model, + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + + transform_aliases = {t.alias for t in enriched.transforms} + expression_aliases = {e.alias for e in enriched.expressions} + assert not (transform_aliases & expression_aliases) + + # `growth` must reference the hoisted shift, not the user's measure. + growth_sql = next(e.sql for e in enriched.expressions if e.name == "growth") + assert "orders._t0_growth" not in growth_sql.replace("orders._t0_growth_2", "") + async def test_dev_1692_repro_shape( self, generator: SQLGenerator, orders_model: SlayerModel ) -> None: From 66d9ca9e12da19ff86787aa776b1a164a9a6095f Mon Sep 17 00:00:00 2001 From: whimo Date: Mon, 20 Jul 2026 21:21:26 +0400 Subject: [PATCH 5/5] Reserve dimension names against hidden transform allocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dimensions and time dimensions alias as `.`, the same shape hoisted transforms use, but only measure names were reserved. A dimension named `_t0_growth` therefore claimed the same alias as the shift hoisted out of a measure named `growth`, and the measure silently computed `revenue - ` instead of the period difference. Verified against DuckDB with a numeric colliding dimension (tier=7): growth returned 293/118/318 — `revenue - 7` — where Jan=NULL, Feb=-175, Mar=200 was correct. A non-numeric colliding dimension surfaces as a binder type error instead, so the failure mode is silent only when the types happen to line up. Reserve dimensions and time dimensions by bare name alongside the existing measure reservations. Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 2 +- slayer/engine/enrichment.py | 22 +++++++++++++--------- tests/test_sql_generator.py | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 10 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 02779ef9..27b167fa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -72,7 +72,7 @@ poetry run ruff check slayer/ tests/ - **Result column naming**: `revenue:sum` → `orders.revenue_sum` (colon becomes underscore). `*:count` → `orders._count` — the `*` is dropped but the underscore is kept as a leading marker so the alias never collides with a user-defined column literally named `count`. When converting queries to models (`create_model_from_query`), the same colon-to-underscore mapping applies. An explicit `name` on the measure spec overrides the canonical form for **both** simple aggregations and arithmetic/transform formulas — `{"formula": "amount:sum", "name": "rev"}` surfaces as `orders.rev`. This matters most for inner stages of multi-stage `source_queries`, where downstream stages reference inner-stage outputs by the chosen name. **DEV-1443**: when a query measure is renamed via `name`, filters and ORDER BY entries in the same node may reference EITHER the raw colon form (`amount:sum > 100`) OR the user alias (`rev > 100`) — both resolve to the user alias, and a colon-form filter classifies as HAVING. Renaming never changes the legal filter/order form. Two enrichment-time validations on renames: (1) a query measure `name` colliding with a source column on the source model is rejected (the alias-form filter would otherwise silently bind to the source column instead of the aggregate); (2) a rename whose canonical alias (e.g. `revenue_sum`) literally shadows a source column on the same model is also rejected — the colon-form filter would otherwise be ambiguous between the renamed aggregate and the source column. **DEV-1448**: the user-supplied `name` on a *cross-model* aggregated measure (`{"formula": "customers.revenue:sum", "name": "cust_rev"}`) is now honored for the projection alias and the downstream-stage virtual model column. Only the canonical **leaf** of the dotted path is swapped to the user name; the hop path is preserved — same dot-syntax shape as every other multi-hop caller-facing key. So `{"formula": "customers.revenue:sum", "name": "cust_rev"}` surfaces as `orders.customers.cust_rev`, and `{"formula": "customers.regions.population:sum", "name": "region_pop"}` surfaces as `orders.customers.regions.region_pop`. Two caller-facing surfaces: (a) the top-level result-column key uses the hop-preserved form (`orders.customers.cust_rev`); (b) the downstream-stage virtual model column built by `_query_as_model` uses the BARE user name (`cust_rev`) — a special-case in the cross-model loop short-circuits the `__`-flattening of `_alias_to_short` whenever `cm.name` is a bare identifier, so a stage-2 reference like `cust_rev:max` resolves directly. The inner `CrossModelMeasure.measure` (used to build the CTE aggregate expression) intentionally retains the canonical form — only the outer handle is renamed. The canonical-collision guard from DEV-1443 was lifted into a pre-pass so it fires for both local AND cross-model renames symmetrically. Covers `customers.*:count` and `customers.col:count_distinct` cross-model variants. Same-stage **filter** referencing a renamed cross-model measure (`filters=["cust_rev > 100"]` or the colon form `"customers.revenue:sum > 100"`) is NOT auto-resolved — both raise at strict resolution; the SQL generator has no path to route the bare user alias to the cross-model CTE's output column for filter classification, so admitting the filter would emit broken SQL (`WHERE orders.cust_rev > 100` against a column that doesn't exist on the base table). Filter remap stays DEV-1445 territory. **ORDER BY** referencing the bare user alias (`order=[{"column": "cust_rev"}]`) DOES resolve via `SQLGenerator._resolve_order_column`'s `alias_lookup[cm.name] = cm.alias` mapping and emits `ORDER BY "orders.customers.cust_rev"` against the cross-model CTE's output column. Workaround until DEV-1445 lands: restructure as a multi-stage `source_queries` so the cross-model measure becomes a local measure in the downstream stage. See companion tickets DEV-1445 (cross-model filter remap) and DEV-1446 (transform-wrapped inner-ref dedup). - **Response attributes**: `SlayerResponse.attributes` is a `ResponseAttributes` with `.dimensions` and `.measures` dicts, each mapping column alias → `FieldMetadata(label, format)`. Split by type so consumers can distinguish dimension metadata from measure metadata. - Available formula transforms: cumsum, time_shift, change, change_pct, rank, percent_rank, dense_rank, ntile, first (FIRST_VALUE window ASC), last (FIRST_VALUE window DESC), lag, lead, consecutive_periods. time_shift uses a self-join CTE where the shifted sub-query has the time column expression offset by INTERVAL (calendar-based, gap-safe). change and change_pct are desugared at enrichment time into a hidden time_shift + arithmetic expression. lag/lead use LAG/LEAD window functions directly (more efficient but produce NULLs at edges). Non-transform SQL function calls (`nullif`, `coalesce`, `ln`, `sqrt`, etc.) may also wrap aggregated refs inside arithmetic expressions, e.g. `"*:count / nullif(revenue:max, 0)"` — the call passes through to emitted SQL while the inner refs resolve to their measure aliases -- **Hidden-transform naming** (DEV-1692): a transform wrapped in arithmetic (`revenue:sum - time_shift(revenue:sum, -1, 'month')`) is hoisted out of the formula under a hidden name. `slayer/core/formula.py::_parse_mixed_arithmetic` mints per-formula placeholders `_t0`, `_t1`, … from a counter that **restarts on every formula parse**, so the `MixedArithmeticField` branch of `_flatten_spec` (`slayer/engine/enrichment.py`) qualifies each one with the owning measure's `field_name` — `_t0_growth`, `_t1_growth_pct` — and then runs it through `_allocate_hidden_name` before handing it to `_add_transform`. The `field_name` qualifier is what separates hoisted names from *each other*, but it is not sufficient on its own: nothing stops a user from also declaring a measure literally named `_t0_growth`, which would claim the same alias and make the hoisted reference silently resolve to that measure (valid SQL, wrong numbers). So allocation is **collision-checked** against the query's public measure names (`_public_field_name` over `query.measures`), previously allocated hidden names, and `known_aliases`, appending `_2`, `_3`, … until free. This matters because the name becomes both the self-join CTE name (`shifted_{name}` / `sjoin_{name}` in `slayer/sql/generator.py`) and the projection alias: unqualified, two measures each wrapping a transform both became `_t0`, which surfaced as a duplicate-CTE parser error for `time_shift`/`change` and, for the CTE-less window transforms (`cumsum`, `lag`, …), as *silently wrong values* (the second measure read the first's). The `change`/`change_pct` desugar uses the same scheme (`_ts_{field_name}`) and composes — `change(x) * 2` yields `_ts__t0_{field_name}`. Filter-extracted transforms use a separate `_ft{n}` namespace off a query-wide shared counter, so they never collide either. Extending: any new hoisting path must derive its name from `field_name`, never from a formula-local counter alone +- **Hidden-transform naming** (DEV-1692): a transform wrapped in arithmetic (`revenue:sum - time_shift(revenue:sum, -1, 'month')`) is hoisted out of the formula under a hidden name. `slayer/core/formula.py::_parse_mixed_arithmetic` mints per-formula placeholders `_t0`, `_t1`, … from a counter that **restarts on every formula parse**, so the `MixedArithmeticField` branch of `_flatten_spec` (`slayer/engine/enrichment.py`) qualifies each one with the owning measure's `field_name` — `_t0_growth`, `_t1_growth_pct` — and then runs it through `_allocate_hidden_name` before handing it to `_add_transform`. The `field_name` qualifier is what separates hoisted names from *each other*, but it is not sufficient on its own: nothing stops a user from also declaring a measure literally named `_t0_growth`, which would claim the same alias and make the hoisted reference silently resolve to that measure (valid SQL, wrong numbers). So allocation is **collision-checked** against every projected name — the query's public measure names (`_public_field_name` over `query.measures`), the selected dimensions and time dimensions (reserved by bare `name`, since they alias as `.` exactly like hoisted transforms do), previously allocated hidden names, and `known_aliases` — appending `_2`, `_3`, … until free. This matters because the name becomes both the self-join CTE name (`shifted_{name}` / `sjoin_{name}` in `slayer/sql/generator.py`) and the projection alias: unqualified, two measures each wrapping a transform both became `_t0`, which surfaced as a duplicate-CTE parser error for `time_shift`/`change` and, for the CTE-less window transforms (`cumsum`, `lag`, …), as *silently wrong values* (the second measure read the first's). The `change`/`change_pct` desugar uses the same scheme (`_ts_{field_name}`) and composes — `change(x) * 2` yields `_ts__t0_{field_name}`. Filter-extracted transforms use a separate `_ft{n}` namespace off a query-wide shared counter, so they never collide either. Extending: any new hoisting path must derive its name from `field_name`, never from a formula-local counter alone - **Rank-family transforms** (DEV-1353): `rank`, `percent_rank`, `dense_rank`, and `ntile` are timeless window-function transforms emitted as `RANK() / PERCENT_RANK() / DENSE_RANK() / NTILE(n) OVER (... ORDER BY DESC)`. They default to **no `PARTITION BY`** (rank across the entire result set, unlike cumsum/lag/lead which auto-partition by query dimensions), and accept an optional `partition_by=col` or `partition_by=[col1, col2]` kwarg to opt into per-partition ranking; the columns referenced must be query dimensions or time dimensions. `ntile` additionally requires `n=`. Standard SQL across SQLite (≥3.25), Postgres, DuckDB, MySQL, and ClickHouse — no UDFs needed. - Filters can reference computed field names or contain inline transform expressions (e.g., `"change(revenue:sum) > 0"`, `"last(change(revenue:sum)) < 0"`). These are auto-extracted as hidden fields and applied as post-filters on the outer query - **Window functions in filters**: filter strings and `ModelMeasure.formula` cannot contain raw `OVER (...)` SQL — SLayer's formula parser is Python-AST-based and rejects with an actionable error pointing at the `rank()` / `first()` / `last()` / `lag()` / `lead()` transforms. Filtering on a `Column` whose `sql` contains a window function is also rejected (DEV-1369; the prior auto-promotion escape hatch from DEV-1336 is removed). For top-N use the inline `rank() <= N` transform (or `dense_rank` / `percent_rank` / `ntile(n=)`); for non-standard window expressions, factor them into an earlier stage of a multi-stage `source_queries` model. diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index f58c69a8..b9737c23 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -610,15 +610,19 @@ def _resolve_rank_partition(transform: str, partition_by: list[str]) -> list[str # DEV-1692: names for transforms hoisted out of an arithmetic formula are # derived from the owning measure's field_name, which keeps them distinct - # from one another — but nothing stops a user from *also* declaring a - # measure literally named ``_t0_growth``. Both would then claim the alias - # ``._t0_growth``: the self-join CTE projects two columns under that - # name and the hoisted reference silently resolves to the user's measure - # (no error, wrong numbers). Allocate against everything already spoken - # for instead of trusting field_name uniqueness on its own. - _reserved_public_names: set[str] = { - _public_field_name(qf) for qf in (query.measures or []) - } + # from one another — but nothing stops a user from *also* selecting a + # measure or dimension literally named ``_t0_growth``. Both would then + # claim the alias ``._t0_growth``: the self-join CTE projects two + # columns under that name and the hoisted reference silently resolves to + # the user's column (no error, wrong numbers). Allocate against every + # projected name instead of trusting field_name uniqueness on its own. + # Dimensions and time dimensions alias as ``.`` just like + # hoisted transforms do, so they are reserved by bare name too. + _reserved_public_names: set[str] = ( + {_public_field_name(qf) for qf in (query.measures or [])} + | {d.name for d in dimensions} + | {td.name for td in time_dimensions} + ) _hidden_names: set[str] = set() def _allocate_hidden_name(preferred: str) -> str: diff --git a/tests/test_sql_generator.py b/tests/test_sql_generator.py index 34378ced..3ccc491c 100644 --- a/tests/test_sql_generator.py +++ b/tests/test_sql_generator.py @@ -1425,6 +1425,43 @@ async def test_hidden_transform_name_avoids_user_measure_collision( growth_sql = next(e.sql for e in enriched.expressions if e.name == "growth") assert "orders._t0_growth" not in growth_sql.replace("orders._t0_growth_2", "") + async def test_hidden_transform_name_avoids_dimension_collision( + self, orders_model: SlayerModel + ) -> None: + """DEV-1692: dimensions alias as `.` too, so they're reserved. + + A dimension named `_t0_growth` otherwise claims the same alias as the + shift hoisted out of `growth`, and the measure silently computes + `revenue - ` instead of the period difference. + """ + orders_model.default_time_dimension = "created_at" + orders_model.columns.append( + Column(name="_t0_growth", sql="customer_id", type=DataType.DOUBLE) + ) + query = SlayerQuery( + source_model="orders", + dimensions=[ColumnRef(name="_t0_growth")], + time_dimensions=[ + TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH) + ], + measures=[ + ModelMeasure(formula="revenue:sum - time_shift(revenue:sum, -1, 'month')", name="growth"), + ], + ) + enriched = await enrich_query( + query=query, + model=orders_model, + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + + dim_aliases = {d.alias for d in enriched.dimensions} | { + td.alias for td in enriched.time_dimensions + } + transform_aliases = {t.alias for t in enriched.transforms} + assert not (dim_aliases & transform_aliases) + async def test_dev_1692_repro_shape( self, generator: SQLGenerator, orders_model: SlayerModel ) -> None: