DEV-1713 Stage 9: full naming module (result keys, flat names, dialect alias mangling) - #269
Merged
Conversation
…t mangling) Grow slayer/sql/naming.py into the single owner of every alias/result-key decision and close the DEV-1495/DEV-1692 defects it gates. - naming module: result_key() (dotted final keys), result_key_from_alias() (canonical dotted aliases), flat_name() (inner-stage __ binds); relocate encode_alias/decode_alias (delete dialects/_alias_mangle.py) and the DEV-1645 mixed-case quoting policy here; add assert_unique_cte_names() (per-WITH belt). - D3 (DEV-1495 bug 1): joined DERIVED dimensions project + return under the dotted key (orders.customers.rev_x2), not the flat orders.customers__revenue. generator._full_alias_for_slot and response_meta._slot_result_keys both route the three ROW key shapes through result_key so they can't drift; ORDER BY on a projected joined dim follows the same dotted alias. - Bare named-measure aliasing: a bare saved-measure ref surfaces under the measure NAME, not the formula-derived canonical. - DEV-1692: unique CTE names AND unique hidden time_shift value aliases in the typed pipeline (fixes both the duplicate-WITH error and the value collapse). - Remove the BigQuery scope-validator TypeError carve-out (verified zero residual); BigQuery output is now validated like every dialect. - Legacy flatteners delegate to flat_name (byte-identical). - Promote the 5 Stage-9 pins (2 named-measure, 2 DEV-1692, DEV-1495 bug 1) and un-pin the 2 OSI notebooks + DEV-1692 integration test. Full non-integration suite green (8197 passed); ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR centralizes SQL naming helpers, standardizes dotted result keys and saved-measure aliases, allocates unique transform CTE names, and removes the BigQuery SQL validation exception. It adds coverage for naming, multi-stage queries, time shifts, alias mangling, quoting, and scoped CTE validation. ChangesNaming and SQL validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant QueryEngine
participant StagePlanner
participant SQLGenerator
participant ResponseMeta
QueryEngine->>StagePlanner: resolve aliases and saved measures
StagePlanner->>SQLGenerator: provide planned query slots
SQLGenerator->>SQLGenerator: allocate aliases and emit SQL
SQLGenerator->>ResponseMeta: provide projected aliases
ResponseMeta->>QueryEngine: return normalized result keys
Possibly related PRs
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
slayer/sql/scope_check.py (2)
114-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse keyword arguments for both validation calls.
Lines 124 and 126 pass
sqlpositionally. Callassert_scope_closed(sql=sql, dialect=dialect)andassert_unique_cte_names(sql=sql, dialect=dialect).As per coding guidelines, “Use keyword arguments for functions with more than one parameter.”
🤖 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/scope_check.py` around lines 114 - 126, The validation calls in the environment-gated flow should pass both parameters by keyword. Update assert_scope_closed and assert_unique_cte_names to use sql=sql and dialect=dialect, preserving their existing order and behavior.Source: Coding guidelines
87-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse keyword arguments for
sqlglot.parse_one.Change line 87 to
sqlglot.parse_one(sql=sql, dialect=dialect)to follow the project convention for functions with multiple parameters.🤖 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/scope_check.py` at line 87, Update the sqlglot.parse_one call in the scope-checking flow to pass the SQL input using the sql keyword argument while retaining the existing dialect keyword argument.Source: Coding guidelines
slayer/engine/stage_planner.py (2)
95-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local
flat_namevariables so they do not shadow the new import.Line 95 binds
flat_nameas a module-level function. Inside_declared_measures_from_query, lines 1158 and 1180 rebindflat_nameto astr. The shadow is confined to that function body, so the current code works. However, any later call toflat_name(...)after line 1158 in that function raisesTypeError: 'str' object is not callable. Rename the locals toflat.♻️ Proposed rename inside `_declared_measures_from_query`
- flat_name = _flatten_dotted(full) + flat = _flatten_dotted(full) fmt, desc = _format_description_for_dimension( scope=scope, full_name=full, ) dim_type = _type_for_dimension( scope=scope, full_name=full, bundle=bundle, ) declared.append(DeclaredMeasure( bound=bound, - declared_name=flat_name, - public_name=flat_name, + declared_name=flat, + public_name=flat,Apply the same rename to the time-dimension loop at lines 1180-1184.
🤖 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/engine/stage_planner.py` at line 95, In _declared_measures_from_query, rename both local string variables currently named flat_name to flat, including the time-dimension loop, and update their references while preserving the imported flat_name function for calls.
1113-1133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared saved-measure lookup out of the two gate functions.
_bare_saved_measure_namerepeats the exact prelude of_saved_model_measure_type(lines 1104-1109): theModelScope/source_modelcheck, thestrip().isidentifier()gate, and theget_measure(bare)lookup. Only the returned attribute differs. If one gate is later changed (for example to accept a qualified form), the two silently diverge, and a measure could lift its saved type without lifting its saved name. Extract one helper that returns theModelMeasure.♻️ Proposed shared helper
+def _bare_saved_measure( + *, scope: Union[ModelScope, StageSchema], formula: str, +) -> Optional[ModelMeasure]: + """The saved ``ModelMeasure`` when ``formula`` is a BARE reference to one. + + Single gate shared by the type-lift and name-lift helpers so the two + cannot drift. Fires only for a bare identifier matching a + ``ModelMeasure.name`` on the source model. + """ + if not isinstance(scope, ModelScope) or scope.source_model is None: + return None + bare = formula.strip() + if not bare.isidentifier(): + return None + return scope.source_model.get_measure(bare) + + def _bare_saved_measure_name( *, scope: Union[ModelScope, StageSchema], formula: str, ) -> Optional[str]: - if not isinstance(scope, ModelScope) or scope.source_model is None: - return None - bare = formula.strip() - if not bare.isidentifier(): - return None - saved = scope.source_model.get_measure(bare) + saved = _bare_saved_measure(scope=scope, formula=formula) return saved.name if saved is not None else NoneRoute
_saved_model_measure_typethrough the same helper and returnsaved.type.🤖 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/engine/stage_planner.py` around lines 1113 - 1133, Extract the shared ModelMeasure lookup prelude from `_bare_saved_measure_name` and `_saved_model_measure_type` into one helper that performs the ModelScope/source-model check, bare-identifier validation, and `get_measure` lookup, returning the saved measure or None. Update both gate functions to call this helper, with `_bare_saved_measure_name` returning `saved.name` and `_saved_model_measure_type` returning `saved.type`.tests/test_dev1713_naming.py (1)
65-149: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClean up the temp directory and dispose the engine in the fixture.
Line 65 creates a directory with
tempfile.mkdtemp(). The fixture yields at line 149 and never removes that directory, so every test that usesengineleaves a directory, a SQLite file, and a YAML store behind for the process lifetime. The fixture also never callsengine.aclose(), so the per-engine async SQL clients are not disposed — the repo documentsaclose()as the disposal path for pooled connections. Use pytest'stmp_pathfor the directory and add teardown after theyield.♻️ Proposed fixture cleanup
`@pytest.fixture` -async def engine() -> AsyncIterator[SlayerQueryEngine]: - d = tempfile.mkdtemp() - db_path = os.path.join(d, "t.db") +async def engine(tmp_path) -> AsyncIterator[SlayerQueryEngine]: + d = str(tmp_path) + db_path = os.path.join(d, "t.db")- yield SlayerQueryEngine(storage=storage) + eng = SlayerQueryEngine(storage=storage) + try: + yield eng + finally: + await eng.aclose()With
tmp_pathin use, thetempfileimport at line 26 becomes unused; drop it.🤖 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 `@tests/test_dev1713_naming.py` around lines 65 - 149, Update the fixture to accept pytest’s tmp_path fixture and use it instead of tempfile.mkdtemp() for db_path and the YAMLStorage base directory; remove the now-unused tempfile import. After yielding the SlayerQueryEngine, add teardown that awaits engine.aclose() so pooled SQL clients are disposed.
🤖 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/sql/generator.py`:
- Around line 3281-3288: Update the consecutive-periods slot handling around
_iter_slot_deps and _canonical_name so hidden slots receive unique aliases. When
slot.public_aliases is empty, allocate slot_alias through
cte_allocator.allocate_cte(slot.declared_name), then use that alias for both
full_slot_alias and cp_reset_alias; preserve canonical/public alias behavior for
slots with public aliases.
In `@tests/test_dev1713_naming.py`:
- Around line 63-64: Add module-level pytestmark in
tests/test_dev1713_naming.py, adjacent to the existing imports or fixtures,
setting it to pytest.mark.integration so the engine fixture’s real file-backed
SQLite queries are classified as integration tests. Do not add an
external-database skip.
---
Nitpick comments:
In `@slayer/engine/stage_planner.py`:
- Line 95: In _declared_measures_from_query, rename both local string variables
currently named flat_name to flat, including the time-dimension loop, and update
their references while preserving the imported flat_name function for calls.
- Around line 1113-1133: Extract the shared ModelMeasure lookup prelude from
`_bare_saved_measure_name` and `_saved_model_measure_type` into one helper that
performs the ModelScope/source-model check, bare-identifier validation, and
`get_measure` lookup, returning the saved measure or None. Update both gate
functions to call this helper, with `_bare_saved_measure_name` returning
`saved.name` and `_saved_model_measure_type` returning `saved.type`.
In `@slayer/sql/scope_check.py`:
- Around line 114-126: The validation calls in the environment-gated flow should
pass both parameters by keyword. Update assert_scope_closed and
assert_unique_cte_names to use sql=sql and dialect=dialect, preserving their
existing order and behavior.
- Line 87: Update the sqlglot.parse_one call in the scope-checking flow to pass
the SQL input using the sql keyword argument while retaining the existing
dialect keyword argument.
In `@tests/test_dev1713_naming.py`:
- Around line 65-149: Update the fixture to accept pytest’s tmp_path fixture and
use it instead of tempfile.mkdtemp() for db_path and the YAMLStorage base
directory; remove the now-unused tempfile import. After yielding the
SlayerQueryEngine, add teardown that awaits engine.aclose() so pooled SQL
clients are disposed.
🪄 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: f5b0938e-ecc6-44b8-b339-9d476992de68
📒 Files selected for processing (22)
DECISIONS.mdslayer/engine/enrichment.pyslayer/engine/query_engine.pyslayer/engine/response_meta.pyslayer/engine/stage_planner.pyslayer/sql/dialects/_alias_mangle.pyslayer/sql/dialects/bigquery.pyslayer/sql/dialects/tsql.pyslayer/sql/generator.pyslayer/sql/naming.pyslayer/sql/scope_check.pyslayer/sql/stage_wrapper.pytests/_engine_helpers.pytests/dialects/conftest.pytests/dialects/test__alias_mangle.pytests/integration/test_notebooks.pytests/parity_xfails.pytests/test_dev1713_naming.pytests/test_naming.pytests/test_projection_trim.pytests/test_scope_check.pytests/test_sql_generator.py
💤 Files with no reviewable changes (4)
- tests/integration/test_notebooks.py
- slayer/sql/dialects/_alias_mangle.py
- tests/test_projection_trim.py
- tests/parity_xfails.py
…eRabbit fixes Address the PR #269 review round (Codex + CodeRabbit + Sonar): - CodeRabbit (Major): apply the DEV-1692 hidden-alias de-collision to _emit_consecutive_periods_ctes_for_planned too — two arithmetic-wrapped consecutive_periods slots shared the _consecutive_periods_inner placeholder and collided the same way time_shift did. Add a 2-consecutive_periods test. - CodeRabbit (nitpick): keyword args on the two maybe_validate_scopes calls. - Sonar S5778: hoist the ModelMeasure build out of the pytest.raises block so only one invocation can throw. - Sonar S3776: NOSONAR the CC-46 _emit_time_shift_ctes_for_planned (one cohesive per-slot emission; matches this file's 22 other S3776 suppressions). - Codex (case-sensitive CTE collision): deferred to DEV-1726 (dialect-aware fold); pre-existing + edge case, recorded in DECISIONS.md. - CodeRabbit (integration marker): replied invalid — file-backed SQLite tests are not marked integration by convention (siblings confirm). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Sonar S7632: rewrite the NOSONAR(S3776) reason on _emit_time_shift_ctes_for_planned to be parenthesis-free — SonarQube greedily matches parens after NOSONAR, so a second (...) in the prose was parsed as malformed rule-keys. - Sonar S3776: NOSONAR _emit_consecutive_periods_ctes_for_planned (CC 26; modified by the round-1 consecutive_periods de-collision) with a paren-free reason. - Codex: seed the typed transform-chain allocator with the bare forms of every already-projected column alias, so a hidden transform alias (_time_shift_inner / _consecutive_periods_inner) can never shadow a real user column of that name (mirrors the legacy path seeding base_aliases). Add a regression test. Full non-integration suite green (8199 passed); ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings in Stage 4 (DEV-1708) + Stage 6 (DEV-1710). Conflict resolution: - DECISIONS.md: kept both sides, ordered chronologically (Stage 4/6 then Stage 9). - tests/test_dev1713_naming.py: two F6 tests combined a joined derived dim with a cross-model measure, which DEV-1708 now deliberately gates (NotImplementedError — a derived dim as cross-model shared grain is deferred to DEV-1495-b1). Adjusted both to use local measures only; the derived-dim dotted-key coverage is unaffected, cross-model dotted keys stay covered by test_cross_model_measure_key_still_dotted. Full support tracked in DEV-1728. Full non-integration suite green (8286 passed); ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_sql_generator.py (1)
241-242: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the DEV reference in the comment.
The promoted tests correspond to DEV-1526 Stage 4. Line [242] names DEV-1708 Stage 4 instead. Change the reference to keep the test history accurate.
Proposed fix
-# DEV-1708 Stage 4; its pinned strict-xfails were promoted to plain tests. +# DEV-1526 Stage 4; its pinned strict-xfails were promoted to plain tests.🤖 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 `@tests/test_sql_generator.py` around lines 241 - 242, Update the history comment above the promoted tests to reference DEV-1526 Stage 4 instead of DEV-1708 Stage 4, leaving the rest of the comment unchanged.
🤖 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.
Outside diff comments:
In `@tests/test_sql_generator.py`:
- Around line 241-242: Update the history comment above the promoted tests to
reference DEV-1526 Stage 4 instead of DEV-1708 Stage 4, leaving the rest of the
comment unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c74f0a9-d63c-48cd-9872-13ad682001ae
📒 Files selected for processing (6)
DECISIONS.mdslayer/sql/dialects/tsql.pyslayer/sql/generator.pyslayer/sql/scope_check.pytests/test_dev1713_naming.pytests/test_sql_generator.py
🚧 Files skipped from review as they are similar to previous changes (2)
- slayer/sql/dialects/tsql.py
- slayer/sql/scope_check.py
…flat_name
Codex: two cross-model downstream-name paths in enrichment.py still called
`.replace(".", "__")` directly (lines 1152, 1485), bypassing the naming module.
Route both through `flat_name()` (byte-identical) so the single-owner flatten
contract holds across every legacy delegation site in this file.
Full non-integration suite green (8286 passed); ruff clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings in Stage 5 (DEV-1709, widened Law-3 isolation trigger). Only conflict was DECISIONS.md (append-vs-append) — kept both sides, Stage 5 entry ordered before the Stage 9 entries. Code auto-merged cleanly. Full non-integration suite green (8360 passed); ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DECISIONS.md (1)
78-78: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not defer case-folded CTE collisions without a release guard.
Case-only measure aliases can produce duplicate CTE names on case-folding dialects. Add dialect-aware collision checks or reject unsafe aliases before release. Add regression tests for case-folding and case-sensitive dialects.
🤖 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 78, Update the decision documented for DEV-1726 so release is blocked until case-folded CTE collisions are handled: implement dialect-aware collision detection or reject unsafe case-only aliases before CTE generation. Preserve distinct names on case-sensitive dialects, and add regression coverage for both dialect categories before removing the deferment.
🤖 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 deferred-case description in the decision entry by
changing “crossing derived column” to “crossing-derived column,” preserving all
other wording and content.
---
Outside diff comments:
In `@DECISIONS.md`:
- Line 78: Update the decision documented for DEV-1726 so release is blocked
until case-folded CTE collisions are handled: implement dialect-aware collision
detection or reject unsafe case-only aliases before CTE generation. Preserve
distinct names on case-sensitive dialects, and add regression coverage for both
dialect categories before removing the deferment.
🪄 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: f77776da-3db9-429c-8ac4-ba3ca51f8415
📒 Files selected for processing (5)
DECISIONS.mdslayer/engine/enrichment.pyslayer/engine/stage_planner.pyslayer/sql/generator.pytests/test_sql_generator.py
🚧 Files skipped from review as they are similar to previous changes (4)
- slayer/engine/enrichment.py
- slayer/engine/stage_planner.py
- tests/test_sql_generator.py
- slayer/sql/generator.py
| @@ -70,3 +70,9 @@ implementation detail. Include issue refs when known. | |||
| - 2026-08-02 — Cross-model / isolation CTE renderer on `ScopeFrame` (DEV-1708, DEV-1703 Stage 4). The forward `_cm_*` CTE renderer (`generator.py::_render_cross_model_cte`) and its routed-filter renderer now build a per-CTE `ScopeFrame` rooted at the target relation and route every expression through it (Law 1): the rerooted aggregate source, positional args, column-ref kwargs, `Column.filter`, shared-grain dimensions, target-model filters, and routed host WHERE/HAVING filters all `resolve()`/register their crossed joins into the CTE's single ordered `join_paths` set — the ad-hoc `_add_cte_join_paths` closure is deleted and the CTE FROM is built from that set. Closes **DEV-1526** (source `Column.sql` crossing a further join), **DEV-1527** cross-model remainder (a column-ref kwarg naming a derived target column now expands through the scope and is embedded as a typed `ResolvedAggKwarg(kind="expr")`, not a bare non-existent column), and the WHERE/HAVING routed-filter derived-ref gap. A routed-filter **pre-pass** (`_register_routed_filter_joins`) walks the full `ValueKey` tree (nested arithmetic/boolean/IN operands + aggregate leaves' source/args/kwargs/`column_filter`) before the FROM is built, so a HAVING (rendered later, after the ranked-subquery rn maps exist) still contributes its joins. **Law 2** (DEV-1702 B2, forward variant): when the CTE contains a first/last ranked subquery whose SOURCE value crosses a join, the crossing value is materialised as a `_val_<n>` projection INSIDE the subquery (the outer `MAX(CASE WHEN _last_rn = 1 THEN … END)` would otherwise reference a table bound only inside the subquery); the routed-HAVING variant binds the SAME alias via `FirstLastRenderState.value_alias_by_sql`. A generation-wide `AliasAllocator` is installed by `generate_from_planned` (save/restore) so inline forward CTEs and the host base share `_val_<n>` naming; recursive rerooted sub-generations get their own. **DEV-1701** host-side fix: a joined derived TIME dimension whose `Column.sql` crosses a further join expands with `is_root=False` (host-path alias `customers_v2__regions`, not bare `regions`) in the shared `_raw_time_col_expr_for_planned`, fixing host base and CTE from one helper — deliberately touching host-base rendering (nominally out of the issue's scope) because the e2e query is otherwise invalid and the suite-wide scope validator rejects it. **Null-safe grain join-back** (Codex F2): the combined-SELECT `LEFT JOIN _cm_* ON` uses `SqlDialect.build_null_safe_eq` — base `NullSafeEQ` → `IS NOT DISTINCT FROM` (Postgres/DuckDB/Snowflake/BigQuery/Trino/Presto/Databricks/Spark/ClickHouse), MySQL `<=>`, SQLite `IS` override (native form needs ≥3.39), and an expanded `a = b OR (a IS NULL AND b IS NULL)` for T-SQL/Oracle/Redshift — so NULL dimension values and nullable truncated time grains join back instead of dropping. **Decision (user-approved):** a PLAIN derived (non-time) dimension used as cross-model shared grain now raises `NotImplementedError` (planner, gated on `not hidden` so filter-only derived refs are unaffected) instead of silently CROSS-JOIN-broadcasting the global aggregate across groups — full support rides with DEV-1495-b1 (Stage 8/9). Out of scope, deferred to Stage 5 (DEV-1531/1709): the DEV-1702-B2 filtered-local (host-rooted) variant, whose value materialisation lives in `_build_first_last_base_select`. | |||
| - 2026-08-02 — first/last explicit-time completion (DEV-1710, DEV-1703 Stage 6): a first/last explicit ranking-time arg (`amount:last(customers.signup_at)`) now discovers its crossed join through the host `ScopeFrame` like every other input, not through a bespoke collector. Three sites are unified on one arg-selection contract, `SQLGenerator._explicit_time_arg_of(key)` (first positional arg iff it is a `ColumnKey`/`ColumnSqlKey`, else `None` — first/last never takes a leading non-column positional): the raise-gate in `_build_first_last_base_select` (was a divergent `any(isinstance(a,(ColumnKey,ColumnSqlKey)))` scan-all-args, so a scalar-first/col-later shape slipped the "requires a ranking time column" raise and then crashed building the `ROW_NUMBER` map — Codex F1), the new discovery sub-pass, and the render seam. `_resolve_agg_inputs_via_scope` gains sub-pass 4 (position 7): for each LOCAL first/last it `scope.resolve(arg)` register-only, so the crossed LEFT JOIN base-pulls as a Law-1 side effect (bare single-hop, bare multi-hop — every prefix registered — and local derived args whose `Column.sql` reaches a joined table); a path-bearing `ColumnSqlKey` (the DEV-1526 residual) is skipped here, not anchored. `_resolve_explicit_time_col` renders through a throwaway host-rooted `ScopeFrame` when a bundle is present (its `join_paths` discarded — discovery is owned by the base pass; same throwaway pattern as `_resolve_agg_kwargs_for_key`), which also gains DEV-1686 reserved-word qualifier quoting for free; the early returns/raises (None for no explicit arg, `NotImplementedError` DEV-1526 before any resolve, `ValueError` for a not-found derived column before any resolve) are preserved in order, and the pre-existing `bundle=None` fallback (bare-ident f-string / verbatim emit) stays verbatim because the `_build_agg_render_spec_from_planned` unit pins and the two direct-call guard tests invoke it bundle-less. Safe because `synth.time_column` is re-parsed downstream (interpolated into a `ROW_NUMBER() OVER (... ORDER BY {tc} ...)` string that `_parse` re-emits), so resolver normalization (`date()`→`DATE()`) washes out. The `Phase.AGGREGATE` arm of `_collect_joined_paths_for_base` is deleted and its signature narrowed to `(base_render_order, slots_by_id)` — it now collects only ROW dimension paths. Closes DEV-1476 fully (all four acceptance reprose green: local no-TD, cross-model bare, cross-model derived, plus the Stage-A local cases). Deliberately NOT done under Option A: the residual-hop `NotImplementedError` is kept (narrowed, owned by DEV-1526/Stage 4) rather than removed, and Stage 5 arg-isolation is not pulled forward. Out of scope and flagged separately: a derived time column whose `Column.sql` references ANOTHER derived column (nested inlining) — `expand_derived_refs_sync` does not recursively inline bare sibling-derived refs, and this fails identically for a plain dimension, so it is a general column-expansion limitation, not a time-arg concern. | |||
| - 2026-08-03 — Widened Law-3 isolation trigger (DEV-1709, DEV-1703 Stage 5): a LOCAL aggregate isolates into a host-rooted `_cm_*` CTE when ANY explicit input crosses a join — source `Column.sql` (dotted / `__` / sibling derived chains), `Column.filter` (the pre-existing DEV-1503 half), positional args incl. the explicit first/last time arg (D2), and kwargs (column refs, user template-fragment strings, and non-overridden model-default `AggregationParam.sql` fragments — an unparseable fragment contributes nothing, parity with the filter scan's fallback, a documented D1 carve-out). Non-filter kinds are computed plan-time by `slayer/engine/aggregate_input_paths.py::compute_aggregate_input_join_paths` (crossing info recomputed from the bundle, never cached on keys — DEV-1703 Q3); the recursion flag is renamed `disable_dev1503_isolation` → `disable_host_rooted_isolation` and gates ONLY the host-rooted half. Headline consequence: the top-level host base only ever contains purely-local aggregates, so a measure-pulled 1:N join can no longer multiply the rows sibling measures see (sibling protection — pinned by executed DuckDB values); the crossing measure itself keeps multiply-per-match semantics inside its CTE (F1). Aggregate-phase filters referencing a newly-isolated aggregate route to the combined-SELECT outer WHERE (never HAVING-into-the-CTE); host ROW filters (local and pathed) propagate into the host-rooted sub-plan (F4); composite crossing leaves isolate individually per interned `AggregateKey` slot (identical keys share one CTE, distinct keys get distinct CTEs — merging is DEV-1688/`may_inline` territory). Inside the CTE's sub-render, `_build_first_last_base_select` gains the Law-2 materialisation (closing DEV-1531 and DEV-1702-B1): every crossing input expression the ranked outer scope consumes — aggregate SOURCE and column-ref KWARG values (also closing the DEV-1527/DEV-1476 first/last kwarg deferral) — is projected inside the ranked subquery as a `_val_<n>` whose body is the RESOLVED value (qualified + `Column.type` inner CAST for non-bare expressions, so `SUM(CAST(x AS t))` semantics survive materialisation and same-sql-different-type aggregates keep distinct `_val`s); alias maps are keyed by that resolved text end-to-end (host path, Stage-4 CTE path, HAVING/composite consumers). `_validate_aggregate_kwarg_paths` is relaxed for LOCAL sources (structurally-crossing kwargs are now supported inputs; the cross-model path-mismatch rejection survives). Deferred with a strict-xfail + follow-up ticket: an IMPLICITLY-resolved crossing time column (model `default_time_dimension` pointing at a crossing derived column) does not trigger — D2 covers explicit args only, and plan-time duplication of the render-time time-fallback resolution was judged not worth the drift risk. | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hyphenate the compound modifier.
Change crossing derived column to crossing-derived column for clear description of the deferred case.
🧰 Tools
🪛 LanguageTool
[grammar] ~72-~72: Use a hyphen to join words.
Context: ...t_time_dimension` pointing at a crossing derived column) does not trigger — D2 co...
(QB_NEW_EN_HYPHEN)
🤖 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, Update the deferred-case description in the
decision entry by changing “crossing derived column” to “crossing-derived
column,” preserving all other wording and content.
Source: Linters/SAST tools
ZmeiGorynych
merged commit Aug 3, 2026
1d4a27d
into
egor/dev-1703-comprehensive-approach-expressions-crossing-joins-on-the
6 checks passed
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.
Closes the naming half of the DEV-1703 typed-pipeline redesign (spec v3, Stage 9, decision D3). Scoped Stage 9: lands on the Stage-3 baseline; only the pins that don't depend on Stages 4–8 are promoted.
What lands
slayer/sql/naming.py) becomes the single owner of every alias / result-key decision:result_key()— dotted FINAL-stage keys (hops viapath, dot-freeleaf)result_key_from_alias()— an already-canonical relative alias that may embed hop dots (cross-model measurecustomers.revenue_sum)flat_name()— the__-joined INNER-stage downstream bind namesencode_alias/decode_aliasbijection (dialects/_alias_mangle.pydeleted)assert_unique_cte_names()— per-WITH-scope CTE-collision belt (DEV-1692)ColumnSqlKeywith a path) now projects + returns under the dotted keyorders.customers.rev_x2, not the flatorders.customers__revenue._full_alias_for_slotandresponse_meta._slot_result_keysboth route the three ROW key shapes throughresult_key, so the SQL alias and response key can't drift. ORDER BY on a projected joined dim follows the same dotted alias. Deliberate breaking change for consumers adapted to the flat form.time_shiftvalue aliases (the value collapse the duplicate-WITH error had masked)._alias_to_short,_alias_to_short_local,_flatten_dotted,_cte_name_from_alias, stage-wrapper strip) delegate toflat_name(byte-identical) so the two forms can't drift while the legacy stack lives.Pins promoted
test_named_measures.py::TestBareNamedMeasureAliasing::{test_select_alias_uses_measure_name, test_order_by_resolves_against_measure_name}test_sql_generator.py::TestFields::test_multiple_time_shifts_in_arithmetic_unique_ctes+integration::test_multiple_time_shifts_in_one_querytest_projection_trim.py::…::test_cross_model_dotted_dimension_projection(in-source xfail removed)Verification
tests/test_dev1713_naming.py+ additions totests/test_naming.py.DECISIONS.mdupdated with the Stage-9 entries.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes