DEV-1707 Stage 3: unified reroot_aggregate_key (source + args + kwargs + column_filter) - #266
Merged
Conversation
…s + column_filter) One pure `reroot_aggregate_key(key, *, target_path)` in slayer/core/keys.py re-anchors ALL embedded references of an AggregateKey — source, positional args, kwarg values, and column_filter_key — symmetrically when a cross-model aggregate renders in its target scope. Replaces the three scattered per-field strip implementations that had diverged into two semantics: - cross_model_planner.py::_local_agg_formula / _reroot_col_kwarg (string-level) - generator.py::_render_cross_model_cte source/_reroot_kwarg/local_args block - generator.py HAVING-route _reroot_having block Unified on prefix-strip-with-residual (planner semantics; the generator's old exact-match is subsumed). Non-matching paths pass through unchanged (the function is total, never raises). column_filter_key is owner-anchored, hence invariant under reroot and copied through unchanged. Closes DEV-1476 (c): positional args now strip the target prefix in lockstep with kwargs. Closes DEV-1476 (d-cross): a path-bearing ColumnSqlKey explicit time arg is rerooted to a target-local key before _resolve_explicit_time_col, so it no longer raises. The residual deeper-hop guard is reworded from the now-closed DEV-1476 to DEV-1526/Stage 4 (the isolated CTE does not yet pull the deeper join); the analogous bare-column case is caught by the SLAYER_VALIDATE_SCOPES scope-closure validator. Full non-integration suite green; new tests/test_reroot_aggregate_key.py (45 tests) pins the pure key algebra, filter-key invariance, the _local_agg_formula behaviour-lock, and the reworded guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oach-expressions-crossing-joins-on-the' into egor/dev-1707-dev-1703-stage-3-unified-reroot_aggregate_key-source-args # Conflicts: # DECISIONS.md
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesCross-model aggregate rerooting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CrossModelPlanner
participant reroot_aggregate_key
participant SQLGenerator
participant AggregateKey
CrossModelPlanner->>reroot_aggregate_key: reroot source and aggregate arguments
reroot_aggregate_key-->>CrossModelPlanner: return target-local AggregateKey
SQLGenerator->>reroot_aggregate_key: reroot CTE or HAVING aggregate
reroot_aggregate_key-->>SQLGenerator: return normalized AggregateKey
SQLGenerator->>AggregateKey: render residual references and preserved filter metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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.
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)
4436-4463: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFail fast for residual
ColumnKeytime arguments.SLAYER_VALIDATE_SCOPESis opt-in and is enabled only by the test suite. A residualColumnKeycan therefore emit an unjoined alias such ascustomers__regions.population; add a symmetric eagerNotImplementedErrorin theColumnKeybranch.🤖 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 4436 - 4463, The ColumnKey branch in the first/last positional-argument handling must fail fast when a residual join path exists, rather than constructing an unjoined relation alias. Add a symmetric NotImplementedError for ColumnKey values with a non-empty path, while preserving the current relation/leaf return for pathless keys and the existing ColumnSqlKey behavior.
🧹 Nitpick comments (2)
slayer/core/keys.py (1)
518-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider rebuilding with
model_copyto avoid field drift.The function enumerates all five
AggregateKeyfields explicitly. That matches the current model. If a field is added toAggregateKeylater, reroot silently drops it from the returned key, and the identity changes without any test failure in this module.model_copy(update=...)re-anchors only the fields that reroot owns.Note that
model_copyskips the_canonicalize_kwargsvalidator. Rerooting preserves kwarg names, so the input order is already canonical and the sort is a no-op.test_kwargs_canonical_sort_preserved_after_rerootpins that behavior either way.♻️ Proposed refactor
- return AggregateKey( - source=_reroot_path_ref(key.source, target_path=target_path), - agg=key.agg, - args=tuple( - _reroot_path_ref(a, target_path=target_path) for a in key.args - ), - kwargs=tuple( - (k, _reroot_path_ref(v, target_path=target_path)) - for k, v in key.kwargs - ), - column_filter_key=key.column_filter_key, - ) + return key.model_copy(update={ + "source": _reroot_path_ref(key.source, target_path=target_path), + "args": tuple( + _reroot_path_ref(a, target_path=target_path) for a in key.args + ), + "kwargs": tuple( + (k, _reroot_path_ref(v, target_path=target_path)) + for k, v in key.kwargs + ), + })🤖 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/core/keys.py` around lines 518 - 529, Update the AggregateKey reroot construction to use model_copy(update=...) on the existing key, replacing only source, args, and kwargs with their rerooted values while preserving all other fields automatically. Keep kwarg ordering unchanged, relying on the existing canonical input order.tests/test_reroot_aggregate_key.py (1)
570-576: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the fixture call out of the
pytest.raisesblock.
_guard_source_model()runs inside thewith pytest.raises(NotImplementedError)block. If that helper ever raises, the test passes for the wrong reason. This is also what SonarCloud flags at Line 570.♻️ Proposed refactor
+ source_model = _guard_source_model() with pytest.raises(NotImplementedError) as excinfo: gen._resolve_explicit_time_col( key=key, - source_model=_guard_source_model(), + source_model=source_model, source_relation="customers", bundle=None, )🤖 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_reroot_aggregate_key.py` around lines 570 - 576, Move the _guard_source_model() call before the pytest.raises(NotImplementedError) context and store its result, then pass that stored source model to gen._resolve_explicit_time_col. Keep only the intended method call inside the exception assertion so the test validates the correct failure.Source: Linters/SAST tools
🤖 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 4436-4463: The ColumnKey branch in the first/last
positional-argument handling must fail fast when a residual join path exists,
rather than constructing an unjoined relation alias. Add a symmetric
NotImplementedError for ColumnKey values with a non-empty path, while preserving
the current relation/leaf return for pathless keys and the existing ColumnSqlKey
behavior.
---
Nitpick comments:
In `@slayer/core/keys.py`:
- Around line 518-529: Update the AggregateKey reroot construction to use
model_copy(update=...) on the existing key, replacing only source, args, and
kwargs with their rerooted values while preserving all other fields
automatically. Keep kwarg ordering unchanged, relying on the existing canonical
input order.
In `@tests/test_reroot_aggregate_key.py`:
- Around line 570-576: Move the _guard_source_model() call before the
pytest.raises(NotImplementedError) context and store its result, then pass that
stored source model to gen._resolve_explicit_time_col. Keep only the intended
method call inside the exception assertion so the test validates the correct
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9031c18-36dd-498a-ace2-86de1aeaabdc
📒 Files selected for processing (6)
DECISIONS.mddocs/architecture/cross-model-aggregates.mdslayer/core/keys.pyslayer/engine/cross_model_planner.pyslayer/sql/generator.pytests/test_reroot_aggregate_key.py
- tests/test_reroot_aggregate_key.py: hoist _guard_source_model() out of the pytest.raises block so the only possibly-throwing call inside it is the one under test (Sonar python:S5778 MAJOR; CodeRabbit tests nitpick). - slayer/core/keys.py: rebuild the rerooted AggregateKey via model_copy(update=) so fields reroot doesn't own (agg, column_filter_key, any future field) ride through instead of being enumerated and silently droppable (CodeRabbit nitpick). Output-equivalent — model_copy skips _canonicalize_kwargs but reroot preserves kwarg names/order so the already-canonical sort is unchanged. Not applied: CodeRabbit's "fail fast for residual ColumnKey time args" in _resolve_explicit_time_col — the suggested guard would break the green test_composite_first_last_with_joined_time_arg_adds_join (a LOCAL aggregate legally uses a joined-path ColumnKey time arg the base SELECT pulls); the method cannot distinguish that from the cross-model-CTE residual case, so the guard belongs at the CTE call site and is DEV-1526/Stage-4 scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
ZmeiGorynych
merged commit Aug 2, 2026
0a14862
into
egor/dev-1703-comprehensive-approach-expressions-crossing-joins-on-the
6 checks passed
This was referenced Aug 3, 2026
Merged
Merged
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 3. Closes DEV-1707; closes the cross-model residuals of DEV-1476 (c) and (d-cross).
What
One pure
reroot_aggregate_key(key, *, target_path)inslayer/core/keys.pyre-anchors all embedded references of anAggregateKey— source, positional args, kwarg values, andcolumn_filter_key— symmetrically when a cross-model aggregate renders in its target scope. It replaces the three scattered per-field strip implementations that had diverged into two semantics:slayer/engine/cross_model_planner.py::_local_agg_formula/_reroot_col_kwarg(string-level, prefix-strip-with-residual)slayer/sql/generator.py::_render_cross_model_ctesource rebuild +_reroot_kwarg+local_args(key-level, exact-match)slayer/sql/generator.pyHAVING route_reroot_having(near-verbatim copy of the above)Unified on prefix-strip-with-residual (
('customers','regions')under target('customers',)→('regions',); exact match → local). The generator's old exact-match is subsumed. Non-matching paths pass through unchanged — the function is total and never raises; a genuinely mis-pathed ref still surfaces at the downstream binder / kwarg-path validator exactly as before.column_filter_keyis owner-anchored (itscanonical_sql/referenced_join_pathsare relative to the model owning the filtered column), hence invariant under reroot and copied through unchanged. A rerooted filtered cross-model aggregate still reads local-source + non-empty filter paths — the DEV-1503 filtered-local trigger shape.Closes
ColumnSqlKeyexplicit time arg is rerooted to a target-local key before_resolve_explicit_time_col, so it no longer raises.The
_resolve_explicit_time_colresidual-hop guard is reworded from the now-closed DEV-1476 to DEV-1526 / Stage 4 (the isolated CTE does not yet pull a join a hop past the target). The analogous bare-column residual case is caught loudly by theSLAYER_VALIDATE_SCOPESscope-closure validator.Out of scope
DEV-1476 (b) (no-time-dimension gating — Stage 6); CTE ScopeFrame migration (Stage 4).
Tests
New
tests/test_reroot_aggregate_key.py(45 tests) pins the pure key algebra (source/args/kwargs × exact/deeper-residual/non-matching/scalar for bothColumnKeyandColumnSqlKey),column_filter_keyinvariance + the DEV-1503 trigger shape, the_local_agg_formulabehaviour-lock, empty-target identity, and the reworded DEV-1526 guard. Codex reviewed both the plan and the tests; findings folded in.Full non-integration suite green (8640 passed, 48 skipped, 75 xfailed, 1 unrelated xpassed);
ruff check slayer/ tests/clean.DECISIONS.md+docs/architecture/cross-model-aggregates.mdupdated.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests