DEV-1712 Stage 8: order-only hidden slots + plan-time validations (order-only refs, CMA trim, partition_by) - #274
Open
Open
Conversation
Implements the DEV-1703 Stage 8 slice: ORDER BY refs not declared as dimensions/measures, hidden cross-model-aggregate projection trim, and the rank-family partition_by grain guard. Order-only refs (Law 2), classified at plan time in stage_planner.plan_query: - aggregate (local or cross-model): materialised hidden, sorted on, stripped from the result / StageSchema (never rejected). - local row column: split emission `orders.<col>` in the raw-rows case; a grouped query raises ValueError (not in GROUP BY). - joined row column: UnresolvableOrderColumnError. - inline transform/composite (change(...) etc.): ValueError -> declare as a measure. Full support deferred to DEV-1733 (worktree + strict-xfail future tests). Composite arithmetic is unexpressible via OrderItem (Pydantic). The generator's old NotImplementedError becomes a defensive assertion. Hidden cross-model aggregate trim (DEV-1495 bug 2): an order-only CMA is hidden=True/public_alias=None and skips the combined projection, with a CTE-qualified ORDER BY term -- gated on `not transform_layers` so a hidden CMA feeding a cumsum step stays projected for the step CTE. The malformed `._sum` alias half was already fixed by Stage 9's naming module. partition_by grain guard (DEV-1497): a pre-intern pass (planning.rewrite_rank_partition_keys, mirroring lower_sugar_transforms) validates each rank-family partition_by key is a query dimension/time-dim by exact ValueKey membership, and rewrites a time-dim source column to its TimeTruncKey bucket (no more raw-timestamp grain widening / duplicate alias); a non-dimension raises the restored legacy message. DEV-1645 Flavor-A ORDER BY: ported main's lost legacy _OrderColRef / _order_split_sql / _resolve_order_column fix (split-not-composite + UnresolvableOrderColumnError) into the legacy generator so the 7 pinned unit tests + 1 integration pin promote. tests/parity_xfails.py is now empty (the DEV-1485 Stage 11 end-state). Deliberate typed-pipeline divergence: a grouped raw-row order is rejected at plan time (HTTP 400) rather than emitting SQL the database rejects at execution. Docs: queries.md ORDER BY semantics, formulas.md partition_by note, slayer-query skill, DECISIONS.md entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesOrder planning and SQL generation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Query
participant StagePlanner
participant SQLGenerator
participant SQLResult
Query->>StagePlanner: submit order and partition_by targets
StagePlanner->>StagePlanner: validate and classify targets
StagePlanner->>SQLGenerator: provide planned order references
SQLGenerator->>SQLResult: render aliases, qualified columns, and CTE references
SQLResult-->>Query: return ordered projection and rows
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
slayer/engine/planning.py (1)
405-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake
rewrite_fnkeyword-only.
rewrite_rank_partition_keystakes two parameters and both are passed positionally atslayer/engine/stage_planner.pyline 605. The coding guidelines require keyword arguments for functions with more than one parameter. A keyword-only callback also documents intent at every recursive call site.Also consider annotating the callback type so the contract described in the docstring is machine-checkable.
♻️ Proposed signature change
-def rewrite_rank_partition_keys(key: ValueKey, rewrite_fn) -> ValueKey: +def rewrite_rank_partition_keys( + key: ValueKey, *, rewrite_fn: Callable[[TransformKey], frozenset], +) -> ValueKey:Every recursive call and the
stage_planner._rwcall site then passrewrite_fn=....🤖 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/planning.py` around lines 405 - 431, Update rewrite_rank_partition_keys so rewrite_fn is a keyword-only parameter, and add a callable type annotation matching its TransformKey-to-frozenset contract. Change every recursive invocation and the stage_planner _rw call site to pass rewrite_fn by keyword, preserving existing behavior.Source: Coding guidelines
slayer/sql/generator.py (1)
6258-6273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute the trimmed-plan set once.
The trim predicate
plan.hidden and not planned_query.transform_layersappears twice: at line 6140 (trim_hidden, which drops the aggregate fromcombined_parts) and again at line 6266 (which decides whether the ORDER BY term is CTE-qualified). The two must always agree. If they diverge, the projection drops the column while the ORDER BY still names the bare alias, and the emitted SQL is invalid.Note also that this block runs after the
return self._render_cross_model_transform_chain(...)at line 6238, sonot planned_query.transform_layersis always true here.Build one set of trimmed plan ids in the projection loop and reuse it.
♻️ Proposed refactor
+ trimmed_cma_plan_ids: Set[str] = set() for plan in planned_query.cross_model_aggregate_plans: ... trim_hidden = plan.hidden and not planned_query.transform_layers + if trim_hidden: + trimmed_cma_plan_ids.add(plan.aggregate_slot_id)hidden_cma_order_ref: Dict[str, str] = {} for plan in planned_query.cross_model_aggregate_plans: - # Only CMAs actually trimmed from the projection (hidden + no - # transform chain) need the CTE-qualified ORDER BY reference. - if not (plan.hidden and not planned_query.transform_layers): + # Only CMAs actually trimmed from the projection need the + # CTE-qualified ORDER BY reference. + if plan.aggregate_slot_id not in trimmed_cma_plan_ids: continue🤖 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 6258 - 6273, Compute a set of trimmed cross-model aggregate plan IDs in the projection loop using the existing trim predicate, then reuse that set when building hidden_cma_order_ref instead of repeating the predicate. Since this block runs after the transform-chain return, retain the projection loop’s established conditions and use the shared IDs to CTE-qualify exactly the aggregates removed from combined_parts.tests/test_dev1712_order_only_hidden_slots.py (1)
298-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error text for the joined-column rejection.
Both tests assert only the exception type.
UnresolvableOrderColumnErrorembeds the column and the qualifier in its message, and the planner builds those two fields atslayer/engine/stage_planner.pyline 677. A type-only assertion cannot detect a malformed message there — see the separate comment on that line, where the qualifier is currently duplicated into the rendered text.Add a message assertion so the user-facing text is pinned.
💚 Proposed test tightening
- with pytest.raises(UnresolvableOrderColumnError): - await _sql(engine, query) + with pytest.raises(UnresolvableOrderColumnError) as ei: + await _sql(engine, query) + msg = str(ei.value) + assert "'customers.region'" in msg, f"malformed qualifier/column: {msg}"Apply the same change to
test_joined_row_column_grouped_raises.🤖 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_dev1712_order_only_hidden_slots.py` around lines 298 - 317, Update both test_joined_row_column_ungrouped_raises and test_joined_row_column_grouped_raises to assert the exact user-facing message of UnresolvableOrderColumnError, including the joined column and qualifier, while preserving the existing exception-type assertions.
🤖 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/stage_planner.py`:
- Around line 673-677: Update the UnresolvableOrderColumnError construction in
the shown partition-key branch to pass the bare leaf column as column and the
full joined row-key path as qualifier, avoiding duplicated qualifiers in the
formatted message. Prefer deriving both values directly from the ColumnKey,
ColumnSqlKey, and TimeTruncKey structure via a small helper adjacent to
_row_key_path rather than splitting _partition_key_display output.
- Around line 580-594: Update the time-dimension lookup near `_td_by_source` to
detect source columns mapped to multiple `TimeTruncKey` granularities, storing
those columns in `_td_ambiguous_sources` while preserving unambiguous mappings.
In `_validate_partition_keys`, check ambiguous source columns before resolving
through `_td_by_source` and raise an error naming the column and competing
granularities, directing the user to specify the qualified time bucket.
In `@slayer/sql/generator.py`:
- Around line 11018-11045: Update the hidden local ColumnSqlKey handling in the
split-emission path so derived SQL expressions are expanded and their join paths
registered before generating the order expression. Ensure local expressions such
as customers.region bind the required relation instead of producing an unbound
ORDER BY reference, while preserving existing ColumnKey and TimeTruncKey
behavior. Add an ungrouped order-only regression test covering this case.
In `@tests/test_dev1712_order_only_hidden_slots.py`:
- Around line 691-697: Update
test_composite_order_string_rejected_at_construction to expect
pydantic.ValidationError instead of the broad Exception type, and add the
ValidationError import with the module’s existing top-level imports. Keep the
test’s current construction and boundary assertion unchanged.
---
Nitpick comments:
In `@slayer/engine/planning.py`:
- Around line 405-431: Update rewrite_rank_partition_keys so rewrite_fn is a
keyword-only parameter, and add a callable type annotation matching its
TransformKey-to-frozenset contract. Change every recursive invocation and the
stage_planner _rw call site to pass rewrite_fn by keyword, preserving existing
behavior.
In `@slayer/sql/generator.py`:
- Around line 6258-6273: Compute a set of trimmed cross-model aggregate plan IDs
in the projection loop using the existing trim predicate, then reuse that set
when building hidden_cma_order_ref instead of repeating the predicate. Since
this block runs after the transform-chain return, retain the projection loop’s
established conditions and use the shared IDs to CTE-qualify exactly the
aggregates removed from combined_parts.
In `@tests/test_dev1712_order_only_hidden_slots.py`:
- Around line 298-317: Update both test_joined_row_column_ungrouped_raises and
test_joined_row_column_grouped_raises to assert the exact user-facing message of
UnresolvableOrderColumnError, including the joined column and qualifier, while
preserving the existing exception-type assertions.
🪄 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: 402dea2a-27a0-4970-b673-bff8d384a3f2
📒 Files selected for processing (12)
.claude/skills/slayer-query.mdDECISIONS.mddocs/concepts/formulas.mddocs/concepts/queries.mdslayer/engine/planning.pyslayer/engine/stage_planner.pyslayer/sql/generator.pytests/parity_xfails.pytests/test_dev1712_order_only_hidden_slots.pytests/test_nested_dag_cross_stage_refs.pytests/test_projection_trim.pytests/test_sql_generator.py
💤 Files with no reviewable changes (1)
- tests/test_projection_trim.py
…t cleanups Codex (correctness): a joined ORDER BY ref whose leaf collides with a local declared dimension/measure (order=owners.status when `status` is a local dim) silently bound to the local column via the bare-leaf shortcut in the order-binding loop, sorting by the wrong field. Guard the bare-name shortcuts (stage_planner.plan_query) so they apply only to unqualified refs or refs qualified with the host model; a foreign-qualified ref falls through to the dotted/bind_expr path, where a truly-joined order ref is then rejected by the plan-time order validation. Regression test added. Sonar S3776 (planning.py): flattened rewrite_rank_partition_keys (cognitive complexity 26 -> under threshold) using a local recursion helper + ternary rebuilds; also made rewrite_fn keyword-only + type-annotated (CodeRabbit). Sonar S5685 (stage_planner.py): replaced the walrus-in-argument-list in the bound_filters rebuild with an explicit loop. Sonar S9073 (tests): split composite `assert a and b` into separate asserts. Sonar S5958 (tests): pytest.raises(Exception) -> pytest.raises(ValidationError). Full non-integration suite green (8971 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex: the order-binding host-local guard used a ModelScope-only host name, so for a downstream DAG stage (StageSchema, e.g. `s1`) it resolved to None and a self-qualified `s1.metric` order would be misclassified as foreign and skip the local-alias lookup. Use `_host_model_name(scope)` (the same resolution used everywhere else — source model for ModelScope, stage relation name for StageSchema). Regression test for the self-qualified downstream order. Sonar S3776 (planning.py): rewrite_rank_partition_keys is a closed-union isinstance dispatch (cognitive complexity 22); NOSONAR(S3776) with the same "scatter the contract" justification the sibling ValueKey walkers carry (_canonical_name, _iter_first_last_leaves). The prior flatten already cut it from 26. Full non-integration suite green (8972 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
slayer/engine/planning.py (1)
425-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass both arguments by keyword.
Use
key=kin the recursive call.Proposed fix
- return rewrite_rank_partition_keys(k, rewrite_fn=rewrite_fn) + return rewrite_rank_partition_keys(key=k, rewrite_fn=rewrite_fn)As per coding guidelines, “Use keyword arguments for functions with more than one parameter.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/planning.py` at line 425, Update the recursive rewrite_rank_partition_keys call to pass both arguments by keyword, using key=k and retaining rewrite_fn=rewrite_fn.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@slayer/engine/planning.py`:
- Line 425: Update the recursive rewrite_rank_partition_keys call to pass both
arguments by keyword, using key=k and retaining rewrite_fn=rewrite_fn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34c681be-9937-4a39-bade-45ce5a3f53b6
📒 Files selected for processing (3)
slayer/engine/planning.pyslayer/engine/stage_planner.pytests/test_nested_dag_cross_stage_refs.py
🚧 Files skipped from review as they are similar to previous changes (2)
- slayer/engine/stage_planner.py
- tests/test_nested_dag_cross_stage_refs.py
Codex: mirror the joined-ORDER-BY host-local guard into the LEGACY _resolve_order_column (generator.py) — a foreign-qualified ref whose leaf collides with a local column no longer takes the bare-name / _name shortcut, so it reaches the joined-qualifier rejection instead of silently sorting by the local column. Verified via the legacy enrich+generate path. CodeRabbit line threads: - T1 (stage_planner): two time-dimension granularities over one source column (created_at month + day) collapsed in _td_by_source (last wins); detect the ambiguous source and raise instead of silently picking a bucket. - T2 (stage_planner): UnresolvableOrderColumnError formats `qualifier.column`; pass the bare leaf + joined path so the message reads `customers.region`, not a duplicated `customers.customers.region`. - T3 (generator): a hidden order-only LOCAL DERIVED column (ColumnSqlKey, path=()) whose Column.sql crosses a join is not projected, so its join is never in the base FROM. The split path now resolves it through a throwaway scope to detect the crossing and rejects (project it) rather than emitting an unbound ORDER BY; a non-crossing derived column still orders on its (mixed-case-quoted) expression via the planned-dim helper. Regression tests for all four. Full non-integration suite green (8975 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex: - Partition-ambiguity guard (stage_planner): only flag a time-dimension source column ambiguous when the competing declarations differ in granularity; identical `created_at:month` declarations are one bucket, not a clash. - Derived-crossing ORDER BY error (generator): report the derived column's own qualified name (`orders.cust_region`) instead of the fabricated `customers.cust_region` — the column is local, it merely depends on an unpulled join. Removing the `sorted(...)[0]` also clears Sonar S8517. CodeRabbit nitpick (planning.py:425): pass `key=k` in the recursive rewrite_rank_partition_keys call (keyword-args convention). Regression test for the same-granularity non-ambiguity. Full non-integration suite green (8976 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion wrapper Codex: _apply_pagination_to_sql is only ever applied over the CTE-wrapped computed-column assembly (its single caller builds `WITH … SELECT … FROM <final_cte>`), so a SPLIT `<base_model>.<col>` reference names a table unbound in that scope — a raw-row query with a computed expression + unprojected base-column order emitted invalid SQL. Reject the unprojected sort key here (UnresolvableOrderColumnError) instead, consistent with the typed pipeline's plan-time guard and the base-SELECT applier (where the split IS bound). Full non-integration suite green (8976 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DEV-1703 Stage 8. Closes DEV-1712; closes DEV-1472, DEV-1495 bug 2, DEV-1497; promotes the Stage-8 DEV-1645 Flavor-A ORDER-BY pins.
What lands
Order-only hidden slots (Law 2) — an ORDER BY ref not declared as a dimension/measure is classified at plan time (
stage_planner.plan_query, right after_bucket_slots):StageSchema(never rejected)distinct_dimension_values=false)ORDER BY orders.<col>(mixed-case-aware)ValueError(HTTP 400) — not inGROUP BY; add to dims or order by an aggregateUnresolvableOrderColumnError(HTTP 400)change(amount:sum))ValueError— declare it as a measure. Deferred to DEV-1733 (worktree + strict-xfail future tests). Composite arithmetic is unexpressible viaOrderItem(Pydantic rejects at construction).The grouping predicate is planner-semantic (
agg_slotspresent, or dims/tds with dedup), so a hidden order aggregate that induces grouping correctly forces a row column in the same query to be rejected. The generator's oldNotImplementedErrorbecomes a defensive assertion — the plan-time pass guarantees only the split shape reaches it.Hidden cross-model-aggregate trim (DEV-1495 bug 2) — an order-only CMA is
hidden=True/public_alias=Noneand skips the combined projection, with a CTE-qualified ORDER BY term. Gated onnot transform_layersso a hidden CMA feeding acumsum(...)step stays projected for the step CTE (the transform outer-wrap trims there). The malformedorders.customers._sumalias half was already fixed by Stage 9's naming module.partition_by grain guard (DEV-1497) — a pre-intern pass (
planning.rewrite_rank_partition_keys, mirroringlower_sugar_transforms' identity-preserving rebuild) validates every rank-familypartition_bykey resolves to a query dimension/time-dimension by exact ValueKey membership (the typed binder resolves it to a ValueKey before validation, so legacy string-match ambiguity can't arise). A time-dim source column is rewritten to itsTimeTruncKeybucket (kills the raw-timestamp grain widening + duplicate alias); a non-dimension raises the restored legacy message (transform + column + available dims).DEV-1645 Flavor-A ORDER BY — ported main's lost legacy
_OrderColRef/_order_split_sql/_resolve_order_columnfix (split-not-composite +UnresolvableOrderColumnError) into the legacy generator across all three order-emission sites, so the 7 pinned unit tests + 1 integration pin promote.tests/parity_xfails.pyis now empty — the DEV-1485 (Stage 11) end-state.Deliberate divergence from main: the typed pipeline rejects a grouped raw-row order at plan time (HTTP 400) instead of emitting SQL the database rejects at execution.
Tests
New
tests/test_dev1712_order_only_hidden_slots.py(the contract table incl. execution/response-strip, diamond CMA, partition matrix, transform deferral + DEV-1733 xfail). Un-pinned: 7 Flavor-A unit + 1 Postgres integration + the DEV-1495/DEV-1497 xfails. The skipped cross-stage test became 3 passing variants. Full non-integration suite green (8970 passed, 0 failed, 31 xfailed);ruffclean.Out of scope
Result-key naming (Stage 9, already landed); windowed measures (Stage 10); inline transform/composite order targets (DEV-1733).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation