Fix duplicate CTE name when a query has multiple arithmetic-wrapped transforms#247
Merged
whimo merged 5 commits intoJul 20, 2026
Conversation
…ransforms
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 <[email protected]>
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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change documents calendar-aware, partition-safe transform semantics, qualifies hidden arithmetic-transform aliases by measure name, and adds regression tests for multiple ChangesCalendar transforms and hidden aliases
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Query
participant Enrichment
participant SQLGenerator
participant IntegrationTests
Query->>Enrichment: submit multiple arithmetic-wrapped time_shift formulas
Enrichment->>Enrichment: allocate distinct collision-safe aliases
Enrichment->>SQLGenerator: emit independent shifted CTEs and expressions
SQLGenerator-->>IntegrationTests: return calendar-aware query results
IntegrationTests->>IntegrationTests: verify gap handling and distinct offsets
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/concepts/formulas.md`:
- Around line 138-139: The user-facing documentation for change_pct must state
that a zero shifted baseline returns NULL instead of showing unconditional
division. Update the change_pct entries at docs/concepts/formulas.md lines
138-139 and slayer/memories/help_content/04_transforms.md lines 14-15
consistently, either documenting the CASE WHEN ts != 0 guard or explicitly
describing the NULL behavior; no direct change is required for change.
In `@slayer/engine/enrichment.py`:
- Around line 895-903: The hidden alias allocation around _flatten_spec must
avoid collisions with all public measure names and previously allocated hidden
aliases, not just rely on field_name qualification; update this flow to choose
an available alias before flattening each sub-transform and preserve the
resulting alias mappings. In CLAUDE.md lines 75-75, document that hidden names
use collision-checked allocation and remove any claim that field_name uniqueness
alone guarantees uniqueness.
- Around line 895-903: Update the `_flatten_spec` call in this placeholder-alias
flow to pass its arguments by keyword, and replace
`known_aliases.pop(placeholder, None)` with `del known_aliases[placeholder]`
because each temporary binding is guaranteed to exist. Keep the surrounding
alias resolution behavior unchanged.
In `@slayer/mcp/server.py`:
- Around line 357-361: Update the transform documentation near the
change/change_pct/time_shift description to state that change and change_pct
default to comparing with the prior time bucket, while time_shift compares using
its explicitly requested offset and granularity. Preserve the existing
explanation of non-time dimension joins and grouped-query behavior.
In `@tests/integration/test_integration.py`:
- Line 473: Update the regression test around the time_shift calendar self-join
to use non-contiguous month data, such as January and March with February
absent, and assert that March shifted by one month returns NULL. Rename the test
and its comments to describe calendar shifting rather than row-based LAG/LEAD
behavior, ensuring the assertions distinguish the two implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fee66179-35e6-4ae5-81ad-4ee98d9ceb56
📒 Files selected for processing (10)
.claude/skills/slayer-query.mdCLAUDE.mddocs/concepts/formulas.mddocs/concepts/terminology.mddocs/examples/04_time/time_nb.ipynbslayer/engine/enrichment.pyslayer/mcp/server.pyslayer/memories/help_content/04_transforms.mdtests/integration/test_integration.pytests/test_sql_generator.py
…-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.
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 <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/engine/enrichment.py`:
- Around line 619-621: Extend the _reserved_public_names construction to include
the public names of all selected dimensions and time dimensions in addition to
query measures. Reuse the existing field-name helper and selection symbols so
names such as _t0_growth are reserved before hoisted transforms are assigned
aliases, while preserving the current measure reservations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c8ab919-1a93-4d36-aae3-5bbc54adce26
📒 Files selected for processing (7)
CLAUDE.mddocs/concepts/formulas.mdslayer/engine/enrichment.pyslayer/mcp/server.pyslayer/memories/help_content/04_transforms.mdtests/integration/test_integration.pytests/test_sql_generator.py
🚧 Files skipped from review as they are similar to previous changes (3)
- slayer/mcp/server.py
- CLAUDE.md
- slayer/memories/help_content/04_transforms.md
Dimensions and time dimensions alias as `<model>.<name>`, 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 - <dimension>` 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 <[email protected]>
|
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes DEV-1692.
Problem
A query with two or more
time_shift()calls embedded in arithmetic formulas — the standard period-over-period shape — failed at SQL generation:Root cause
When a transform is wrapped in arithmetic it gets hoisted out of the formula under a placeholder name.
slayer/core/formula.py::_parse_mixed_arithmeticmints those placeholders (_t0,_t1, …) from a counter that restarts on every formula parse, so every such measure claimed the name_t0. That name flows through_flatten_specverbatim and becomes both the self-join CTE name (shifted_{name}/sjoin_{name}) and the projection alias — so two measures collided on both.Fix
Qualify each placeholder with the owning measure's
field_namein theMixedArithmeticFieldbranch of_flatten_spec, mirroring the_ts_{field_name}scheme thechange/change_pctdesugar already uses.field_nameis unique per query (duplicate measure names are rejected), so the hoisted names are too. One file, ~20 lines.Bare top-level
time_shiftmeasures already used their own measure name and are untouched.Two things worth flagging beyond the ticket
1. The duplicate CTE name was only the visible half. Both measures' expressions also resolved to the same alias (
orders._t0), so a rename-only fix at emission time would have left the second measure silently reading the first's shifted value. Uniquifying at hoist time re-wires the expression references too.2. The same collision silently corrupted results for window transforms.
cumsum/lag/leadwrapped in arithmetic emit no CTE, so two such measures produced valid SQL with duplicate aliases — no error, just wrong numbers. Confirmed against the pre-fix code:Relatedly, the ticket's suggested workaround ("use
change()instead") only held for barechange(x)measures —change(x) * 2in two measures hit the identical duplicate-CTE failure. Both are fixed here.Tests
tests/test_sql_generator.py— three tests: distinct CTE names, distinct aliases/offsets (pins the silent-corruption half), and the ticket's exact four-measure repro shape.tests/integration/test_integration.py— one execution-level test asserting real values with distinct offsets (-1vs-2), so a regression that re-collapses the shifts surfaces as wrong numbers rather than only a parser error.All four verified to fail without the fix and pass with it.
Verification
Regressions were checked by diffing the full failure set before and after, not by comparing counts: 141 unit failures before, 141 after, identical test names. Those are all pre-existing environment issues (the
advanced_search/tantivyextra isn't installed, Postgres unavailable, plus a pre-existing ClickHouseWEEK_SUNDAYand SQLite-ingest failure). Same identical-set result for the integration suites (54 before / 54 after).Dialect tests asserting on CTE names (
shifted__ts_pct,shifted_rev_prev) are unaffected — both of those paths keep their naming.ruff check slayer/ tests/passes.Edge cases verified to compose:
change()inside arithmetic (_ts__t0_a), mixedlag+time_shiftwithin one formula, the filter-extraction_ft{n}namespace, and the nested-self-join guard still raising.Not included
Deduplicating content-identical shift CTEs — the repro emits one per occurrence, which is correct but verbose. That's an optimization rather than a correctness issue and is better scoped alongside DEV-1446.
Docs: no page claimed this limitation, so nothing to correct. The related transform-doc rewording is tracked separately as DEV-1693. Added a CLAUDE.md note on the naming rule, since "derive hoisted names from
field_name, never from a formula-local counter" is exactly what the next hoisting path would otherwise re-break.Summary by CodeRabbit
Bug Fixes
time_shiftcalculations appear in the same query.time_shiftoffsets independent across expressions to avoid unintended reuse.change/change_pctand grouped period-over-period calculations reset by non-time dimensions and returnNULLfor missing calendar periods.Documentation
time_shiftas calendar-aware andchange/change_pctas partition-safe and gap-safe.Tests