DEV-1450 principled redesign of syntax (WIP)

[ZmeiGorynych]

Summary

Work-in-progress on DEV-1450 — replacing today's tangled multi-path resolution with a typed pipeline of composable stages: raw → slack normalize → parse_expr → bind_expr → ProjectionPlanner → plan_query → PlannedQuery.

• 23 commits since main, broken into ~25 numbered stages.
• The typed pipeline is in place and dormant — generator + engine cutover not yet wired in.
• Per-stage rhythm: failing tests first → Codex review of tests → implement → unit suite + ruff → Codex review of impl → commit.

See the DEV-1450 progress checkpoints for stage-by-stage detail.

Test plan

• All non-integration tests green at each stage (currently 3500 passing).
• SQLite + DuckDB integration tests run by user.
• Generator slices (7b.7–7b.13) assert SQL parity against the legacy generator via a temporary parity adapter.
• Engine cutover (7b.15) adds DEV-1445 / DEV-1446 / DEV-1448 / DEV-1449 end-to-end acceptance tests.
• Bit-identical result keys (orders.revenue_sum, orders._count, orders.customers.regions.name, renamed orders.<user_name>).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Typed multi-stage planning and projection pipeline with cross-model aggregate planning and deterministic filter routing
  • First-class time-dimension support including derived temporal expressions
  • Query transforms (cumsum, change, change_pct, rank, lag/lead) and automatic function-style → colon normalization
  • Structured normalization warnings surfaced in responses

• Bug Fixes

  • Parametric cross-model aggregate result-key collisions resolved
  • Nested transform/aggregate deduplication improved
  • Model-filter handling for derived SQL fixed; windowed filters rejected

• Documentation

  • Comprehensive architecture and pipeline docs added/updated

• Tests

  • Extensive new unit and end-to-end coverage across parsing, binding, planning, and execution

[linear]

DEV-1450

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a typed resolution pipeline: typed keys/phases, Mode‑B parser and Mode‑A SQL canonicalizer, measure expansion, binder, projection/planning (ValueRegistry, ProjectionPlanner), cross‑model planning, source‑bundle builder, normalization rules, engine wiring, response metadata, sync derived‑column expander, structured errors/warnings, docs, and extensive tests.

Changes

DEV-1450 Typed Pipeline and Planning

