DEV-1484 Stage C: migrate pre-existing tests off legacy enrichment

[ZmeiGorynych]

Summary

Stage C of DEV-1452 (delete the legacy enrichment pipeline): migrate every
pre-existing test file off slayer.engine.enrichment / slayer.engine.enriched
/ MixedArithmeticField so Stage D (DEV-1485) can delete the legacy modules.
Test-only changes — no production behaviour changed in Stage C.

The acceptance grep over tests/ is clean except test_agg_render_spec_shim.py
(the shim that dies in Stage D). This branch also merges in the latest
egor/dev-1450-principled-redesign-of-syntax (PR #144 / Stage A+B + Stage B
rounds 6–8); the merge was conflict-free.

• Migrated test_sql_generator.py, test_reference_semantics.py,
  test_formula.py, test_projection_trim.py, test_cross_model_derived_columns.py,
  test_query_backed_models.py, test_named_measures.py, test_help.py,
  test_aggregation_gating.py, test_filter_renamed_measure.py,
  test_nested_dag_cross_stage_refs.py off the legacy stack — engine._enrich
  / enrich_query / raw EnrichedQuery sites now run through
  engine.execute(dry_run=True) or AggRenderSpec + the dialect helpers, with
  SQL-based assertions.
• Deleted legacy-only test classes (parse_formula / named-measure expansion /
  extract-filter / placeholder / TestAutoMoveDimensions / stage-origin
  resolver) after auditing coverage; backfilled the unique cases additively
  into test_value_registry.py, test_syntax.py, test_measure_expansion.py,
  test_agg_render_spec.py, and test_slack_normalization.py.

Deferred typed-pipeline divergences (strict-xfail + tracking issue)

Genuine behaviour gaps the migration surfaced — each pinned by a
@pytest.mark.xfail(strict=True) test so it auto-promotes when fixed:

• DEV-1502 — measure-source Column.sql __-path alias doesn't trigger join inference.
• DEV-1494 — column-level Column.filter cross-table ref doesn't trigger join discovery (derived + direct-dotted-ref flavours).
• DEV-1503 — typed pipeline lacks isolated-CTE handling for cross-model-FILTERED local measures (legacy _fm_ feature). Stage-D blocker — 13 tracking tests.

(Earlier Stage A/B sessions raised DEV-1492 / 1495 / 1496 / 1497 / 1499 / 1500 / 1501 the same way.)

Test plan

• Acceptance grep clean over tests/ (only test_agg_render_spec_shim.py remains)
• poetry run pytest -m "not integration" — 3902 passed, 6 skipped, 42 xfailed, 0 failed
• poetry run ruff check slayer/ tests/ — clean
• Integration smoke (SQLite + DuckDB) — 133 passed

🤖 Generated with Claude Code

Summary by CodeRabbit

• Documentation

  • Expanded DSL and SQL-generation docs: lowercase like allowed; like(value, pattern) → SQL LIKE (NOT LIKE via not(...)); || → concat(...); detailed notes on CTEs, predicate join-paths, and filtered-local aggregate isolation.

• New Features

  • DSL recognizes 2-arg like(...) and concat via ||; SQL generator emits explicit LIKE/concat; planner supports filtered-local aggregate isolation to avoid join-intersection issues.

• Bug Fixes

  • change_pct divide-by-zero guarded; validation errors now map to client validation errors; improved join-path/derived-column handling to prevent incorrect SQL.

• Tests

  • Large suite updates and many new regression/integration tests covering DSL, SQL emission, planner isolation, join discovery, and normalization.

[linear]

DEV-1484

DEV-1452

