DEV-1705 Stage 1: assert_scope_closed validator + carrier×scope acceptance matrix - #264
Hidden character warning
Conversation
Add slayer/sql/scope_check.py — a mechanical scope-closure validator that flags provable out-of-scope references (C1 unbound table qualifier, C2 unprojected inner-scope column) by walking sqlglot scopes. Sound-on-corpus (no false positives): unqualified refs and physical-table column names are unverifiable; plain/REPLACE stars export every name, `* EXCEPT (c)` drops c. Pre-RLS by default, with an allow_rls_correlation allowlist for the session-policy `_rls_src` correlated EXISTS. Wire it env-gated (SLAYER_VALIDATE_SCOPES) at the generator's post-mangle, pre-RLS terminals; the test conftest enables it suite-wide so a scope leak fails at generation time. Post-mangle (not pre-mangle) because BigQuery/T-SQL dotted aliases are mangled to `___` there — pre-mangle dotted refs parse as table.column (false leaks) and trigger BigQuery's TypeError. Add the carrier x scope acceptance matrix (Layer-1 green scope-closure sweep + Layer-2 strict-xfail defect pins for Stages 4/7/10, F1/F4 semantic pins, and RLS x isolation-CTE cells) and harvest tests from the abandoned point-fix worktrees (DEV-1526/1527/1531/1474/1496), each pinned to its owning stage. Also: give the DEV-1502 `__`-path reference-semantics test a real join chain (its minimal no-join fixture emitted genuinely-unbound SQL the validator correctly flags). Test-and-validator only; no generator behavior change beyond the env-gated hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oach-expressions-crossing-joins-on-the' into egor/dev-1705-dev-1703-stage-1-assert_scope_closed-validator-carrier×scope
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds dialect-aware SQL scope validation, integrates it into SQL generation, enables it across tests, and adds coverage for scope closure, RLS correlation, cross-model aggregates, derived first/last measures, and windowed-measure guards. ChangesScope validation
Aggregate SQL coverage
Estimated code review effort: 5 (Critical) | ~120 minutes 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: 1
🧹 Nitpick comments (6)
tests/test_sql_generator.py (4)
3526-3544: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate this test with the existing DEV-1531 xfail.
test_local_last_with_path_aliased_derived_source_xfailat lines 3897-3939 already pins the same defect with the sameregion_paymentcolumn, the sameregion_payment:last(orders.created_at)measure, and a strict xfail. Two strict xfails on one defect must both flip at Stage 5, so both need maintenance and both can block promotion.Keep the stronger assertion set (this new test) and remove the older one, or reduce the older one to a comment reference.
🤖 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_sql_generator.py` around lines 3526 - 3544, Consolidate the duplicate DEV-1531 coverage by keeping the stronger test_local_last_with_path_aliased_derived_source assertions and removing the existing test_local_last_with_path_aliased_derived_source_xfail test. If retaining the older test location, replace its implementation with a comment reference so only one strict xfail remains for this defect.
3516-3519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the composite assertion for clearer failure output.
The assertion combines two independent conditions. Split it so the failure message identifies which condition failed.
♻️ Proposed split
- first_subquery = norm.find("FROM (") - assert first_subquery != -1 and first_proj.start() > first_subquery, ( - f"DEV-1531: `_val` projection of `{ref}` is not inside the ranked " - f"subquery:\n{norm}" - ) + first_subquery = norm.find("FROM (") + assert first_subquery != -1, ( + f"DEV-1531: no ranked subquery (`FROM (`) found:\n{norm}" + ) + assert first_proj.start() > first_subquery, ( + f"DEV-1531: `_val` projection of `{ref}` is not inside the ranked " + f"subquery:\n{norm}" + )🤖 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_sql_generator.py` around lines 3516 - 3519, Split the composite assertion in the ranked-subquery validation into separate assertions: one verifying first_subquery is found and another verifying first_proj.start() is after first_subquery. Give each assertion a failure message that clearly identifies its specific failed condition while preserving the existing diagnostic context.Source: Linters/SAST tools
3502-3524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
_assert_ref_only_in_valtolerate the CAST-wrapped projection shape.The helper matches only the bare form
<ref> AS _val_<n>.test_same_sql_different_type_no_bad_dedupeat lines 3620-3645 expects the materialised projection to be CAST-wrapped (CAST(... AS DOUBLE PRECISION) AS _val_0). If the Stage 5 fix emits the CAST form, the helper's strip at line 3520 leavesrefinstrippedand the assertion at line 3521 fails even though the SQL is correct. That converts an auto-promotion into a false failure.Also, line 3614 calls
.group(1)on an unguarded_re.searchresult. After promotion, a shape mismatch raisesAttributeErrorinstead of a readable assertion.♻️ Suggested helper adjustment
- norm = _norm(sql) - first_proj = _re.search(rf"{_re.escape(ref)} AS _val_\d+", norm) + norm = _norm(sql) + # The materialised projection may be CAST-wrapped, so allow an + # optional trailing `AS <type>)` between the ref and the alias. + val_proj_re = rf"{_re.escape(ref)}[^,]*? AS _val_\d+" + first_proj = _re.search(val_proj_re, norm) @@ - stripped = _re.sub(rf"{_re.escape(ref)} AS _val_\d+", "", norm) + stripped = _re.sub(val_proj_re, "", norm)🤖 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_sql_generator.py` around lines 3502 - 3524, Update _assert_ref_only_in_val to recognize and strip both bare and CAST-wrapped materialized projections, including the CAST form around ref before AS _val_<n>, while preserving its ranked-subquery and leakage assertions. In test_same_sql_different_type_no_bad_dedupe, guard the _re.search result before calling group(1) so shape mismatches produce a clear assertion failure rather than AttributeError.
11685-11693: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a pre-assertion so each guard xfail fails for the intended reason.
Every test in this class puts only the generation call inside
pytest.raises. A strict xfail reportsxfailfor any failure in the test body, including an unrelatedValueErrorfromSlayerQuery(...)construction or a storage error. The pins therefore cannot distinguish "the guard is missing" from "the query shape is rejected earlier".Assert that generation currently succeeds before the
pytest.raisesblock, or record the today-behaviour SQL, so the xfail reason stays accurate. This applies to all eight tests inTestWindowedMeasureGuards.🤖 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_sql_generator.py` around lines 11685 - 11693, Update all eight tests in TestWindowedMeasureGuards to validate the query setup and generation path before the pytest.raises assertion, ensuring SlayerQuery construction and _engine_generate succeed for the intended input. Keep the existing expected ValueError assertion for the guard failure, so strict xfail only reflects the missing windowed-measure validation rather than unrelated setup or storage errors.tests/integration/test_integration_duckdb.py (2)
959-965: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an executed-value F1 pin, or narrow this header comment.
The header states that this block pins F1 multiply-per-match semantics. No test in
TestF1F4SemanticValuesasserts an F1 multiplied value. The fixture creates two paid orders for customer 1 precisely to expose that shape, and the F1 pin at lines 1060-1065 defers to the SQL-shape test intests/test_carrier_scope_matrix.py.Add a test that groups by a dimension sharing grain with the join so the 1:N multiplication is visible in the returned value, or remove the F1 line from this comment.
Do you want me to draft the F1 executed-value test against this fixture?
🤖 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_duckdb.py` around lines 959 - 965, Add an executed-value F1 test in TestF1F4SemanticValues using the existing fixture, grouping by a dimension at the join’s grain and asserting the returned aggregate reflects multiply-per-match semantics for customer 1’s two paid orders. Keep the existing F4 coverage unchanged; only remove the F1 header claim if such a value assertion cannot be added.
826-826: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFunction-local
ModelJoinimports in both new DuckDB fixtures. The module does not importModelJoinat the top, so each new fixture re-imports it inside its body. AddModelJointo the existing module-levelslayer.core.modelsimport and remove both local imports.
tests/integration/test_integration_duckdb.py#L826-L826: remove the localfrom slayer.core.models import ModelJoinin_dev1531_duckdb_storage.tests/integration/test_integration_duckdb.py#L971-L971: remove the localfrom slayer.core.models import ModelJoinin_f1f4_duckdb_storage.Based on coding guidelines: "Use keyword arguments for functions with more than one parameter, and keep 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/integration/test_integration_duckdb.py` at line 826, Move ModelJoin into the existing module-level slayer.core.models import, then remove the function-local imports from _dev1531_duckdb_storage and _f1f4_duckdb_storage in tests/integration/test_integration_duckdb.py at lines 826-826 and 971-971.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.
Inline comments:
In `@slayer/sql/scope_check.py`:
- Around line 24-27: Update the docstrings describing maybe_validate_scopes to
reflect post-mangle, pre-RLS validation: in slayer/sql/scope_check.py lines
24-27, replace the pre-mangle wording and note that dialect alias mangling runs
first; in tests/conftest.py lines 83-97, change the generator hook description
to post-mangle, pre-RLS. No code behavior changes are needed.
---
Nitpick comments:
In `@tests/integration/test_integration_duckdb.py`:
- Around line 959-965: Add an executed-value F1 test in TestF1F4SemanticValues
using the existing fixture, grouping by a dimension at the join’s grain and
asserting the returned aggregate reflects multiply-per-match semantics for
customer 1’s two paid orders. Keep the existing F4 coverage unchanged; only
remove the F1 header claim if such a value assertion cannot be added.
- Line 826: Move ModelJoin into the existing module-level slayer.core.models
import, then remove the function-local imports from _dev1531_duckdb_storage and
_f1f4_duckdb_storage in tests/integration/test_integration_duckdb.py at lines
826-826 and 971-971.
In `@tests/test_sql_generator.py`:
- Around line 3526-3544: Consolidate the duplicate DEV-1531 coverage by keeping
the stronger test_local_last_with_path_aliased_derived_source assertions and
removing the existing test_local_last_with_path_aliased_derived_source_xfail
test. If retaining the older test location, replace its implementation with a
comment reference so only one strict xfail remains for this defect.
- Around line 3516-3519: Split the composite assertion in the ranked-subquery
validation into separate assertions: one verifying first_subquery is found and
another verifying first_proj.start() is after first_subquery. Give each
assertion a failure message that clearly identifies its specific failed
condition while preserving the existing diagnostic context.
- Around line 3502-3524: Update _assert_ref_only_in_val to recognize and strip
both bare and CAST-wrapped materialized projections, including the CAST form
around ref before AS _val_<n>, while preserving its ranked-subquery and leakage
assertions. In test_same_sql_different_type_no_bad_dedupe, guard the _re.search
result before calling group(1) so shape mismatches produce a clear assertion
failure rather than AttributeError.
- Around line 11685-11693: Update all eight tests in TestWindowedMeasureGuards
to validate the query setup and generation path before the pytest.raises
assertion, ensuring SlayerQuery construction and _engine_generate succeed for
the intended input. Keep the existing expected ValueError assertion for the
guard failure, so strict xfail only reflects the missing windowed-measure
validation rather than unrelated setup or storage errors.
🪄 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: 6d195e4d-66d0-4caf-a075-8d20637137b0
📒 Files selected for processing (10)
DECISIONS.mddocs/development.mdslayer/sql/generator.pyslayer/sql/scope_check.pytests/conftest.pytests/integration/test_integration_duckdb.pytests/test_carrier_scope_matrix.pytests/test_reference_semantics.pytests/test_scope_check.pytests/test_sql_generator.py
- Correct the stale "pre-mangle" wording to post-mangle, pre-RLS in the scope_check module docstring, the TypeError carve-out comment, and the conftest fixture docstring (the hook validates after rewrite_emitted_sql). - Consolidate the duplicate DEV-1531 local first/last xfail: the pre-existing `test_local_last_with_path_aliased_derived_source_xfail` placeholder is subsumed by the harvested `test_local_last_with_path_aliased_derived_source` (stronger `_assert_ref_only_in_val` assertion); left a comment pointer so a single strict-xfail flips at Stage 5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
aed4268
into
egor/dev-1703-comprehensive-approach-expressions-crossing-joins-on-the
1 participant
DEV-1703 Stage 1 (DEV-1705). Test-and-validator only — no generator behavior change beyond one env-gated hook.
What lands
Validator —
slayer/sql/scope_check.pyassert_scope_closed(sql)walks every sqlglot scope and flags a provable out-of-scope reference:REPLACEstar exports every name;* EXCEPT (c)dropsc).Sound-on-corpus (zero false positives): unqualified/ambiguous refs and physical-table column names are unverifiable and never flagged. Pre-RLS by default;
allow_rls_correlation=Truewhitelists the session-policy_rls_srccorrelatedEXISTS. Returns structuredScopeCheckResult/ raisesScopeLeakError.Harness wiring — env-gated
maybe_validate_scopesat the generator's post-mangle, pre-RLS terminals;conftest.pysetsSLAYER_VALIDATE_SCOPES=1suite-wide, so a scope leak fails at generation time. Post-mangle (not pre-) because BigQuery/T-SQL dotted aliases become unambiguous___names there — pre-mangle dotted refs parse astable.column(false leaks) and trigger BigQuery'sTypeError; residual BigQuery parseTypeErroris a bounded, reported skip owned by Stage 9 (DEV-1713).Carrier×scope acceptance matrix —
tests/test_carrier_scope_matrix.pyLayer-1 green scope-closure invariant sweep (each cell asserts the scope shape was exercised, then closure) + Layer-2 strict-xfail defect pins (Stages 4/7/10) + F1/F4 semantic pins (SQL-shape) + RLS×isolation-CTE cells.
Harvest from the abandoned point-fix worktrees, each pinned to its owning stage: DEV-1526 (Stage 4), DEV-1531 SQL-shape + DuckDB executed values (Stage 5), DEV-1496 raise-don't-degrade guards (Stage 10), reconstructed DEV-1474 (Stage 7); DEV-1527 covered by the existing placeholder. F1/F4 executed-value pins are green. Manifest in the matrix docstring.
Verification
Notes
__-path reference-semantics fixture had no join target → emitted unbound SQL the validator correctly flags; gave it a real join chain).DECISIONS.md+docs/development.mdupdated.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
SLAYER_VALIDATE_SCOPES=1.Bug Fixes
firstandlastvalues and filter behavior.Documentation
Tests