Layer / File(s)	Summary
Core types and scope slayer/core/keys.py, slayer/core/scope.py, slayer/core/refs.py	Adds ValueKey family with phases, TimeTruncKey, Aggregate/Transform/Arithmetic/Scalar keys, scalar normalization, StageColumn/StageSchema/ModelScope, and agg-kwarg canonicalization helpers.
Errors, warnings, model validators slayer/core/errors.py, slayer/core/warnings.py, slayer/core/models.py	Stable _format_error_message and many typed errors, NormalizationWarning/SlayerNormalizationWarning, UnreachableFilterDroppedWarning; removes multi-dot auto-rewrite from model validators and validates filters via sql predicate parsing.
Parsing (Mode‑B) and parsed-ref walker slayer/engine/syntax.py, tests/test_syntax.py	Introduces Python-AST-based Mode‑B parser, colon preprocessing, parse_filter_expr, and walk_parsed_refs for scope‑free reference extraction.
SQL canonicalization (Mode‑A) slayer/sql/sql_expr.py, tests/test_sql_expr.py	sqlglot wrapper producing SqlExprKey, dialect rewrites (SQLite json_extract, log10/log2), window detection and assert_no_window_in_filter.
Slack-normalization slayer/engine/normalization.py, slayer/core/warnings.py, tests/test_slack_normalization.py	FUNC_STYLE_AGG, MISPLACED_MEASURE, DOT_PATH_IN_SQL rules; normalize_query/normalize_model produce NormalizationResult and structured warnings surfaced via warnings and SlayerResponse.warnings.
Measure pre-expansion & schema-drift extraction slayer/engine/measure_expansion.py, slayer/engine/schema_drift.py, tests/test_model_measure_expansion.py	expand_model_measures with recursion/cycle limits and AST-based formula ref extraction replacing legacy parsing for schema drift.
Binding slayer/engine/binding.py, tests/test_binding.py	bind_expr/bind_filter/bind_time_dimension produce BoundExpr/BoundFilter, scope-dependent ref resolution (ModelScope vs StageSchema), phase computation, transform/agg binding and validations, window-in-filter rejection.
Planned shapes & projection slayer/engine/planned.py, slayer/engine/planning.py, tests/test_planned.py, tests/test_projection_planner.py	PlannedQuery/ValueSlot/FilterPhase shapes, ValueRegistry interning, desugar_change/desugar_change_pct, lower_sugar_transforms, ProjectionPlanner, hidden-slot materialization, and filter→slot mapping.
Cross-model planning slayer/engine/cross_model_planner.py, tests/test_cross_model_planner.py, tests/test_cross_model_planner_wiring.py	CrossModelPlanner protocol, IsolatedCteCrossModelPlanner with host-filter routing, shared-grain computation, CTE schema construction, and optional rerooting.
Source bundle & path resolution slayer/engine/source_bundle.py, slayer/engine/path_resolution.py, tests/test_source_bundle.py, tests/test_path_resolution.py	ResolvedSourceBundle builder, sibling/synthetic model wiring, transitive referenced_models collection, and async walk_join_chain with NoJoinError.
Engine wiring & response metadata slayer/engine/query_engine.py, slayer/engine/response_meta.py, tests/*	Integrates normalization into engine pipeline, propagates structured warnings, computes touched models, persists normalized models, builds response metadata/expected columns for rendered SQL.
SQL helpers & derived expansion slayer/engine/column_expansion.py, tests/test_column_expansion_sync.py	Adds synchronous expand_derived_refs_sync for derived Column.sql inlining and related sync utilities for derived-column expansion.
Docs & navigation docs/**, mkdocs.yml	New architecture docs (parsing, binding, planning, cross-model aggregates, slack-normalization, errors & warnings, orchestration, typed keys) and updated mkdocs nav.
Tests (comprehensive) tests/**	Large test surface covering keys, parser, binding, measure expansion, planning, cross-model planner wiring, normalization, response metadata, integration/acceptance tests for DEV-1445/1446/1448/1449/1450 behaviors.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(102, 153, 255, 0.5)
  participant Client
  participant Engine
  end
  rect rgba(153, 204, 255, 0.5)
  participant Storage
  participant SourceBundle
  end
  rect rgba(204, 255, 204, 0.5)
  participant Parser
  participant Binder
  participant Planner
  end
  rect rgba(255, 204, 153, 0.5)
  participant SQLGen
  participant DB
  end

  Client->>Engine: execute(query)
  Engine->>Storage: build_resolved_source_bundle
  Storage-->>Engine: models
  Engine->>Parser: normalize + parse
  Parser-->>Engine: ParsedExpr
  Engine->>Binder: bind_expr/bind_filter/bind_time_dimension
  Binder-->>Engine: BoundExpr/BoundFilter
  Engine->>Planner: plan_query/plan_stages
  Planner-->>Engine: PlannedQuery
  Engine->>SQLGen: generate_from_planned
  SQLGen->>DB: run SQL
  DB-->>Engine: rows
  Engine-->>Client: SlayerResponse + warnings

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• MotleyAI/slayer#34: Variables-in-filters merging/apply helpers and related wiring.
• MotleyAI/slayer#89: Derived-column expansion behavior (sync/async expansion overlap).
• MotleyAI/slayer#123: Error vocabulary and cycle/alias-collision handling.

Suggested reviewers

• AivanF

Poem

A rabbit taps keys with gentle might,
Parsing colons into tidy light—
It binds and plans through day and night,
Warnings hop and errors write,
Thump—ship the pipeline into flight!

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch egor/dev-1450-principled-redesign-of-syntax

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (8)

tests/test_time_trunc_key.py (1)

109-112: ⚡ Quick win

Specify the exact exception type for frozen instance mutation.

The test uses pytest.raises(Exception) which is too broad. Specify the exact exception type that's raised when attempting to mutate a frozen Pydantic model or dataclass field (typically ValidationError for Pydantic v2 or FrozenInstanceError for frozen dataclasses).

♻️ Proposed fix to specify exception type

+from pydantic import ValidationError
+
 class TestTimeTruncKeyImmutability:
     def test_is_frozen(self) -> None:
         k = TimeTruncKey(column=ColumnKey(leaf="ordered_at"), granularity="month")
-        with pytest.raises(Exception):
+        with pytest.raises(ValidationError):
             k.granularity = "day"  # type: ignore[misc]

Note: Use ValidationError if TimeTruncKey is a Pydantic model, or dataclasses.FrozenInstanceError if it's a frozen dataclass.

🤖 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_time_trunc_key.py` around lines 109 - 112, The test test_is_frozen
should assert the specific exception type instead of pytest.raises(Exception):
replace the broad Exception with the concrete exception thrown when mutating a
frozen TimeTruncKey (use pydantic.errors.ValidationError /
pydantic.ValidationError if TimeTruncKey is a Pydantic model, or
dataclasses.FrozenInstanceError if it is a frozen dataclass), update the test
import(s) accordingly, and keep the mutating line k.granularity = "day" and
references to TimeTruncKey and ColumnKey unchanged.

slayer/engine/path_resolution.py (1)

75-75: ⚡ Quick win

Prefer more specific type annotation.

The nq variable is assigned a dict value, so nq: dict[str, Any] would be clearer than nq: Any.

♻️ Suggested improvement

-    nq: Any = named_queries if named_queries is not None else {}
+    nq: dict[str, Any] = named_queries if named_queries is not None else {}

🤖 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/path_resolution.py` at line 75, The local variable `nq` is
annotated too broadly as `Any`; change its annotation to a more specific mapping
type (e.g., `dict[str, Any]`) so callers and linters know it holds a dict
produced from `named_queries`. Update the assignment `nq: Any = named_queries if
named_queries is not None else {}` to use `nq: dict[str, Any] = ...` (or use
`cast(dict[str, Any], named_queries)` if necessary) and ensure `typing` imports
include `Any`/`dict` typing forms compatible with the project's Python version.

tests/test_path_resolution.py (1)

27-34: 💤 Low value

Consider using keyword arguments consistently in test calls.

The _make_model helper has 2 parameters. Per coding guidelines, functions with more than 1 parameter should be called using keyword arguments. Several calls use positional arguments (e.g., lines 160, 175, 191).

Example:

# Current
return _make_model(model_name)

# Preferred per guideline
return _make_model(name=model_name)

As per coding guidelines: "Use keyword arguments for functions with more than 1 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 `@tests/test_path_resolution.py` around lines 27 - 34, The test helper
_make_model accepts parameters (name, joins=None) but several test calls use
positional args; update all usages to use keyword arguments per guideline: call
_make_model(name=..., joins=...) instead of positional calls (e.g., replace
_make_model(model_name) with _make_model(name=model_name) and any two-argument
positional calls with explicit joins=...), locating usages by the function name
_make_model in the tests and editing each call.

tests/test_source_bundle.py (1)

20-29: 💤 Low value

Consider using keyword arguments consistently in test calls.

The _model helper has 2 parameters. Per coding guidelines, functions with more than 1 parameter should be called using keyword arguments for clarity. Multiple calls throughout this file use positional arguments (e.g., lines 39, 49, 50, 56, 64, 74, 83, etc.).

Example:

# Current
m = _model("orders")
m = _model("orders", ds="warehouse")

# Preferred per guideline
m = _model(name="orders")
m = _model(name="orders", ds="warehouse")

As per coding guidelines: "Use keyword arguments for functions with more than 1 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 `@tests/test_source_bundle.py` around lines 20 - 29, The test helper
_model(name: str, ds: str = "prod") is being invoked with positional arguments
in multiple places; update all calls to use keyword arguments (e.g.,
_model(name="orders") or _model(name="orders", ds="warehouse")) so they follow
the guideline for functions with more than one parameter; search for usages of
_model in tests/test_source_bundle.py and replace positional invocations with
keyword form, leaving the SlayerModel construction (and related Column/DataType
references) unchanged.

tests/test_boundexpr_unification.py (1)

161-161: ⚡ Quick win

Move function-local imports to module scope.

Line 161 and Line 177 import inside test methods; this violates the repo rule to keep imports at the top of the file.

Suggested diff

 from slayer.core.enums import DataType
-from slayer.core.keys import AggregateKey, ColumnKey
+from slayer.core.keys import AggregateKey, ArithmeticKey, ColumnKey
 from slayer.core.models import Column, SlayerModel
 from slayer.core.query import SlayerQuery
 from slayer.engine.binding import BoundExpr as BinderBoundExpr
+from slayer.engine.binding import walk_value_keys
 from slayer.engine.planned import BoundExpr as PlannedBoundExpr
 from slayer.engine.source_bundle import ResolvedSourceBundle
 from slayer.engine.stage_planner import plan_query
@@
-        from slayer.core.keys import ArithmeticKey
         assert isinstance(fp.expression.value_key, ArithmeticKey)
@@
-        from slayer.engine.binding import walk_value_keys
         keys = list(walk_value_keys(fp.expression.value_key))

As per coding guidelines: "**/*.py: Place imports at the top of files".

