DEV-1710 Stage 6: first/last explicit-time completion - #268
Conversation
…Frame Explicit first/last ranking-time args now discover their crossed join via the host ScopeFrame (Law 1) instead of a bespoke collector: - Extract _explicit_time_arg_of: one arg-selection contract shared by the raise-gate, discovery pass, and render seam (Codex F1). Fixes a latent gate bug where any()-scan let a scalar-first/col-later shape skip the "requires a ranking time column" raise and crash the ROW_NUMBER build. - _resolve_agg_inputs_via_scope gains sub-pass 4: scope.resolve(arg) register-only pulls the crossed LEFT JOIN (bare / multi-hop / local derived); a path-bearing ColumnSqlKey (DEV-1526 residual) is skipped. - _resolve_explicit_time_col renders through a throwaway host ScopeFrame when a bundle is present (reserved-word quoting for free); the bundle=None fallback and the DEV-1526 / not-found guards are preserved in order. - Delete the Phase.AGGREGATE arm of _collect_joined_paths_for_base. Closes DEV-1476 (all four reprose green). Residual-hop guard kept (DEV-1526 / Stage 4) per Option A. +23 tests; full non-integration suite green (8156 passed, 61 xfailed unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rame) Integrates DEV-1708 Stage 4 into the Stage 6 branch. Only conflict was the DECISIONS.md append (kept both entries, Stage 4 then Stage 6); generator.py auto-merged cleanly. Composition verified: Stage 6's cross-model first/last tests run through Stage 4's rewritten _render_cross_model_cte and pass; Stage 6's DEV-1526 time-arg residual guard is orthogonal to Stage 4's DEV-1526 source-crossing closure and still holds. Full non-integration suite green: 8219 passed, 49 xfailed (Stage 4 promoted the DEV-1526 source cluster from 61). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 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 (1)
📝 WalkthroughWalkthroughThe SQL generator now resolves cross-model CTE expressions through scoped join registration, allocates aliases across the full render, materializes joined first/last values, and emits dialect-specific null-safe grain predicates. Shared-grain planning rejects unsupported plain derived dimensions. ChangesCross-model SQL rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlannedQuery
participant SQLGenerator
participant ScopeFrame
participant CrossModelCTE
participant SqlDialect
PlannedQuery->>SQLGenerator: generate_from_planned
SQLGenerator->>ScopeFrame: resolve expressions and register joins
ScopeFrame->>CrossModelCTE: provide scoped paths and aliases
CrossModelCTE->>SqlDialect: build null-safe grain predicates
CrossModelCTE-->>SQLGenerator: rendered CTE SQL
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/test_dev1708_stage4_cte_scope.py (2)
777-782: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
tmp_pathso the fixture cleans up its temporary directory.
tempfile.mkdtemp()creates a directory that is never removed. The fixture runs once per test that requests it, so each run leaves a SQLite file and a YAML store behind. The pytesttmp_pathfixture removes the directory automatically.♻️ Proposed change
`@pytest.fixture` -async def sqlite_engine() -> AsyncIterator[SlayerQueryEngine]: - d = tempfile.mkdtemp() - db_path = os.path.join(d, "t.db") +async def sqlite_engine(tmp_path) -> AsyncIterator[SlayerQueryEngine]: + d = str(tmp_path) + db_path = os.path.join(d, "t.db")🤖 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_dev1708_stage4_cte_scope.py` around lines 777 - 782, Update the async fixture sqlite_engine to accept pytest’s tmp_path fixture and use it as the temporary base directory instead of creating a directory with tempfile.mkdtemp(). Build the database and YAMLStorage paths from tmp_path so pytest automatically cleans up all generated files.
418-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the composite assertion.
SonarCloud flags this line. Two separate assertions report which condition failed.
✏️ Proposed fix
- assert "ORDER BY" in inner and "regions.opened_at" in inner, inner + assert "ORDER BY" in inner, inner + assert "regions.opened_at" in inner, inner🤖 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_dev1708_stage4_cte_scope.py` at line 418, Split the composite assertion in the relevant test into two independent assertions: one verifying "ORDER BY" is present in inner and another verifying "regions.opened_at" is present, preserving the existing failure context where appropriate.Source: Linters/SAST tools
slayer/sql/generator.py (3)
4494-4498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the single-iteration loop with a direct first-element lookup.
The
forloop returns on its first iteration, so it never iterates. The intent — "the first positional arg, if it is a column ref" — is clearer as an index lookup. This helper is the shared contract for three call sites, so its selection rule should read unambiguously.♻️ Proposed simplification
if key.agg not in ("first", "last"): return None - for a in key.args: - return a if isinstance(a, (ColumnKey, ColumnSqlKey)) else None - return None + if not key.args: + return None + first = key.args[0] + return first if isinstance(first, (ColumnKey, ColumnSqlKey)) else 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 `@slayer/sql/generator.py` around lines 4494 - 4498, In the helper containing the key.agg check, replace the single-iteration loop over key.args with a direct lookup of the first positional argument, returning it only when it is a ColumnKey or ColumnSqlKey and otherwise returning None. Preserve the existing "first"/"last" aggregation guard and empty-arguments behavior.
6492-6508: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the unsupported-derived-grain message with the planner.
slayer/engine/cross_model_planner.pyraisesNotImplementedErrorwith the same multi-line text for the same condition. Two copies of one user-facing message drift apart on the next edit. Extract the message (or a smallraise_derived_shared_grain_unsupported(column_name)helper) into one module and call it from both sites.Note also that the planner check runs first in the engine path, so this generator branch is reachable only from direct
_render_cross_model_ctecalls. Keep it as the defence-in-depth guard, but say so in the comment.🤖 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 6492 - 6508, Centralize the unsupported plain-derived shared-grain error message used by the generator branch and the planner’s corresponding NotImplementedError, preferably via a small shared helper such as raise_derived_shared_grain_unsupported(column_name), and call it from both sites. Preserve the existing message and condition, keep the generator check as a defense-in-depth guard for direct _render_cross_model_cte calls, and state that purpose in its comment.
5899-5912: 🚀 Performance & Scalability | 🔵 TrivialConsider the join-plan cost of the expanded null-safe predicate.
On T-SQL, Oracle, and Redshift the join-back becomes
a = b OR (a IS NULL AND b IS NULL). That is not an equijoin, so those planners can fall back to a nested-loop or hash-with-residual plan on large grain sets. The native forms (IS NOT DISTINCT FROM,<=>,IS) usually keep the equijoin. Track query latency on the expanded-form dialects, and consider aCOALESCE(col, <sentinel>)grain key if a regression appears.🤖 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 5899 - 5912, Review the join-back generated by _null_safe_join_pair_sql for T-SQL, Oracle, and Redshift, and measure query latency and execution plans on large grain sets where it expands to OR-based NULL-safe equality. If a regression is observed, replace that dialect-specific form with a planner-friendly COALESCE-based grain key using a type-safe sentinel, while preserving NULL-equals-NULL semantics and leaving native IS NOT DISTINCT FROM, <=>, and IS forms unchanged.tests/test_dev1476_first_last_explicit_time.py (1)
1073-1082: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the bundle construction out of the
pytest.raisesblock.SonarCloud flags both blocks:
_u_bundle(...)is evaluated inside thewithbody, so a failure there would also satisfy the assertion. Build the bundle first, then call the method under test.💚 Proposed fix (same shape for both tests)
orders = _u_orders() + bundle = _u_bundle(orders) key = AggregateKey( source=ColumnKey(leaf="amount"), agg="last", args=(ColumnSqlKey(path=(), model="orders", column_name="not_a_real_col"),), ) with pytest.raises(ValueError, match="Derived time column 'not_a_real_col'"): self._gen()._resolve_explicit_time_col( key=key, source_model=orders, source_relation="orders", - bundle=_u_bundle(orders), + bundle=bundle, )Also applies to: 1088-1097
🤖 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_dev1476_first_last_explicit_time.py` around lines 1073 - 1082, Construct the `_u_bundle(orders)` result before entering each `pytest.raises` block in the affected tests. Then pass the prebuilt bundle to `_resolve_explicit_time_col`, ensuring only that method’s ValueError can satisfy the assertion while preserving the existing test setup and expectations.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.
Inline comments:
In `@slayer/sql/generator.py`:
- Around line 6608-6613: Update the positional-argument registration near
local_agg_key.args to resolve only the argument selected by
_explicit_time_arg_of, matching _resolve_first_last_time_arg, and preserve the
host pass’s behavior of skipping a residual ColumnSqlKey. Do not resolve every
ColumnKey/ColumnSqlKey argument, so the ranked CTE registers only joins required
by the rendered ORDER BY.
- Around line 6912-6946: Update _register_routed_filter_joins to traverse filter
dependencies through walk_value_keys, covering BetweenKey, ScalarCallKey, and
TransformKey alongside existing key types. Preserve the current AggregateKey
rerooting, scope resolution, and column_filter join-path handling while reusing
the shared traversal instead of maintaining incomplete local recursion.
---
Nitpick comments:
In `@slayer/sql/generator.py`:
- Around line 4494-4498: In the helper containing the key.agg check, replace the
single-iteration loop over key.args with a direct lookup of the first positional
argument, returning it only when it is a ColumnKey or ColumnSqlKey and otherwise
returning None. Preserve the existing "first"/"last" aggregation guard and
empty-arguments behavior.
- Around line 6492-6508: Centralize the unsupported plain-derived shared-grain
error message used by the generator branch and the planner’s corresponding
NotImplementedError, preferably via a small shared helper such as
raise_derived_shared_grain_unsupported(column_name), and call it from both
sites. Preserve the existing message and condition, keep the generator check as
a defense-in-depth guard for direct _render_cross_model_cte calls, and state
that purpose in its comment.
- Around line 5899-5912: Review the join-back generated by
_null_safe_join_pair_sql for T-SQL, Oracle, and Redshift, and measure query
latency and execution plans on large grain sets where it expands to OR-based
NULL-safe equality. If a regression is observed, replace that dialect-specific
form with a planner-friendly COALESCE-based grain key using a type-safe
sentinel, while preserving NULL-equals-NULL semantics and leaving native IS NOT
DISTINCT FROM, <=>, and IS forms unchanged.
In `@tests/test_dev1476_first_last_explicit_time.py`:
- Around line 1073-1082: Construct the `_u_bundle(orders)` result before
entering each `pytest.raises` block in the affected tests. Then pass the
prebuilt bundle to `_resolve_explicit_time_col`, ensuring only that method’s
ValueError can satisfy the assertion while preserving the existing test setup
and expectations.
In `@tests/test_dev1708_stage4_cte_scope.py`:
- Around line 777-782: Update the async fixture sqlite_engine to accept pytest’s
tmp_path fixture and use it as the temporary base directory instead of creating
a directory with tempfile.mkdtemp(). Build the database and YAMLStorage paths
from tmp_path so pytest automatically cleans up all generated files.
- Line 418: Split the composite assertion in the relevant test into two
independent assertions: one verifying "ORDER BY" is present in inner and another
verifying "regions.opened_at" is present, preserving the existing failure
context where appropriate.
🪄 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: 3761d652-8d73-4ed2-bdfe-46edb9252351
📒 Files selected for processing (12)
DECISIONS.mddocs/architecture/cross-model-aggregates.mdslayer/engine/cross_model_planner.pyslayer/sql/dialects/_tier2.pyslayer/sql/dialects/base.pyslayer/sql/dialects/sqlite.pyslayer/sql/dialects/tsql.pyslayer/sql/generator.pytests/test_carrier_scope_matrix.pytests/test_dev1476_first_last_explicit_time.pytests/test_dev1708_stage4_cte_scope.pytests/test_sql_generator.py
💤 Files with no reviewable changes (1)
- tests/test_carrier_scope_matrix.py
Two new exception tests called self._gen() inside the pytest.raises block, so Sonar flagged two potentially-throwing invocations. Hoist gen + bundle out so the only call that can raise inside the block is the one under test — matches the existing pattern in test_reroot_aggregate_key.py.
…ng-joins-on-the' of https://github.com/MotleyAI/slayer into egor/dev-1710-dev-1703-stage-6-firstlast-explicit-time-completion
|
31f9aae
into
egor/dev-1703-comprehensive-approach-expressions-crossing-joins-on-the
1 participant
DEV-1703 Stage 6 (DEV-1710) — closes DEV-1476
Explicit
first/lastranking-time args now discover their crossed join through the hostScopeFrame(Law 1), not a bespoke collector — the last first/last-specific legacy join collector is gone.Changes (
slayer/sql/generator.py)_explicit_time_arg_of(key)— one arg-selection contract shared by the raise-gate, the discovery pass, and the render seam, so they can't drift (Codex F1). Fixes a latent gate bug: the oldany(...)scan let a scalar-first/col-later shape skip the "requires a ranking time column" raise and then crash building theROW_NUMBERmap._resolve_agg_inputs_via_scope—scope.resolve(arg)register-only pulls the crossed LEFT JOIN as a Law-1 side effect (bare single/multi-hop, and local-derived args reaching a joined table). Path-bearingColumnSqlKey(DEV-1526 residual) is skipped._resolve_explicit_time_colrenders through a throwaway hostScopeFramewhen a bundle is present (reserved-word quoting for free, DEV-1686); thebundle=Nonefallback and the DEV-1526 / not-found guards are preserved in order. Safe becausetime_columnis re-parsed downstream, so resolver normalization washes out.Phase.AGGREGATEarm of_collect_joined_paths_for_base; it now collects only ROW dimension paths.Under the agreed Option A: the residual-hop
NotImplementedErroris kept (narrowed, owned by DEV-1526 / Stage 4), and Stage-5 arg isolation is not pulled forward.Tests (+23, in
tests/test_dev1476_first_last_explicit_time.py)Written first (TDD), Codex-reviewed against the plan. Shared-helper contract, discovery join-registration (single/multi-hop/derived/composite, cross-model-skip + residual-skip proven with a
resolvespy), gate-mutation catch, resolver-path rendering incl. reserved-word quoting + three bundle-setColumnSqlKeycases, and four end-to-end pins. DEV-1476 fully closes (all four reprose green).Note — stacked on dev-1708
This branch merges
origin/egor/dev-1708(Stage 4) in, so until dev-1708 lands indev-1703the diff below also shows Stage 4's cross-model/isolation CTE renderer. Once dev-1708 merges, the diff collapses to Stage 6 only. Full non-integration suite green on the merged tree: 8219 passed, 49 xfailed, 0 failed (Stage 4 promoted the DEV-1526 source-crossing cluster from 61).Out of scope (flagged)
A derived time column whose
Column.sqlreferences another derived column (nested inlining) —expand_derived_refs_syncdoesn't recursively inline bare sibling-derived refs; this fails identically for a plain dimension, so it's a general column-expansion limitation, not a time-arg concern.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
firstandlastcalculations with explicit time fields.Bug Fixes
Documentation