DEV-1711 Stage 7: time_shift CTEs on ScopeFrame (cross-model partitions) - #272
Conversation
Migrate `_emit_time_shift_ctes_for_planned` onto a per-slot `ScopeFrame`: every partition key and the shift-axis time expression enters through `scope.resolve()` (Law 1), and the shifted CTE's FROM is built from the scope's registered `join_paths`. Closes DEV-1474 and, as one uniform rule, three adjacent silent-broadcast defects the old joinless shifted CTE could not express. - ScopeFrame._anchor: a path'd ColumnSqlKey (derived column ON a joined model) expands at the `__`-path alias with is_root=False (DEV-1701 shape), instead of mis-anchoring at the host relation. - Remove the `7b.12` cross-model-partition raise; widen the auto-partition walk from ColumnKey-only to ColumnKey | ColumnSqlKey | TimeTruncKey ROW slots (the shift axis is excluded by slot id). The sjoin grain is now uniformly every projected dimension: joined columns (stores.name), derived columns (local upper(status) or joined stores.tier), and any secondary time dimension. - Joined time axis (stores.opened_at) resolves through the scope, pulling its join into the shifted CTE. - Lift the joined-filter guard: _build_shifted_cte_where_parts returns (parts, crossed_join_paths) and _guard_no_joined_refs is deleted; a joined-column ROW filter (query or Mode-A model filter) now pulls its join and re-aggregates over the same filtered population as _base. - Null-safe sjoin grain join-back (Codex F2): time axis + every partition pair use _null_safe_join_pair_sql, so a NULL dimension value or NULL time bucket keeps its prior-period value instead of dropping under plain `=`. Tests: new tests/test_dev1474_time_shift_cross_model_partition.py (SQL-shape + hand-computed DuckDB execution ground-truth); ScopeFrame path'd-ColumnSqlKey unit tests in test_scope.py; the DEV-1474 carrier_scope_matrix pin promoted from strict-xfail; 04_time QoQ notebook un-skipped (09_lightning_talk stays skipped for the unrelated DEV-1692/Stage-9 duplicate-CTE-name gap). Docs: formulas.md null-safe/every-dim grain wording + DECISIONS.md entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough
ChangesTime-shift scope resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PlannedTransform
participant ScopeFrame
participant ShiftedCTE
participant SourceJoins
participant SJoin
PlannedTransform->>ShiftedCTE: pass filters and filter paths
ShiftedCTE->>ScopeFrame: resolve dimensions, time axis, and inputs
ScopeFrame->>SourceJoins: register required joins
ShiftedCTE->>SourceJoins: build shifted CTE FROM and joins
ShiftedCTE->>SJoin: compare grain columns with null-safe equality
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
slayer/sql/generator.py (1)
8133-8196: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConfirm the rendered-text scan cannot silently lose a join path.
_collect_pathsreceives the already-rendered predicate SQL and re-parses it inside_filter_join_paths._filter_join_pathsswallows parse failures and returns no paths. If a rendered predicate ever fails_parse_predicatefor the active dialect, the shifted CTE loses the LEFT JOIN the filter needs and emits an unbound alias instead of raising.For typed filters the join paths are already available structurally through
_value_key_join_paths, which does not depend on a text round-trip. Consider using that for thefp.expressionbranch and keeping_filter_join_pathsonly for the Mode-Afp.textbranch.🤖 Prompt for 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. In `@slayer/sql/generator.py` around lines 8133 - 8196, Update the fp.expression branch in the surrounding filter-building method to collect join paths structurally via _value_key_join_paths instead of rescanning rendered SQL with _collect_paths; retain _collect_paths only for the fp.text branch. Preserve the existing BetweenKey omission and predicate rendering behavior, and ensure each structural path is deduplicated in crossed_paths.
🤖 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`:
- Line 158: Update the inline SQL example in the self-join explanation to use
the documented null-safe comparison operator for both the shifted time column
and projected dimensions, such as IS NOT DISTINCT FROM or the dialect-equivalent
syntax, while preserving the existing grain and partition-safety description.
---
Nitpick comments:
In `@slayer/sql/generator.py`:
- Around line 8133-8196: Update the fp.expression branch in the surrounding
filter-building method to collect join paths structurally via
_value_key_join_paths instead of rescanning rendered SQL with _collect_paths;
retain _collect_paths only for the fp.text branch. Preserve the existing
BetweenKey omission and predicate rendering behavior, and ensure each structural
path is deduplicated in crossed_paths.
🪄 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: e1fdafd1-1cb4-49b9-ba76-23f585f1e547
📒 Files selected for processing (9)
DECISIONS.mddocs/concepts/formulas.mdslayer/sql/generator.pyslayer/sql/scope.pytests/integration/test_notebooks.pytests/test_carrier_scope_matrix.pytests/test_dev1474_time_shift_cross_model_partition.pytests/test_scope.pytests/test_sql_generator.py
💤 Files with no reviewable changes (1)
- tests/test_sql_generator.py
…oach-expressions-crossing-joins-on-the' into egor/dev-1711-dev-1703-stage-7-time_shift-ctes-on-scopeframe-cross-model # Conflicts: # DECISIONS.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
DECISIONS.md (1)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the decision entry at the decision level.
Line 72 includes internal implementation details, phase behavior, helper mechanics, and test coverage. Keep only the durable decision and its rationale, as required by the file format. Move implementation details to the PR description or technical documentation.
Suggested rewrite
-- 2026-08-03 — time_shift CTEs on ScopeFrame ... +- 2026-08-03 — time_shift CTEs resolve projected partitions, time axes, and joined filters through per-slot ScopeFrame instances, enabling joined and derived dimensions with null-safe grain joins. (DEV-1711 / DEV-1703 Stage 7)🤖 Prompt for 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. In `@DECISIONS.md` at line 72, Rewrite the decision entry to retain only the durable Law-3 isolation decision and its rationale: LOCAL aggregates isolate into host-rooted CTEs when explicit inputs cross joins, preserving sibling-measure correctness while retaining multiply-per-match semantics for the crossing measure. Remove implementation mechanics, helper paths, flag renames, phase-specific behavior, materialization details, validation rules, test references, and deferred-ticket notes from the entry.
🤖 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 `@DECISIONS.md`:
- Line 72: Update the DECISIONS.md wording in the default_time_dimension
sentence to hyphenate the compound modifier as “crossing-derived column,”
without changing the surrounding technical description.
---
Nitpick comments:
In `@DECISIONS.md`:
- Line 72: Rewrite the decision entry to retain only the durable Law-3 isolation
decision and its rationale: LOCAL aggregates isolate into host-rooted CTEs when
explicit inputs cross joins, preserving sibling-measure correctness while
retaining multiply-per-match semantics for the crossing measure. Remove
implementation mechanics, helper paths, flag renames, phase-specific behavior,
materialization details, validation rules, test references, and deferred-ticket
notes from the entry.
🪄 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: e848eaba-de3f-4852-9bba-5040a38ed1c3
📒 Files selected for processing (3)
DECISIONS.mdslayer/sql/generator.pytests/test_sql_generator.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_sql_generator.py
- slayer/sql/generator.py
…/S9073 Address the CodeRabbit nitpick + SonarCloud new-code issues on the Stage-7 diff (quality gate was already green; these clear the attributed issues): - CodeRabbit + Sonar python:S3776 on _build_shifted_cte_where_parts: extract the per-filter rendering into _shifted_where_part. For a TYPED filter the join paths are now scanned STRUCTURALLY on the already-rendered AST via _joined_paths_in_sql (no text round-trip that could silently swallow a parse failure and drop a needed LEFT JOIN); a Mode-A text filter keeps the _filter_join_paths dual raw+expanded scan (DEV-1494). The extraction also drops the function's cognitive complexity back under 15. - Sonar python:S3776 on _emit_time_shift_ctes_for_planned: NOSONAR(S3776) with a justification matching the sibling _render_cross_model_cte — one cohesive per-slot CTE-emission unit whose tightly-coupled state would only scatter through many-argument helpers if split. - Sonar python:S1192: extract the duplicated "SELECT\n " CTE-head literal to the _SQL_SELECT_HEAD module constant (shifted + consecutive-periods sites). - Sonar python:S9073: split the two composite `assert X and Y` filter- preservation assertions into separate assertions. Full non-integration suite green (8317 passed); ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Combine Stage 9's collision-safe CTE naming (cte_allocator threaded through _emit_time_shift_ctes_for_planned) with Stage 7's ScopeFrame-based cross-model partition resolution + null-safe sjoin. Conflict resolution: - _emit_time_shift_ctes_for_planned: keep Stage 9's cte_allocator param + collision-safe shifted_/sjoin_ CTE naming AND Stage 7's shifted ScopeFrame, partition-via-resolve, guard lift, and null-safe grain join-back; merged NOSONAR(S3776) justification covers both. - Merge interaction fix (BigQuery): Stage 9 dropped the BigQuery scope-validator carve-out and added .→___ alias mangling, exposing that Stage 7's null-safe _grain_eq round-tripped the dotted public alias through _null_safe_join_pair_sql's string parse — which re-splits `base.\`orders. created_at\`` into `\`base___orders\`.\`created_at\`` on BigQuery. Build the null-safe predicate from AST nodes directly (alias as one quoted=True identifier), matching the SELECT parts byte-for-byte on every dialect. - DECISIONS.md: keep both the Stage-7 entry and the Stage-9 entries. Full non-integration suite green (8383 passed); ruff clean.
- docs/concepts/formulas.md: align the inline self-join example with the null-safe grain — `ON base.month IS NOT DISTINCT FROM shifted.month AND ...` (the surrounding text already documents null-safe matching). - DECISIONS.md: compress the DEV-1711 Stage-7 entry to the decision + rationale per the file's "1–3 lines, not implementation detail" format; the mechanics live in the PR description, commits, and code comments. Also addresses (already fixed in an earlier commit) the CodeRabbit nitpick that _build_shifted_cte_where_parts rescanned rendered SQL: the typed-filter branch now collects join paths structurally via _joined_paths_in_sql in _shifted_where_part, with _filter_join_paths kept only for the Mode-A text branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
0c1598f
into
egor/dev-1703-comprehensive-approach-expressions-crossing-joins-on-the
1 participant
DEV-1703 Stage 7. Closes DEV-1474. Base branch is the DEV-1703 umbrella, as with the other stage PRs.
What & why
_emit_time_shift_ctes_for_plannedbuilt theshifted_<x>/sjoin_<x>CTE pair with a joinless FROM (_build_from_clause_from_planned), so it could not partition by anything that lived behind a join. That single limitation caused a raise for cross-model partitions and several silent-broadcast bugs. This migrates the shifted CTE onto aScopeFrame: every partition key and the shift-axis time expression enters throughscope.resolve()(Law 1 — anchor + register the crossed join in one call), and the shifted FROM is built from the scope's registeredjoin_paths.Behavior delivered
The sjoin grain is now uniformly every projected dimension, joined back null-safely:
change(order_total:sum)bystores.name) — thestage 7b.12raise is removed (this is DEV-1474). Multi-hop (stores.regions.name) too.upper(status)) or joined (stores.tier); the auto-partition walk widened fromColumnKey-only toColumnKey | ColumnSqlKey | TimeTruncKey. Previously silently broadcast.TimeTruncKey(distinct from the shift axis) now DATE_TRUNC'd, grouped, and joined-on. Previously silently broadcast.stores.opened_atshifted) — the join is pulled into the shifted CTE; previously an unbound-alias scope leak.stores.name = 'North', query or Mode-A model filter) —_build_shifted_cte_where_partsnow returns crossed join paths and the_guard_no_joined_refsraise is deleted; the shifted CTE joins and re-aggregates over the same filtered population as_base.ON(time axis + every partition pair) uses_null_safe_join_pair_sql, so a NULL dimension value or NULL time bucket keeps its prior-period value instead of dropping under plain=.ScopeFrame._anchorgained a path-aware branch: aColumnSqlKeywith non-emptypath(a derived column on a joined model) expands at the__-path alias withis_root=False, mirroring the DEV-1701 time-column branch.Tests
tests/test_dev1474_time_shift_cross_model_partition.py: SQL-shape tests (precise null-safe grain-pair assertions, per-dialect null-safe spelling, filter-preservation, joined secondary TD, local-only regression) + hand-computed DuckDB execution ground-truth (per-store QoQchange/change_pct, incl. a NULL-store group proving the null-safe join; secondary-TD no-broadcast).ScopeFramepath'd-ColumnSqlKeyunit tests intest_scope.py.test_carrier_scope_matrixpin promoted from strict-xfail to a plain passing test.04_timeQoQ notebook un-skipped (now passes end-to-end).09_lightning_talkstays skipped for an unrelated reason — its hero cell runs one query with twotime_shifttransforms that collide on the CTE nameshifted__time_shift_inner(DEV-1692, owned by Stage 9), re-cited accordingly.Docs
docs/concepts/formulas.md(null-safe + every-projected-dim grain wording) and aDECISIONS.mdentry.Out of scope (unchanged guards)
Cross-model aggregate inputs to
time_shift(7b.15eupstream guard); composite-input transforms (7b.11); duplicateshifted_CTE names for multi-time_shift queries (DEV-1692, Stage 9);partition_keys ⊆ dimsvalidation (DEV-1497, Stage 8); windowed measures (Stage 10).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
time_shift,change, andchange_pctmatch projected dimensions, including joined and derived dimensions.