Also applies to: 177-177

🤖 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_boundexpr_unification.py` at line 161, Move the function-local
imports to module scope: remove the in-test import of ArithmeticKey (from
slayer.core.keys) and any other imports currently performed inside test
functions and place them at the top of tests/test_boundexpr_unification.py;
update the module-level import section to include "from slayer.core.keys import
ArithmeticKey" (and any other symbols currently imported inside tests) so tests
reference those symbols without local imports.

tests/test_slack_normalization.py (2)

249-257: ⚡ Quick win

Avoid run_until_complete in pytest async suites.

The fixture manually drives the loop; this is fragile when a loop is already managed by pytest-asyncio. Prefer an async fixture and await the setup calls directly.

As per coding guidelines "**/tests/**/*.py: Tests must use pytest-asyncio with asyncio_mode = \"auto\" so test functions can be async def and await directly."

🤖 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_slack_normalization.py` around lines 249 - 257, The test uses
asyncio.get_event_loop().run_until_complete to call storage.save_datasource and
storage.save_model, which conflicts with pytest-asyncio-managed loops; change
the fixture to be async (make the fixture function async def) and replace those
run_until_complete calls with direct awaits: await
storage.save_datasource(DatasourceConfig(...)) and await
storage.save_model(_orders()), ensuring the fixture yields/returns as before so
pytest-asyncio (asyncio_mode="auto") manages the event loop correctly.