[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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6f19329c-9a42-43b5-9318-2c720bf41317

📥 Commits

Reviewing files that changed from the base of the PR and between e381429 and 1c69c78.

📒 Files selected for processing (4)

• docs/architecture/sql-generation.md
• slayer/sql/generator.py
• tests/integration/test_integration.py
• tests/test_sql_generator.py

---

📝 Walkthrough

Walkthrough

Adds Mode‑B like and ||→concat parsing and binder arity checks, records column-filter join-paths on SqlExprKey, implements filtered-local isolation (DEV‑1503) in cross-model planning, extends derived-ref expansion and join-threading in SQL generation, adjusts Aggregate canonical naming and change_pct lowering, updates several error classes to inherit ValueError, and migrates many tests to engine-driven SQL helpers.

Changes

DSL scalar functions and operator support

Layer / File(s)	Summary
**Documentation for like and `
Parser and binder support slayer/engine/syntax.py, slayer/engine/binding.py, slayer/core/keys.py	Mode‑B preprocessing rewrites `
SQL generation for like slayer/sql/generator.py	Typed SQL rendering now emits exp.Like(...) for ScalarCallKey(name="like") across aggregate composite, post-filter WHERE/HAVING, and value-key rendering paths.

Error hierarchy and planning guard

Layer / File(s)	Summary
Binding-time errors inherit ValueError slayer/core/errors.py, tests/test_errors_hierarchy.py	Seven validation error classes now multi-inherit from ValueError alongside SlayerError; unit tests assert the inheritance contract.
Guard change_pct divisor against zero slayer/engine/planning.py, tests/test_transform_lowerer.py	desugar_change_pct now divides by nullif(shifted, 0) (ScalarCallKey nullif) to avoid divide-by-zero; tests assert nullif shape and reuse of the same time_shift instance.

Derived-column SQL qualification and cross-model path support

Layer / File(s)	Summary
Derived-column expansion & predicate inlining slayer/sql/generator.py, slayer/engine/column_expansion.py	Adds detection of derived refs in predicates (_predicate_references_derived), _render_mode_a_predicate inline-expansion via expand_derived_refs_sync, _expand_degenerate_derived_root, and collect_root_scope_joined_paths to discover join-path prefixes.
Column-filter join-path discovery (planner) slayer/engine/column_filter_paths.py, slayer/engine/binding.py	Planner-side compute_column_filter_join_paths parses/expands filter SQL across dialect fallbacks, walks root-scope <alias>.<col> refs, and returns ordered join-path prefixes; binder populates SqlExprKey.referenced_join_paths using it.
Cross-model CTE join-threading and filtered-local isolation slayer/sql/generator.py, slayer/engine/cross_model_planner.py, slayer/engine/planned.py, slayer/engine/stage_planner.py	Generator threads filter-driven join paths into CTE FROMs and ranked subqueries; planner implements DEV‑1503 filtered-local isolation by rerooting host-rooted sub-plans and returning CrossModelAggregatePlan.cte_root_model to signal host-rooted CTE rendering; plan_query gains disable_dev1503_isolation to suppress recursion.
Aggregate canonicalization slayer/engine/planning.py	Hidden-slot canonical names for AggregateKey now include canonicalized args/kwargs (via canonical_agg_name and agg_kwarg canonical strings).

Normalization, reachable custom aggs, and resolver

Layer / File(s)	Summary
Model normalization with custom_agg_names slayer/engine/normalization.py, slayer/engine/query_engine.py, slayer/engine/source_bundle.py, slayer/engine/schema_drift.py, slayer/memories/resolver.py	normalize_model accepts custom_agg_names; ResolvedSourceBundle.reachable_aggregation_names and query_engine/save_model use join-graph BFS to compute reachable aggregation names for FUNC_STYLE_AGG recognition; schema-drift and resolver paths updated accordingly.

SQL generator structural and rendering changes

Layer / File(s)	Summary
Derived-aware expansion and filter join merging slayer/sql/generator.py	Pre-expands derived ROW/TIME dimensions with is_root semantics, merges join paths from Mode‑A inline-expanded predicates and typed ValueKey trees, builds _expand_column_filter_sql and _render_mode_a_predicate helpers, and replaces some generic func emission with specific exp.Like rendering.
Nested-transform rejection message and handling slayer/sql/generator.py	Nested-transform rejections now raise ValueError with guidance text to precompute inner transforms in multi-stage models.

Tests, helpers, and migrations

Layer / File(s)	Summary
Shared engine-driven SQL helpers tests/_engine_helpers.py	Adds _assert_valid_sql, _engine_generate (ephemeral YAMLStorage + engine.execute dry_run), _outer_select, _where_text, _having_text for SQL-shape assertions.
Test migrations to engine dry-run SQL many tests/* (e.g., tests/test_cross_model_derived_columns.py, tests/test_filter_renamed_measure.py, tests/test_projection_trim.py, tests/test_reference_semantics.py, tests/test_projection_planner.py)	Multiple tests switched from enrichment/SQLGenerator internals to engine dry-run SQL assertions; added join-alias extraction, dialect-threading, and new regression tests for DEV‑1494/DEV‑1501/DEV‑1503.
New/refocused tests tests/test_filtered_local_isolation.py, tests/test_measure_expansion.py, tests/test_slack_normalization.py, tests/test_value_registry.py, tests/test_agg_render_spec.py, tests/test_entity_resolution.py, tests/test_source_bundle.py, tests/test_validate_models.py, tests/test_errors_hierarchy.py	Adds planner-focused DEV‑1503 coverage, typed measure-expansion negatives, slack-normalization coverage (DEV‑1484/DEV‑1500), TransformKey interning tests, agg-render-spec qualification test, entity extraction for joined custom aggs, reachable-aggregation-name unit tests, cascade-rule regression, and error-hierarchy assertions.

Sequence Diagram

sequenceDiagram
  participant Test as Test
  participant Engine as SlayerQueryEngine
  participant Binder as Binding
  participant Planner as StagePlanner/CrossModelPlanner
  participant Generator as SQLGenerator
  Test->>Engine: engine.execute(query, dry_run=True)
  Engine->>Binder: bind query, compute SqlExprKey (canonical_sql + referenced_join_paths)
  Binder->>Planner: plan_query(..., disable_dev1503_isolation=False)
  Planner->>Planner: detect filtered-local trigger -> _plan_filtered_local (DEV-1503)
  Planner->>Generator: build SQL (expand derived refs, collect filter join-paths)
  Generator->>Engine: return response.sql
  Engine->>Test: response.sql (validated)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• MotleyAI/slayer#158: Overlaps Mode‑B = rewrite/tokenizer logic and related syntax changes.
• MotleyAI/slayer#163: Overlaps DEV‑1503 filtered-local isolation and related planner/binder generator plumbing.
• MotleyAI/slayer#159: Related changes to aggregate canonical naming for parametric aggregates.

Suggested reviewers

• AivanF

Poem

"I hop where pipes become concat and LIKE,
I guard the shifted time from divide-by-strife.
I stitch join-paths into CTEs, neat and small,
Tests now ask the engine, and SQL shows all.
A carrot for the reviewer, and a happy hop for all."

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch egor/dev-1484-dev-1452-stage-c-migrate-pre-existing-tests-off-legacy

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

slayer/sql/generator.py (1)

3554-3600: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Propagate is_root=False to the other joined-ColumnSqlKey call sites.

This fixes base-row derived dimensions, but joined derived time dimensions and joined derived filter refs still call _expand_derived_column_sql() with the default True (_raw_time_col_expr_for_planned, _collect_filter_join_paths, _render_value_key_for_filter). A multi-hop derived ref on a joined model will still qualify to the bare child alias instead of the full __ path, so those queries keep generating invalid SQL outside the base-select path.

🛠️ Suggested follow-up

-                expanded_sql = self._expand_derived_column_sql(
-                    source_model=joined_model,
-                    source_relation="__".join(time_column.path),
-                    column_name=time_column.column_name,
-                    bundle=bundle,
-                )
+                expanded_sql = self._expand_derived_column_sql(
+                    source_model=joined_model,
+                    source_relation="__".join(time_column.path),
+                    column_name=time_column.column_name,
+                    bundle=bundle,
+                    is_root=False,
+                )

Apply the same change to the joined-model branches in _collect_filter_join_paths() and _render_value_key_for_filter().

As per coding guidelines, "Joined tables use __-delimited path aliases in SQL to disambiguate diamond joins."

Also applies to: 7315-7350

🤖 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 3554 - 3600, The joined-model call
sites that build derived-column SQL must pass the correct is_root flag into
_expand_derived_column_sql so multi-hop derived refs on joined models get full
__-delimited path aliases; update the joined-ColumnSqlKey branches in
_raw_time_col_expr_for_planned, _collect_filter_join_paths, and
_render_value_key_for_filter (the same pattern as the shown generator.py block)
to pass is_root=False (or is_root=not key.path where key is a ColumnSqlKey) when
key.path is non-empty so the expansion roots at the joined owner rather than the
base model.

🧹 Nitpick comments (4)

tests/test_transform_lowerer.py (1)

97-97: ⚡ Quick win

Pin canonical zero type in the denominator assertion.

At Line 97, assert zero == 0 also passes for non-canonical values and won’t catch normalization regressions. Assert Decimal(0) (or normalize_scalar(0)) explicitly.

Proposed test hardening

+from decimal import Decimal
+
 from slayer.core.keys import (
     AggregateKey,
     ArithmeticKey,
@@
-        assert zero == 0
+        assert zero == Decimal(0)

🤖 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_transform_lowerer.py` at line 97, The assertion currently checks
`zero == 0`, which can pass for non-canonical representations; update the test
to assert a canonical zero type by comparing `zero` to `Decimal(0)` (or to the
result of `normalize_scalar(0)` if that helper exists) so the test fails on
normalization regressions—replace the `assert zero == 0` with an assertion that
`zero` equals `Decimal(0)` (or `normalize_scalar(0)`), referencing the `zero`
variable and `normalize_scalar` helper as needed.

tests/test_errors_hierarchy.py (1)

51-51: ⚡ Quick win

Use keyword arguments in both pytest.mark.parametrize decorators.

Switch to argnames= and argvalues= to follow the repo-wide Python call style rule.

Proposed change

-@pytest.mark.parametrize("cls", _VALUE_ERROR_BINDING_ERRORS)
+@pytest.mark.parametrize(argnames="cls", argvalues=_VALUE_ERROR_BINDING_ERRORS)
 def test_binding_error_is_value_error(cls: type) -> None:
@@
-@pytest.mark.parametrize("cls", _VALUE_ERROR_BINDING_ERRORS)
+@pytest.mark.parametrize(argnames="cls", argvalues=_VALUE_ERROR_BINDING_ERRORS)
 def test_binding_error_is_slayer_error(cls: type) -> None:

As per coding guidelines: `**/*.py`: Use keyword arguments for functions with more than 1 parameter.

Also applies to: 59-59

🤖 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_errors_hierarchy.py` at line 51, Update the two
pytest.parametrized decorators to use keyword args: replace the positional call
pytest.mark.parametrize("cls", _VALUE_ERROR_BINDING_ERRORS) (and the other
instance at the same file) with pytest.mark.parametrize(argnames="cls",
argvalues=_VALUE_ERROR_BINDING_ERRORS) so the test decorators follow the repo
style for functions with multiple parameters; locate the decorators in
tests/test_errors_hierarchy.py by the pytest.mark.parametrize usage and change
to argnames= and argvalues= accordingly.

tests/test_aggregation_gating.py (1)

52-73: ⚡ Quick win

Use shared _engine_generate instead of duplicating engine setup.

_generate_sql duplicates the same storage/bootstrap/validate flow that now lives in tests/_engine_helpers.py. Switching to the shared helper will keep test harness behavior consistent and reduce future divergence.

🤖 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_aggregation_gating.py` around lines 52 - 73, The _generate_sql
function duplicates test engine setup/teardown; replace its body with a call to
the shared helper _engine_generate to reuse the centralized
storage/bootstrap/validation logic: import _engine_generate at the top of the
test module (if not already), change _generate_sql to delegate to await
_engine_generate(orders=orders, customers=customers, measures=measures) and
return the SQL string it yields, and remove the duplicated
YAMLStorage/SlugQueryEngine/bootstrap/assert logic (keep any local assertions
only if they’re uniquely required for this test).

tests/_engine_helpers.py (1)

57-64: 🏗️ Heavy lift

Make _engine_generate keyword-only for query and model.

This helper currently encourages positional calls with multiple arguments, which is drifting from the repo’s calling convention. Please make query and model keyword-only and update call sites accordingly.

Suggested change

 async def _engine_generate(
-    query: SlayerQuery,
-    model: SlayerModel,
-    *,
+    *,
+    query: SlayerQuery,
+    model: SlayerModel,
     dialect: str = "postgres",
     extra_models: Optional[list] = None,
     validate: bool = True,
 ) -> str:

As per coding guidelines, **/*.py: “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/_engine_helpers.py` around lines 57 - 64, Change the signature of
_engine_generate to make query and model keyword-only (e.g., async def
_engine_generate(*, query: SlayerQuery, model: SlayerModel, dialect: str =
"postgres", extra_models: Optional[list] = None, validate: bool = True) ) and
then update every call site to pass those two parameters by name (use query=...
and model=... instead of positional args); search for usages of _engine_generate
in tests/_engine_helpers.py and across the test suite to update invocations
accordingly so the function enforces keyword-only for query and model.

🤖 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 4436-4437: Routed filter expressions like like(...) are being
turned into string literals because _collect_routed_filters calls
_render_filter_value_key_in_target_scope which lacks a branch for ScalarCallKey;
add handling for ScalarCallKey in _render_filter_value_key_in_target_scope (and
the analogous places referenced) to detect a ScalarCallKey whose name == "like"
and return an exp.Like AST (or otherwise defer to the existing renderer that
emits exp.Like) instead of falling through to _literal_key_to_exp; update any
identical routines used for cross-model CTE routing so ScalarCallKey predicates
stay predicates rather than literals (refer to exp.Like,
_collect_routed_filters, _render_filter_value_key_in_target_scope,
ScalarCallKey, and _literal_key_to_exp).

In `@tests/test_agg_render_spec.py`:
- Around line 403-408: The call to _slot currently passes key positionally;
update it to use a keyword argument (e.g., key=key) so all parameters are passed
by name; modify the instantiation assigned to variable slot that calls
_slot(declared_name=..., public_name=..., slot_type=...) to include key=key to
follow the keyword-arg style for multi-parameter functions.

In `@tests/test_query_backed_models.py`:
- Around line 52-58: The helper function _expand_stage_as_model currently
accepts multiple positional parameters (engine, stage, *, name: str,
data_source: str) but is being called positionally; change its signature to
enforce keyword-only usage (e.g., make engine and stage positional but require
name and data_source as keyword-only or make all parameters keyword-only) and
update all call sites (including the calls referenced around the other
occurrences) to pass arguments by keyword (e.g., name=<...>, data_source=<...>)
so the function is invoked using keyword-based calls; ensure callers of
_expand_stage_as_model use the exact parameter names (name, data_source) in
their calls.

In `@tests/test_response_meta.py`:
- Line 4: The docstring in tests/test_response_meta.py contains a duplicated
word "legacy legacy" in the phrase "legacy legacy enriched-query path"; update
the docstring to remove the duplicate so it reads "legacy enriched-query path"
(search for the string "legacy legacy enriched-query path" and replace it with
"legacy enriched-query path" in that file).

---

Outside diff comments:
In `@slayer/sql/generator.py`:
- Around line 3554-3600: The joined-model call sites that build derived-column
SQL must pass the correct is_root flag into _expand_derived_column_sql so
multi-hop derived refs on joined models get full __-delimited path aliases;
update the joined-ColumnSqlKey branches in _raw_time_col_expr_for_planned,
_collect_filter_join_paths, and _render_value_key_for_filter (the same pattern
as the shown generator.py block) to pass is_root=False (or is_root=not key.path
where key is a ColumnSqlKey) when key.path is non-empty so the expansion roots
at the joined owner rather than the base model.

---

Nitpick comments:
In `@tests/_engine_helpers.py`:
- Around line 57-64: Change the signature of _engine_generate to make query and
model keyword-only (e.g., async def _engine_generate(*, query: SlayerQuery,
model: SlayerModel, dialect: str = "postgres", extra_models: Optional[list] =
None, validate: bool = True) ) and then update every call site to pass those two
parameters by name (use query=... and model=... instead of positional args);
search for usages of _engine_generate in tests/_engine_helpers.py and across the
test suite to update invocations accordingly so the function enforces
keyword-only for query and model.

In `@tests/test_aggregation_gating.py`:
- Around line 52-73: The _generate_sql function duplicates test engine
setup/teardown; replace its body with a call to the shared helper
_engine_generate to reuse the centralized storage/bootstrap/validation logic:
import _engine_generate at the top of the test module (if not already), change
_generate_sql to delegate to await _engine_generate(orders=orders,
customers=customers, measures=measures) and return the SQL string it yields, and
remove the duplicated YAMLStorage/SlugQueryEngine/bootstrap/assert logic (keep
any local assertions only if they’re uniquely required for this test).

In `@tests/test_errors_hierarchy.py`:
- Line 51: Update the two pytest.parametrized decorators to use keyword args:
replace the positional call pytest.mark.parametrize("cls",
_VALUE_ERROR_BINDING_ERRORS) (and the other instance at the same file) with
pytest.mark.parametrize(argnames="cls", argvalues=_VALUE_ERROR_BINDING_ERRORS)
so the test decorators follow the repo style for functions with multiple
parameters; locate the decorators in tests/test_errors_hierarchy.py by the
pytest.mark.parametrize usage and change to argnames= and argvalues=
accordingly.

In `@tests/test_transform_lowerer.py`:
- Line 97: The assertion currently checks `zero == 0`, which can pass for
non-canonical representations; update the test to assert a canonical zero type
by comparing `zero` to `Decimal(0)` (or to the result of `normalize_scalar(0)`
if that helper exists) so the test fails on normalization regressions—replace
the `assert zero == 0` with an assertion that `zero` equals `Decimal(0)` (or
`normalize_scalar(0)`), referencing the `zero` variable and `normalize_scalar`
helper as needed.

🪄 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: 64624493-4eca-4a6e-bead-2108082f137a

📥 Commits

Reviewing files that changed from the base of the PR and between 8b2239c and 0065024.

📒 Files selected for processing (34)

• CLAUDE.md
• docs/concepts/references.md
• slayer/core/errors.py
• slayer/core/keys.py
• slayer/engine/binding.py
• slayer/engine/planning.py
• slayer/engine/syntax.py
• slayer/sql/generator.py
• tests/_engine_helpers.py
• tests/integration/test_integration.py
• tests/test_agg_render_spec.py
• tests/test_aggregation_gating.py
• tests/test_cross_model_derived_columns.py
• tests/test_cross_model_rename_dev1448.py
• tests/test_errors_hierarchy.py
• tests/test_filter_renamed_measure.py
• tests/test_formula.py
• tests/test_generator2_multistage.py
• tests/test_help.py
• tests/test_measure_expansion.py
• tests/test_memories_resolver_typed.py
• tests/test_named_measures.py
• tests/test_nested_dag_cross_stage_refs.py
• tests/test_projection_trim.py
• tests/test_query_backed_models.py
• tests/test_query_backed_typed_expansion.py
• tests/test_reference_semantics.py
• tests/test_response_meta.py
• tests/test_schema_drift_typed.py
• tests/test_slack_normalization.py
• tests/test_sql_generator.py
• tests/test_syntax.py
• tests/test_transform_lowerer.py
• tests/test_value_registry.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Pass key as a keyword argument in _slot(...).

This new call should follow keyword-arg style for multi-parameter functions.

Suggested patch

-        slot = _slot(
-            key,
+        slot = _slot(
+            key=key,
             declared_name="active_revenue_sum",
             public_name="active_revenue_sum",
             slot_type=DataType.DOUBLE,
         )

As per coding guidelines **/*.py: Use keyword arguments for functions with more than 1 parameter.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	slot = _slot(
	key,
	declared_name="active_revenue_sum",
	public_name="active_revenue_sum",
	slot_type=DataType.DOUBLE,
	)
	slot = _slot(
	key=key,
	declared_name="active_revenue_sum",
	public_name="active_revenue_sum",
	slot_type=DataType.DOUBLE,
	)

🤖 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_agg_render_spec.py` around lines 403 - 408, The call to _slot
currently passes key positionally; update it to use a keyword argument (e.g.,
key=key) so all parameters are passed by name; modify the instantiation assigned
to variable slot that calls _slot(declared_name=..., public_name=...,
slot_type=...) to include key=key to follow the keyword-arg style for
multi-parameter functions.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use keyword-only parameters and keyword-based calls for the new helper.

The new helper and its callsites use positional arguments even though the function has multiple parameters.

Suggested patch

 async def _expand_stage_as_model(
-    engine: SlayerQueryEngine,
-    stage: SlayerQuery,
-    *,
+    *,
+    engine: SlayerQueryEngine,
+    stage: SlayerQuery,
     name: str,
     data_source: str = "ds",
 ) -> SlayerModel:
@@
-            virtual = await _expand_stage_as_model(engine, stage, name="qb_default")
+            virtual = await _expand_stage_as_model(
+                engine=engine,
+                stage=stage,
+                name="qb_default",
+            )
@@
-            virtual = await _expand_stage_as_model(engine, stage, name="qb_collide")
+            virtual = await _expand_stage_as_model(
+                engine=engine,
+                stage=stage,
+                name="qb_collide",
+            )
@@
-            virtual = await _expand_stage_as_model(engine, stage, name="qb_alias")
+            virtual = await _expand_stage_as_model(
+                engine=engine,
+                stage=stage,
+                name="qb_alias",
+            )

As per coding guidelines **/*.py: Use keyword arguments for functions with more than 1 parameter.

Also applies to: 1470-1470, 1489-1489, 1509-1509

🤖 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_query_backed_models.py` around lines 52 - 58, The helper function
_expand_stage_as_model currently accepts multiple positional parameters (engine,
stage, *, name: str, data_source: str) but is being called positionally; change
its signature to enforce keyword-only usage (e.g., make engine and stage
positional but require name and data_source as keyword-only or make all
parameters keyword-only) and update all call sites (including the calls
referenced around the other occurrences) to pass arguments by keyword (e.g.,
name=<...>, data_source=<...>) so the function is invoked using keyword-based
calls; ensure callers of _expand_stage_as_model use the exact parameter names
(name, data_source) in their calls.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix duplicated "legacy" in docstring.

The phrase "legacy legacy enriched-query path" appears to have a duplicated word.

📝 Proposed fix

-what the legacy legacy enriched-query path produced: result keys read straight from
+what the legacy enriched-query path produced: result keys read straight from

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	what the legacy legacy enriched-query path produced: result keys read straight from
	what the legacy enriched-query path produced: result keys read straight from

🤖 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_response_meta.py` at line 4, The docstring in
tests/test_response_meta.py contains a duplicated word "legacy legacy" in the
phrase "legacy legacy enriched-query path"; update the docstring to remove the
duplicate so it reads "legacy enriched-query path" (search for the string
"legacy legacy enriched-query path" and replace it with "legacy enriched-query
path" in that file).

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

slayer/sql/generator.py (3)

5404-5412: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Routed derived filters need to contribute to cte_join_paths.

_collect_routed_filters() can expand a routed target-local ColumnSqlKey into SQL that references deeper __ aliases, but those paths are never added to cte_join_paths before cte_where is attached. A routed filter like where is_eu = true on the target model will therefore generate a predicate against joins this _cm_* CTE never added.

🤖 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 5404 - 5412, _cte_where returned by
_collect_routed_filters may reference deeper join aliases but its join path(s)
are not merged into cte_join_paths, so routed filters produce predicates against
joins the _cm_* CTE never added; update the call site after cte_where is
computed so that when cte_where is not None you also extract and merge the
routed join path information returned by _collect_routed_filters (or call its
companion accessor) into cte_join_paths before appending cte_where, referencing
the symbols _collect_routed_filters, cte_where and cte_join_paths (and the
ColumnSqlKey/_cm_* path names) so the CTE includes the necessary join paths for
the generated predicates.

---

3525-3556: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Collect joins crossed by derived aggregate sources too.

This prepass now covers row dims, row filters, and Column.filter, but it still skips joins introduced by a local aggregate’s ColumnSqlKey source. _build_agg_render_spec_from_planned() expands that SQL later, so a measure like sum(net_amount) can still emit customers__... refs against a FROM that never joined customers unless some unrelated clause happens to pull the same path in.

🤖 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 3525 - 3556, The prepass misses joins
introduced by a local aggregate's ColumnSqlKey source so add a pass that
collects those join paths and appends them to needed_join_paths before
rendering; call a new or existing collector (e.g. implement and call
self._collect_agg_source_join_paths(planned_query=planned_query,
base_render_order=base_render_order, slots_by_id=slots_by_id,
source_relation=source_relation, source_model=source_model, bundle=bundle) or
similar) right alongside the other collectors shown, and for each returned path
do "if p not in needed_join_paths: needed_join_paths.append(p)"; ensure this
complements _build_agg_render_spec_from_planned() so aggregate ColumnSqlKey SQL
won't reference unjoined relations.

---

5361-5438: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The _cm_* join collector still misses the measure source SQL.

cte_join_paths only looks at the measure’s column_filter_key and text target-model filters. If the cross-model measure source itself is a target-local derived ColumnSqlKey, _build_agg_render_spec_from_planned() will expand it to __-qualified refs, but target_from is still built without those joins, so the aggregate body can reference aliases that never entered the CTE’s FROM.

🤖 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 5361 - 5438, cte_join_paths is not
being seeded with the measure's source ColumnSqlKey, so when
_build_agg_render_spec_from_planned expands a target-local derived measure the
CTE's FROM misses required joins; update the block that seeds cte_join_paths
(around local_agg_key and _add_cte_join_paths) to also detect the measure source
ColumnSqlKey on local_agg_key (the same source used by
_build_agg_render_spec_from_planned) and call _add_cte_join_paths with its
canonical_sql (e.g. local_agg_key.<source_column_key>.canonical_sql or whatever
field holds the measure source ColumnSqlKey) so the generated
target_from/cte_base_joins include those join 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.

Outside diff comments:
In `@slayer/sql/generator.py`:
- Around line 5404-5412: _cte_where returned by _collect_routed_filters may
reference deeper join aliases but its join path(s) are not merged into
cte_join_paths, so routed filters produce predicates against joins the _cm_* CTE
never added; update the call site after cte_where is computed so that when
cte_where is not None you also extract and merge the routed join path
information returned by _collect_routed_filters (or call its companion accessor)
into cte_join_paths before appending cte_where, referencing the symbols
_collect_routed_filters, cte_where and cte_join_paths (and the
ColumnSqlKey/_cm_* path names) so the CTE includes the necessary join paths for
the generated predicates.
- Around line 3525-3556: The prepass misses joins introduced by a local
aggregate's ColumnSqlKey source so add a pass that collects those join paths and
appends them to needed_join_paths before rendering; call a new or existing
collector (e.g. implement and call
self._collect_agg_source_join_paths(planned_query=planned_query,
base_render_order=base_render_order, slots_by_id=slots_by_id,
source_relation=source_relation, source_model=source_model, bundle=bundle) or
similar) right alongside the other collectors shown, and for each returned path
do "if p not in needed_join_paths: needed_join_paths.append(p)"; ensure this
complements _build_agg_render_spec_from_planned() so aggregate ColumnSqlKey SQL
won't reference unjoined relations.
- Around line 5361-5438: cte_join_paths is not being seeded with the measure's
source ColumnSqlKey, so when _build_agg_render_spec_from_planned expands a
target-local derived measure the CTE's FROM misses required joins; update the
block that seeds cte_join_paths (around local_agg_key and _add_cte_join_paths)
to also detect the measure source ColumnSqlKey on local_agg_key (the same source
used by _build_agg_render_spec_from_planned) and call _add_cte_join_paths with
its canonical_sql (e.g. local_agg_key.<source_column_key>.canonical_sql or
whatever field holds the measure source ColumnSqlKey) so the generated
target_from/cte_base_joins include those join paths.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 07aeef3a-a52b-4433-b671-9031b96395a8

📥 Commits

Reviewing files that changed from the base of the PR and between 0065024 and 74ed95a.

📒 Files selected for processing (5)

• docs/architecture/index.md
• docs/architecture/sql-generation.md
• slayer/sql/generator.py
• tests/test_cross_model_derived_columns.py
• tests/test_sql_generator.py

✅ Files skipped from review due to trivial changes (2)

• docs/architecture/index.md
• docs/architecture/sql-generation.md

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/test_validate_models.py (1)

453-453: ⚡ Quick win

Move Aggregation to the module import block.

This function-scoped import breaks the repo's import-placement rule and makes the test dependency easy to miss. As per coding guidelines, "Place imports at the top of files."

🤖 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_validate_models.py` at line 453, The test currently does a
function-scoped import of Aggregation (from slayer.core.models import
Aggregation); move that import into the module-level import block at the top of
tests/test_validate_models.py so all imports are grouped together. Locate the
current inline import of Aggregation, remove it from inside the function, and
add a single module-level import line for Aggregation alongside the other test
imports so the test follows the project's import-placement rule.

🤖 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/query_engine.py`:
- Around line 1591-1592: The early-return guard only checks model.measures and
thus skips query-backed models whose formulas live in
SlayerModel.source_queries; update the check in the function containing this
guard to treat a model as empty only if both model.measures and
model.source_queries are empty/None (i.e., proceed when either has content).
Ensure reachable custom-aggregation discovery logic considers source_queries the
same way it considers measures so joined-model funcstyle aggregations normalized
by save_model() are also discovered at execute time.

---

Nitpick comments:
In `@tests/test_validate_models.py`:
- Line 453: The test currently does a function-scoped import of Aggregation
(from slayer.core.models import Aggregation); move that import into the
module-level import block at the top of tests/test_validate_models.py so all
imports are grouped together. Locate the current inline import of Aggregation,
remove it from inside the function, and add a single module-level import line
for Aggregation alongside the other test imports so the test follows the
project's import-placement rule.

🪄 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: 26e96109-fda4-46e8-8006-1a3d9e83d2ff

📥 Commits

Reviewing files that changed from the base of the PR and between 74ed95a and 93bc7cd.

📒 Files selected for processing (10)

• slayer/engine/normalization.py
• slayer/engine/query_engine.py
• slayer/engine/schema_drift.py
• slayer/engine/source_bundle.py
• slayer/memories/resolver.py
• tests/test_entity_resolution.py
• tests/test_slack_normalization.py
• tests/test_source_bundle.py
• tests/test_sql_generator.py
• tests/test_validate_models.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Handle query-backed models here too.

Query-backed models keep their formulas under SlayerModel.source_queries, not model.measures, so this guard skips reachable custom-aggregation discovery for exactly the models that save_model() also normalizes. Joined-model funcstyle custom aggregations in stored stages will therefore stay unnormalized on save even though execute-time normalization now recognizes them.

🤖 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/query_engine.py` around lines 1591 - 1592, The early-return
guard only checks model.measures and thus skips query-backed models whose
formulas live in SlayerModel.source_queries; update the check in the function
containing this guard to treat a model as empty only if both model.measures and
model.source_queries are empty/None (i.e., proceed when either has content).
Ensure reachable custom-aggregation discovery logic considers source_queries the
same way it considers measures so joined-model funcstyle aggregations normalized
by save_model() are also discovered at execute time.

[coderabbitai]

Actionable comments posted: 3

🤖 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/column_filter_paths.py`:
- Around line 57-63: The dialect chain _PLANNER_PARSE_DIALECT_CHAIN is missing
the "clickhouse" dialect which causes compute_column_filter_join_paths (and
Column.filter parsing) to fail to detect ClickHouse-specific predicates; add
"clickhouse" into the tuple (e.g., include "clickhouse" among "postgres", None,
"mysql", "bigquery", "tsql") so SQLGlot will try ClickHouse parsing and preserve
ClickHouse-only join-path semantics. Ensure the ordering keeps permissive None
in place and run unit tests that exerciseClickHouse-specific filters to verify
parsing now returns expected join paths.

In `@slayer/engine/cross_model_planner.py`:
- Around line 518-528: The current code slices host_query.filters and assumes
alignment with host_filters, which breaks when stage_planner.plan_query()
dedupes filters; instead, stop slicing host_query.filters and build
sub_filter_texts from the host_filters list itself using each
HostFilterRouting's text field: iterate over host_filters (the list of
HostFilterRouting), skip routings whose phase is Phase.POST or Phase.AGGREGATE,
and append routing.text (or routing.text if named differently) to
sub_filter_texts only when that text is set; also ensure
stage_planner.plan_query()/stage planning code populates HostFilterRouting.text
for the deduped/surviving user-filter routings so cross_model_planner.consume
the same deduped texts.

In `@tests/integration/test_integration.py`:
- Around line 3539-3548: Test fixture's tie-break is weak because natural status
order could match the expected result even if the secondary ORDER BY is ignored;
update the "rows" fixture (variable rows in the failing test) so the natural
alphabetical order of status would be opposite the intended tie-break (e.g.,
swap the created_at/updated_at values so status "B" would come before "A" if no
secondary key is applied), or add an explicit dry-run assertion in the same test
that verifies two distinct sort keys were produced for the groups (i.e., assert
that the secondary sort key values differ for the tied groups) to make the
secondary-sort failure impossible to miss; adjust the rows data and/or add the
assertion near the rows definition used later in the test to exercise the
secondary ORDER BY.

🪄 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: c4fd0735-6d19-40f1-b9ce-511f7178e7f3

📥 Commits

Reviewing files that changed from the base of the PR and between 93bc7cd and e381429.

📒 Files selected for processing (21)

• CLAUDE.md
• docs/architecture/cross-model-aggregates.md
• docs/architecture/planning.md
• docs/architecture/sql-generation.md
• slayer/core/keys.py
• slayer/engine/binding.py
• slayer/engine/column_expansion.py
• slayer/engine/column_filter_paths.py
• slayer/engine/cross_model_planner.py
• slayer/engine/planned.py
• slayer/engine/planning.py
• slayer/engine/stage_planner.py
• slayer/engine/syntax.py
• slayer/sql/generator.py
• tests/integration/test_integration.py
• tests/test_filtered_local_isolation.py
• tests/test_keys.py
• tests/test_projection_planner.py
• tests/test_projection_trim.py
• tests/test_sql_generator.py
• tests/test_syntax.py

✅ Files skipped from review due to trivial changes (2)

• CLAUDE.md
• docs/architecture/sql-generation.md

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Does sqlglot 30.0.0 support a ClickHouse dialect named clickhouse, and does ClickHouse-specific SQL require selecting that dialect instead of relying on postgres, default, mysql, bigquery, or tsql parsing?

💡 Result:

Yes, SQLGlot 30.0.0 includes official, built-in support for a ClickHouse dialect [1][2][3]. To correctly parse or generate ClickHouse-specific SQL, you must specify clickhouse as the dialect (e.g., read="clickhouse" for parsing or dialect="clickhouse" for generation) [4][5]. Relying on other dialects like postgres, mysql, bigquery, or tsql will likely result in parsing errors or incorrect SQL generation because SQLGlot uses these specific dialect configurations to handle ClickHouse's unique features, such as aggregate combinators (e.g., countIf), specific join types, non-nullable types, and distinct identifier escaping rules [6]. If no dialect is specified, SQLGlot defaults to its own internal "SQLGlot dialect," which acts as a superset of supported dialects and may not correctly interpret or output valid syntax for ClickHouse-specific constructs [5]. Using the official clickhouse dialect ensures that the parser and generator are configured with the correct metadata, tokenization rules, and syntax handling required for ClickHouse [7][6].

Citations:

• 1: https://pypi.org/project/sqlglot/30.0.0/
• 2: https://pypi.org/project/sqlglot/30.0.3/
• 3: https://pypi.org/project/sqlglot/30.0.1/
• 4: ClickHouse: fail to parse "WITH <expression> AS <identifier>" tobymao/sqlglot#3175
• 5: https://github.com/tobymao/sqlglot/blob/main/README.md
• 6: https://deepwiki.com/tobymao/sqlglot/4.8-clickhouse-and-mysql-dialects
• 7: https://sqlglot.com/sqlglot/dialects/clickhouse.html

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the file around the reported lines
sed -n '1,200p' slayer/engine/column_filter_paths.py | cat -n

# Locate and inspect compute_column_filter_join_paths usage/definition
rg -n "compute_column_filter_join_paths" slayer/engine/column_filter_paths.py
rg -n "_PLANNER_PARSE_DIALECT_CHAIN" slayer/engine/column_filter_paths.py

# Print the function body if present
python3 - <<'PY'
import re, itertools, sys, pathlib
path = pathlib.Path("slayer/engine/column_filter_paths.py")
txt = path.read_text()
m = re.search(r"def\s+compute_column_filter_join_paths\s*\([^)]*\)\s*:", txt)
print("found_def:", bool(m))
PY

# If function exists, show a narrower excerpt around it
line=$(rg -n "def compute_column_filter_join_paths" slayer/engine/column_filter_paths.py | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
  start=$((line-40))
  end=$((line+160))
  sed -n "${start},${end}p" slayer/engine/column_filter_paths.py | cat -n
fi

Repository: MotleyAI/slayer

Length of output: 14511

---

Add ClickHouse to _PLANNER_PARSE_DIALECT_CHAIN

slayer/engine/column_filter_paths.py (lines 57-63) omits the clickhouse dialect even though the file explicitly calls out ClickHouse-specific Column.filter syntax; compute_column_filter_join_paths() uses this chain for dialect-aware parsing, so ClickHouse-only predicates that don’t parse under postgres/mysql/bigquery/tsql (or the permissive None default) drop to () and skip DEV-1503 join-path isolation/semantics on a supported ClickHouse backend. SQLGlot’s clickhouse dialect should be included to parse ClickHouse constructs correctly.

Suggested change

 _PLANNER_PARSE_DIALECT_CHAIN: Tuple[Optional[str], ...] = (
     "postgres",
     None,        # sqlglot's permissive default — accepts ANSI SQL broadly
     "mysql",
+    "clickhouse",
     "bigquery",
     "tsql",
 )

🤖 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/column_filter_paths.py` around lines 57 - 63, The dialect chain
_PLANNER_PARSE_DIALECT_CHAIN is missing the "clickhouse" dialect which causes
compute_column_filter_join_paths (and Column.filter parsing) to fail to detect
ClickHouse-specific predicates; add "clickhouse" into the tuple (e.g., include
"clickhouse" among "postgres", None, "mysql", "bigquery", "tsql") so SQLGlot
will try ClickHouse parsing and preserve ClickHouse-only join-path semantics.
Ensure the ordering keeps permissive None in place and run unit tests that
exerciseClickHouse-specific filters to verify parsing now returns expected join
paths.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't recover sub-plan filter text by slicing host_query.filters.

stage_planner.plan_query() dedupes user filters before building host_filters, so len(host_query.filters) is no longer guaranteed to match the routed entries here. With a date_range plus duplicate/alias-equivalent user filters, this slice can mis-pair phases and texts, which lets an AGGREGATE filter be propagated into the DEV-1503 sub-plan as if it were ROW-phase.

Suggested fix

-    user_query_filters = host_query.filters or []
-    if not user_query_filters:
+    user_routings = [
+        routing for routing in host_filters
+        if routing.text is not None
+    ]
+    if not user_routings:
         return None
-    user_routings = list(host_filters[-len(user_query_filters):])
     sub_filter_texts: List[str] = []
-    for routing, filt in zip(user_routings, user_query_filters):
+    for routing in user_routings:
         if routing.phase in (Phase.POST, Phase.AGGREGATE):
             continue
-        # ROW phase — propagate.
-        sub_filter_texts.append(filt)
+        sub_filter_texts.append(routing.text)
     return sub_filter_texts or None

And in slayer/engine/stage_planner.py, populate HostFilterRouting.text for the surviving user-filter routings so this function consumes the same deduped list the planner actually kept.

🤖 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/cross_model_planner.py` around lines 518 - 528, The current
code slices host_query.filters and assumes alignment with host_filters, which
breaks when stage_planner.plan_query() dedupes filters; instead, stop slicing
host_query.filters and build sub_filter_texts from the host_filters list itself
using each HostFilterRouting's text field: iterate over host_filters (the list
of HostFilterRouting), skip routings whose phase is Phase.POST or
Phase.AGGREGATE, and append routing.text (or routing.text if named differently)
to sub_filter_texts only when that text is set; also ensure
stage_planner.plan_query()/stage planning code populates HostFilterRouting.text
for the deduped/surviving user-filter routings so cross_model_planner.consume
the same deduped texts.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make the secondary sort failure impossible to miss.

This fixture can still pass if the second ORDER BY key is ignored, because tied groups may naturally come back as "A", then "B". That makes the DEV-1501 regression weakly covered here. Flip the data so the expected result contradicts status ordering, or add a dry-run assertion that two distinct sort keys were generated.

Suggested way to make the tie-break observable

-    # Per-group last(created_at) is tied at 50 across A and B; the
-    # secondary ORDER BY last(updated_at) ASC must break the tie with
-    # A (10) before B (99).
+    # Per-group last(created_at) is tied at 50 across A and B; the
+    # secondary ORDER BY last(updated_at) ASC must break the tie with
+    # B (10) before A (99), so the expected order now contradicts the
+    # natural status ordering.
     rows = [
-        # Status A: last(created_at)=50, last(updated_at)=10.
+        # Status A: last(created_at)=50, last(updated_at)=99.
         (1, "A", 50.0, "2025-03-01", "2025-01-01"),
-        (2, "A", 10.0, "2025-02-01", "2025-04-01"),
-        # Status B: last(created_at)=50 (tied), last(updated_at)=99.
+        (2, "A", 99.0, "2025-02-01", "2025-04-01"),
+        # Status B: last(created_at)=50 (tied), last(updated_at)=10.
         (3, "B", 50.0, "2025-03-01", "2025-02-01"),
-        (4, "B", 99.0, "2025-01-15", "2025-04-01"),
+        (4, "B", 10.0, "2025-01-15", "2025-04-01"),
     ]
@@
-    # Two rows, both count 2; A before B because primary lc is tied (50)
-    # and secondary lu ASC puts A (lu=10) before B (lu=99).
+    # Two rows, both count 2; B before A because primary lc is tied (50)
+    # and secondary lu ASC puts B (lu=10) before A (lu=99).
     assert len(response.data) == 2, response.data
-    assert response.data[0]["orders.status"] == "A", response.data
-    assert response.data[1]["orders.status"] == "B", response.data
+    assert response.data[0]["orders.status"] == "B", response.data
+    assert response.data[1]["orders.status"] == "A", response.data

Also applies to: 3590-3596

🤖 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/integration/test_integration.py` around lines 3539 - 3548, Test
fixture's tie-break is weak because natural status order could match the
expected result even if the secondary ORDER BY is ignored; update the "rows"
fixture (variable rows in the failing test) so the natural alphabetical order of
status would be opposite the intended tie-break (e.g., swap the
created_at/updated_at values so status "B" would come before "A" if no secondary
key is applied), or add an explicit dry-run assertion in the same test that
verifies two distinct sort keys were produced for the groups (i.e., assert that
the secondary sort key values differ for the tied groups) to make the
secondary-sort failure impossible to miss; adjust the rows data and/or add the
assertion near the rows definition used later in the test to exercise the
secondary ORDER BY.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud