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/CLAUDE.md b/CLAUDE.md index f246be43..27b167fa 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` — 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/docs/concepts/formulas.md b/docs/concepts/formulas.md index 40e7c626..af4e5113 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; 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)` | @@ -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/engine/enrichment.py b/slayer/engine/enrichment.py index 025c1c35..b9737c23 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,36 @@ 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* 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: + 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, @@ -882,13 +924,42 @@ 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 + # 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: - await _flatten_spec(sub_transform, placeholder) + hidden_name = _allocate_hidden_name(f"{placeholder}_{field_name}") + sub_alias = await _flatten_spec( + 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, 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( name=field_name, - sql=_resolve_sql(spec.sql), + sql=resolved_sql, alias=alias, ) ) @@ -1268,9 +1339,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/slayer/mcp/server.py b/slayer/mcp/server.py index 036d9b5c..63d556ae 100644 --- a/slayer/mcp/server.py +++ b/slayer/mcp/server.py @@ -346,11 +346,21 @@ 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: 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"]. 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..e533f89a 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; 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)` | @@ -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 0b35d4d9..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 → LAG (previous row).""" +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): @@ -531,6 +544,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..3ccc491c 100644 --- a/tests/test_sql_generator.py +++ b/tests/test_sql_generator.py @@ -1326,6 +1326,175 @@ 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_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_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: + """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"