---

225-229: ⚡ Quick win

Move test imports to module scope.

These in-function imports make the test module inconsistent with repo style and harder to scan. Please hoist them to the top-level import block.

As per coding guidelines "**/*.py: Place imports at the top of files."

Also applies to: 249-257, 263-271, 287-295, 314-318, 343-351

🤖 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_slack_normalization.py` around lines 225 - 229, Several tests in
test_slack_normalization.py contain in-function imports (e.g., DatasourceConfig,
SlayerQueryEngine, YAMLStorage, sqlite3) — hoist these imports to the
module-level import block at the top of the file, remove the now-redundant local
imports inside the test functions, and consolidate/organize them with the
existing top imports; repeat this change for the other in-function import sites
flagged (around the commented ranges) to comply with the project import style
and keep test-scoped symbols available to all functions.

tests/test_dot_path_in_sql.py (1)

474-474: ⚡ Quick win

Hoist local imports to the module import section.

These test-local imports should be moved to top-level to match project conventions.

As per coding guidelines "**/*.py: Place imports at the top of files."

Also applies to: 489-490

🤖 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_dot_path_in_sql.py` at line 474, The test contains local imports
(e.g., "from slayer.core.models import ModelMeasure" and the other imports
around the later test block) that should be hoisted to the module-level import
section; move these local imports to the top of the file with the other imports,
remove the in-function/local import statements, and run the tests to ensure no
circular import issues—if a circular dependency appears, refactor the import to
a lazy import helper or import only the needed attribute at top-level.

🤖 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/binding.py`:
- Around line 155-156: The call to _bind uses a positional first argument;
change it to use keyword arguments for all parameters (e.g.,
_bind(parsed=parsed, scope=scope, bundle=bundle, in_filter=False)) and update
every other _bind invocation in this module (including the ones around the other
mentioned call sites) to the same keyword style so it conforms to the repo rule;
keep the surrounding return (BoundExpr(value_key=...)) unchanged.

In `@slayer/engine/syntax.py`:
- Around line 180-188: The current check uses _OVER_RE.search(text) and flags
any "OVER(" even when inside string literals (e.g. "status == 'OVER('"); change
the check to ignore matches inside quotes by scanning text
character-by-character tracking single-quote, double-quote, and escape state and
only applying _OVER_RE when not inside a quoted string, then raise
IllegalWindowInFilterError(filter_expr=text, source=..., suggestion=...) as
before; update the code path that currently calls _OVER_RE.search(text) (the
block raising IllegalWindowInFilterError) so it uses the new quote-aware
detection routine on the variable text.
- Around line 409-412: The code silently ignores keyword unpacking (**kwargs)
because the comprehension in _convert_call filters out ast.keyword entries with
arg is None; update _convert_call to detect any kw in node.keywords with kw.arg
is None and raise a clear error (e.g., ValueError or a parsing-specific
exception) rejecting **kwargs in Mode-B call parsing instead of dropping them,
then proceed to build kwargs by converting remaining keywords via _convert;
reference node.keywords, _convert_call, and _convert when implementing the check
and error raise.

In `@tests/test_agg_registry.py`:
- Around line 62-64: Update the test calls in tests/test_agg_registry.py to use
keyword arguments instead of positional arguments for helper functions with more
than one parameter: change calls to collect_reachable_agg_names(...),
resolve_aggregation(...), merge_agg_params(...), and any other multi-parameter
helper invocations (including the occurrences at lines noted in the comment) so
that each argument is passed as param_name=value (e.g., m=...,
resolve_join_target=..., named_queries=...) rather than by position; ensure you
update all instances mentioned (around the ranges 62-64, 79-81, 99-101, 117,
177, 183, 230, 238, 247, 257, 264) and run tests to confirm no signature
mismatches.

In `@tests/test_binding.py`:
- Around line 145-147: Update all calls to bind_expr and bind_filter (and
similar multi-parameter helpers) to pass the parsed expression/result as a
keyword instead of a positional first argument; e.g., replace
bind_expr(parse_expr("amount"), scope=_scope(), bundle=_bundle()) with a keyword
form like bind_expr(parsed=parse_expr("amount"), scope=_scope(),
bundle=_bundle()) (and do the same for bind_filter), applying this change at
every location listed in the comment so all multi-parameter calls use keyword
arguments for clarity and to follow the repository standard.

In `@tests/test_stage_planner.py`:
- Around line 203-220: The test test_stages_in_dependency_order currently
constructs stage1 and stage2 but passes them to plan_stages in dependency order,
so it doesn't verify reordering; update the test to pass stages in reverse order
(pass stage2 then stage1) to force the planner to reorder, and add an assertion
that the planned result is topologically correct (e.g., check planned[0].name ==
"stage1" and planned[1].name == "stage2" or equivalent checks on the planned
stages returned by plan_stages) so the test fails if plan_stages does not
reorder dependencies; references: test_stages_in_dependency_order, plan_stages,
SlayerQuery, planned.

---

Nitpick comments:
In `@slayer/engine/path_resolution.py`:
- Line 75: The local variable `nq` is annotated too broadly as `Any`; change its
annotation to a more specific mapping type (e.g., `dict[str, Any]`) so callers
and linters know it holds a dict produced from `named_queries`. Update the
assignment `nq: Any = named_queries if named_queries is not None else {}` to use
`nq: dict[str, Any] = ...` (or use `cast(dict[str, Any], named_queries)` if
necessary) and ensure `typing` imports include `Any`/`dict` typing forms
compatible with the project's Python version.

In `@tests/test_boundexpr_unification.py`:
- Line 161: Move the function-local imports to module scope: remove the in-test
import of ArithmeticKey (from slayer.core.keys) and any other imports currently
performed inside test functions and place them at the top of
tests/test_boundexpr_unification.py; update the module-level import section to
include "from slayer.core.keys import ArithmeticKey" (and any other symbols
currently imported inside tests) so tests reference those symbols without local
imports.

In `@tests/test_dot_path_in_sql.py`:
- Line 474: The test contains local imports (e.g., "from slayer.core.models
import ModelMeasure" and the other imports around the later test block) that
should be hoisted to the module-level import section; move these local imports
to the top of the file with the other imports, remove the in-function/local
import statements, and run the tests to ensure no circular import issues—if a
circular dependency appears, refactor the import to a lazy import helper or
import only the needed attribute at top-level.

In `@tests/test_path_resolution.py`:
- Around line 27-34: The test helper _make_model accepts parameters (name,
joins=None) but several test calls use positional args; update all usages to use
keyword arguments per guideline: call _make_model(name=..., joins=...) instead
of positional calls (e.g., replace _make_model(model_name) with
_make_model(name=model_name) and any two-argument positional calls with explicit
joins=...), locating usages by the function name _make_model in the tests and
editing each call.

In `@tests/test_slack_normalization.py`:
- Around line 249-257: The test uses asyncio.get_event_loop().run_until_complete
to call storage.save_datasource and storage.save_model, which conflicts with
pytest-asyncio-managed loops; change the fixture to be async (make the fixture
function async def) and replace those run_until_complete calls with direct
awaits: await storage.save_datasource(DatasourceConfig(...)) and await
storage.save_model(_orders()), ensuring the fixture yields/returns as before so
pytest-asyncio (asyncio_mode="auto") manages the event loop correctly.
- Around line 225-229: Several tests in test_slack_normalization.py contain
in-function imports (e.g., DatasourceConfig, SlayerQueryEngine, YAMLStorage,
sqlite3) — hoist these imports to the module-level import block at the top of
the file, remove the now-redundant local imports inside the test functions, and
consolidate/organize them with the existing top imports; repeat this change for
the other in-function import sites flagged (around the commented ranges) to
comply with the project import style and keep test-scoped symbols available to
all functions.

In `@tests/test_source_bundle.py`:
- Around line 20-29: The test helper _model(name: str, ds: str = "prod") is
being invoked with positional arguments in multiple places; update all calls to
use keyword arguments (e.g., _model(name="orders") or _model(name="orders",
ds="warehouse")) so they follow the guideline for functions with more than one
parameter; search for usages of _model in tests/test_source_bundle.py and
replace positional invocations with keyword form, leaving the SlayerModel
construction (and related Column/DataType references) unchanged.

In `@tests/test_time_trunc_key.py`:
- Around line 109-112: The test test_is_frozen should assert the specific
exception type instead of pytest.raises(Exception): replace the broad Exception
with the concrete exception thrown when mutating a frozen TimeTruncKey (use
pydantic.errors.ValidationError / pydantic.ValidationError if TimeTruncKey is a
Pydantic model, or dataclasses.FrozenInstanceError if it is a frozen dataclass),
update the test import(s) accordingly, and keep the mutating line k.granularity
= "day" and references to TimeTruncKey and ColumnKey unchanged.

🪄 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: 2ce23cea-7e91-49fa-a808-715b179c12a0

📥 Commits

Reviewing files that changed from the base of the PR and between a8b8f2c and b65fab8.

📒 Files selected for processing (43)

• slayer/core/errors.py
• slayer/core/keys.py
• slayer/core/scope.py
• slayer/core/warnings.py
• slayer/engine/agg_registry.py
• slayer/engine/binding.py
• slayer/engine/cross_model_planner.py
• slayer/engine/measure_expansion.py
• slayer/engine/normalization.py
• slayer/engine/path_resolution.py
• slayer/engine/planned.py
• slayer/engine/planning.py
• slayer/engine/query_engine.py
• slayer/engine/source_bundle.py
• slayer/engine/stage_planner.py
• slayer/engine/syntax.py
• slayer/engine/variables.py
• slayer/sql/sql_expr.py
• tests/test_agg_registry.py
• tests/test_binding.py
• tests/test_boundexpr_unification.py
• tests/test_cross_model_planner.py
• tests/test_cross_model_planner_wiring.py
• tests/test_dot_path_in_sql.py
• tests/test_error_messages.py
• tests/test_keys.py
• tests/test_model_measure_expansion.py
• tests/test_path_resolution.py
• tests/test_planned.py
• tests/test_projection_planner.py
• tests/test_scope_schema.py
• tests/test_slack_normalization.py
• tests/test_source_bundle.py
• tests/test_sql_expr.py
• tests/test_stage_planner.py
• tests/test_syntax.py
• tests/test_time_dimensions_filters.py
• tests/test_time_dimensions_planner.py
• tests/test_time_trunc_key.py
• tests/test_transform_lowerer.py
• tests/test_transforms_planner.py
• tests/test_value_registry.py
• tests/test_variables_planner.py

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

slayer/engine/syntax.py (1)

189-191: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The OVER( guard still misclassifies some valid Python strings.

_STRING_LITERAL_RE understands doubled-quote SQL literals, not Python backslash escapes, so inputs like status == "x\" OVER(" still leave OVER( visible and raise IllegalWindowInFilterError even though it is inside the string literal. This needs a quote-aware scanner/tokenizer that follows Python string rules instead of _STRING_LITERAL_RE.sub("", text).

#!/bin/bash
set -euo pipefail

python - <<'PY'
import re

OVER_RE = re.compile(r"\bOVER\s*\(", re.IGNORECASE)
STRING_LITERAL_RE = re.compile(r"'(?:[^']|'')*'|\"(?:[^\"]|\"\")*\"")

cases = [
    r'status == "OVER("',
    r'status == "x\" OVER("',
]

for text in cases:
    masked = STRING_LITERAL_RE.sub("", text)
    print(f"text   : {text}")
    print(f"masked : {masked}")
    print(f"match? : {bool(OVER_RE.search(masked))}")
    print("-" * 40)
PY

🤖 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/syntax.py` around lines 189 - 191, The current guard uses
_STRING_LITERAL_RE to mask literals but fails for Python-style escapes; replace
that masking with a Python-aware tokenizer: use the tokenize module
(tokenize.generate_tokens / tokenize.tokenize) on the input text to produce a
new string where tokens of type STRING are replaced with equal-length whitespace
(so positions remain consistent), then run _OVER_RE.search on that
tokenized/masked string instead of _STRING_LITERAL_RE.sub("", text); update the
conditional using the new helper (e.g., _mask_python_strings(text) and call it
where _STRING_LITERAL_RE.sub(...) is used) and ensure imports for tokenize and
io are added and IllegalWindowInFilterError behavior remains unchanged.

slayer/engine/stage_planner.py (1)

368-374: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Flatten dotted ORDER refs before rebinding in stage scope.

This fallback still parses customers.region verbatim when scope is a StageSchema, but downstream stage columns are emitted as customers__region. Structured ORDER refs without raw_formula will still fail or rebind to the wrong slot in multi-stage queries.

Suggested fix

         else:
             bo = bind_expr(
-                parse_expr(full_name, allow_dunder=flat_scope),
+                parse_expr(
+                    _flatten_dotted(full_name) if flat_scope else full_name,
+                    allow_dunder=flat_scope,
+                ),
                 scope=scope, bundle=bundle,
             )

🤖 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 368 - 374, The bind is parsing
the dotted ORDER ref verbatim (parse_expr(full_name,...)) so when scope is a
StageSchema downstream columns are emitted flattened (customers__region) and the
current code rebinds incorrectly; fix by detecting StageSchema (or stage scope)
and transform dotted full_name into the stage-flattened form before calling
parse_expr/bind_expr (e.g. replace dots with the stage separator used by the
StageSchema) so bind_expr receives the flattened identifier (use the same
flattening logic/utility the StageSchema uses) while preserving
allow_dunder=flat_scope and passing scope and bundle to bind_expr.

🤖 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/schema_drift.py`:
- Around line 866-870: The comprehension that builds custom_agg_names scans all
models via graph.models_by_name but should only consider models reachable from
the stage source; change the comprehension to iterate over graph.reachable and
then look up models in graph.models_by_name (e.g. for name in graph.reachable
for m in (graph.models_by_name.get(name),) if present) so only reachable models'
aggregations (m.aggregations) contribute to custom_agg_names; update the
expression that defines custom_agg_names accordingly (refer to the existing
variable name custom_agg_names and usages in schema_drift.py).

---

Duplicate comments:
In `@slayer/engine/stage_planner.py`:
- Around line 368-374: The bind is parsing the dotted ORDER ref verbatim
(parse_expr(full_name,...)) so when scope is a StageSchema downstream columns
are emitted flattened (customers__region) and the current code rebinds
incorrectly; fix by detecting StageSchema (or stage scope) and transform dotted
full_name into the stage-flattened form before calling parse_expr/bind_expr
(e.g. replace dots with the stage separator used by the StageSchema) so
bind_expr receives the flattened identifier (use the same flattening
logic/utility the StageSchema uses) while preserving allow_dunder=flat_scope and
passing scope and bundle to bind_expr.

In `@slayer/engine/syntax.py`:
- Around line 189-191: The current guard uses _STRING_LITERAL_RE to mask
literals but fails for Python-style escapes; replace that masking with a
Python-aware tokenizer: use the tokenize module (tokenize.generate_tokens /
tokenize.tokenize) on the input text to produce a new string where tokens of
type STRING are replaced with equal-length whitespace (so positions remain
consistent), then run _OVER_RE.search on that tokenized/masked string instead of
_STRING_LITERAL_RE.sub("", text); update the conditional using the new helper
(e.g., _mask_python_strings(text) and call it where _STRING_LITERAL_RE.sub(...)
is used) and ensure imports for tokenize and io are added and
IllegalWindowInFilterError behavior remains unchanged.

🪄 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: 3ff7eb9f-2ec5-4535-92cf-1e1db5ba5448

📥 Commits

Reviewing files that changed from the base of the PR and between 7a2824a and 2d9729e.

📒 Files selected for processing (22)

• docs/architecture/errors-and-warnings.md
• slayer/core/keys.py
• slayer/core/refs.py
• slayer/engine/binding.py
• slayer/engine/cross_model_planner.py
• slayer/engine/normalization.py
• slayer/engine/planning.py
• slayer/engine/query_engine.py
• slayer/engine/response_meta.py
• slayer/engine/schema_drift.py
• slayer/engine/stage_planner.py
• slayer/engine/syntax.py
• slayer/memories/resolver.py
• slayer/sql/generator.py
• tests/test_agg_registry.py
• tests/test_dev1445_cross_model_alias_filter.py
• tests/test_dev1450fix_cross_model_derived.py
• tests/test_dev1450fix_derived_time_dimension.py
• tests/test_dev1450fix_group2_correctness.py
• tests/test_generator2_multistage.py
• tests/test_path_resolution.py
• tests/test_stage_planner.py

💤 Files with no reviewable changes (1)

• slayer/engine/planning.py

[coderabbitai]

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/syntax.py`:
- Around line 143-147: Replace the non-escape-aware _STRING_LITERAL_RE with the
new escape-aware _PY_STRING_LITERAL_RE in both _normalize_sql_filter_operators
and _preprocess_colons so rewrites (colon preprocessing and SQL operator
normalization in functions _preprocess_colons and
_normalize_sql_filter_operators) skip over Python-style string literals that
contain backslash-escaped quotes; update any local variable or parameter
references in those functions to use _PY_STRING_LITERAL_RE and run existing
literal-masking logic unchanged so behavior remains identical except now
honoring escaped quotes.

🪄 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: 3c882c1d-4806-4406-9510-0f60db0e0ca6

📥 Commits

Reviewing files that changed from the base of the PR and between 2d9729e and 9e7e59e.

📒 Files selected for processing (6)

• slayer/engine/cross_model_planner.py
• slayer/engine/schema_drift.py
• slayer/engine/syntax.py
• slayer/sql/generator.py
• tests/test_dev1450fix_cross_model_derived.py
• tests/test_dev1450fix_group2_correctness.py

[sonarqubecloud]

Quality Gate passed

Issues
63 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.5% Duplication on New Code

See analysis details on SonarQube Cloud