#267: aggregate/scalar dbt-expectations macros — source-table routing - #269
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds count-scalar detection for manifest-ingested dbt tests, promotes qualifying scalars to ChangesCount-scalar pruning flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
… US-002) Change the ingest classification gate in _classify_manifest_test so a graduatable count-of-rows scalar body (COUNT(*) / COUNT(col) / COUNT(DISTINCT col)) falls through to the common determinism -> safety -> candidate tail (from_manifest=True, sql=cc verbatim) instead of being skip-recorded. A non-count scalar (AVG/SUM/MIN/MAX, arithmetic-on-count, multi-aggregate) still skip-records malformed-supported-test. The determinism and safety gates still apply to a graduatable count-scalar (they live in the common tail). Narrow _AGGREGATE_SKIP_DETAIL to the non-count residue and note count-of-rows scalars are now pruned as of #267. Stage-0 preserved: no SQL building at ingest (the compiler restructures in a separate bead). SkipReason stays the closed 3-value set. Traces DEC-003, DEC-005.
…alars to candidates
…rows form (#267 US-003)
…te routing pins (#267 US-004) Pin the manifest-ingested COUNT-scalar custom_sql behaviour at the engine level via FakeBigQueryClient: - engineered determinism: a restructured count-scalar body with failures=0 routes to dropped/always-passes; with failures>0 routes to kept. The fake matches on sf_agg_value so the match itself proves the US-003 failing-rows restructure reached warehouse dispatch (not the always-1 wrap). - load-bearing mixed-candidate routing pins (materialised + oneshot): one ingested count-scalar (from_manifest, bypasses to SOURCE + restructured <> 0 wrap) plus one drafted not_null (routes to the sampled/materialised temp). Distinct per-test matchers make the pairing order-independent; assertions key on each dispatched compiled_sql (source vs temp + restructure shape), not the routed decision alone, so a per-test override regression is caught. Traces DEC-006 (source routing already covers from_manifest; mixed-candidate pin mandatory) and DEC-002 (restructure shape). Tests-only; no production change.
…candidate routing pins
… test (#267 US-005) Add a singular test to the dbt-expectations fixture whose compiled body is a bare scalar COUNT, plus tests proving it becomes a prunable from_manifest candidate (not skip-recorded). Traces DEC-008. Part A — tests/no_orders_above_threshold.sql: a singular test with a single ref('orders') and no GROUP BY, compiling to a bare top-level `select count(*) from "dev"."main"."orders" where amount > 1000`. amount is 100/200 in the two fixture rows, so the WHERE selects zero rows -> count 0 -> always-passes drop. Uses Jinja comments (dbt strips them) so compiled_code is comment-free and the warehouse adapter's validate_test_sql accepts the restructured body. Part B — regenerated target/manifest.json via the pinned uvx dbt-duckdb flow (dbt deps + dbt compile, scrubbing generated_at / invocation_id / created_at). Purely additive: the five existing dbt-expectations compiled bodies are byte-identical. Part C — ingest loads test (test_bridge_routes_scalar_count_singular_test) proves the node ingests as a CandidateTestCustomSQL with from_manifest=True, associated to orders, and NOT skip-recorded. Updated the sibling fixture-loads and dbt-expectations routing tests for the new 6th test node, and the prune-existing --from-manifest tests for the new 5th prunable manifest candidate (isolating the manifest path from the fixture's committed singular test via an empty --tests-dir).
…ure + ingest loads test
…isolate union-root test (code review)
…count-scalar pruning
wjduenow
changed the title
wjduenow
marked this pull request as ready for review
July 6, 2026 18:30
![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 (2)
tests/prune/test_engine.py (2)
5891-5901: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWeaker
_SESSIONguard than sibling tests.Line 5900 only excludes
"_SESSION._sf_sample_", while the single-candidate pins (Lines 5756, 5803, 5984) exclude the broader"_SESSION"substring. A regression that routes the ingested body to a session-scoped temp table under a different naming pattern (still containing_SESSIONbut not exactly_SESSION._sf_sample_) would slip past this assertion.🔧 Tighten the guard to match sibling tests
- assert "_SESSION._sf_sample_" not in ingested.compiled_sql + assert "_SESSION" not in ingested.compiled_sql🤖 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/prune/test_engine.py` around lines 5891 - 5901, The `_SESSION` assertion in the `test_engine` count-scalar bypass check is too specific and can miss other session-scoped temp table names. Update the `ingested` expectation in this test to match the broader guard used by sibling tests (the same `_SESSION` substring check), so any session-based temp table routing is caught regardless of naming pattern.
5977-5993: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing parity check vs. the materialised sibling test.
The materialised mixed test asserts
"fake_project.dataset.orders" not in not_null.compiled_sql"(Line 5911) to prove the not_null decision doesn't route to source. This oneshot counterpart omits that check, leaving an asymmetry in coverage between the two strategy pins despite the docstring's stated intent to independently pin both strategies.🔧 Add the missing parity assertion
# Drafted not_null: sampled via the CTE (per-test override did NOT bypass). assert "WITH sample" in not_null.compiled_sql + assert "fake_project.dataset.orders" not in not_null.compiled_sql assert not_null.decision == "dropped"🤖 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/prune/test_engine.py` around lines 5977 - 5993, The oneshot mixed prune test is missing the same source-routing parity assertion that exists in the materialised sibling test. In the test body around the `by_anchor["column.id"]` checks, add an assertion against `not_null.compiled_sql` to confirm it does not contain the source table reference, matching the coverage already pinned by `test_engine` for the materialised strategy and keeping the `ingested`/`not_null` strategy parity explicit.
🤖 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 `@src/signalforge/ingest/_compiled_sql.py`:
- Around line 215-267: The bare-count detection in is_prunable_count_scalar is
missing a HAVING guard, so a SELECT with COUNT(*) plus HAVING can still be
treated as prunable even when it returns no rows. Update the Select-root checks
in is_prunable_count_scalar to reject any query with a HAVING clause, alongside
the existing group check, before validating the single Count projection.
---
Nitpick comments:
In `@tests/prune/test_engine.py`:
- Around line 5891-5901: The `_SESSION` assertion in the `test_engine`
count-scalar bypass check is too specific and can miss other session-scoped temp
table names. Update the `ingested` expectation in this test to match the broader
guard used by sibling tests (the same `_SESSION` substring check), so any
session-based temp table routing is caught regardless of naming pattern.
- Around line 5977-5993: The oneshot mixed prune test is missing the same
source-routing parity assertion that exists in the materialised sibling test. In
the test body around the `by_anchor["column.id"]` checks, add an assertion
against `not_null.compiled_sql` to confirm it does not contain the source table
reference, matching the coverage already pinned by `test_engine` for the
materialised strategy and keeping the `ingested`/`not_null` strategy parity
explicit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d128578-555c-4c17-b64c-7ec45eeb79a6
📒 Files selected for processing (18)
.claude/rules/business-rule-tests.md.claude/rules/ingest-layer.md.claude/rules/prune-engine.mdCHANGELOG.mddocs/ingest-ops.mddocs/prune-ops.mdplans/super/267-aggregate-macro-routing.mdsrc/signalforge/ingest/_compiled_sql.pysrc/signalforge/ingest/reader.pysrc/signalforge/prune/compiler.pytests/cli/test_prune_existing.pytests/fixtures/dbt_project_expectations/target/manifest.jsontests/fixtures/dbt_project_expectations/tests/no_orders_above_threshold.sqltests/ingest/test_compiled_sql.pytests/ingest/test_manifest_tests.pytests/manifest/test_expectations_fixture_loads.pytests/prune/test_compiler.pytests/prune/test_engine.py
There was a problem hiding this comment.
Pull request overview
This PR implements #267’s “count-of-rows scalar” manifest-ingest path so bare SELECT COUNT(...) ... compiled test bodies are no longer skip-recorded as scalar/aggregate, but instead are safely pruned/graded by restructuring them into a failing-rows shape that works under the adapter’s SELECT COUNT(*) AS failures FROM (<sql>) envelope. It also adds fixture + unit/engine/CLI coverage and updates plan/docs/rules/changelog to reflect the new behavior and constraints.
Changes:
- Add
is_prunable_count_scalar(sqlglot AST gate) and update manifest-ingest routing to promote only COUNT-family scalar bodies tofrom_manifestcustom_sqlcandidates. - Restructure ingested COUNT-scalar SQL at compile time into a failing-rows form (
... WHERE sf_agg_value <> 0) to avoid the “always-1” scalar-collapse bug. - Add end-to-end pins: compiled SQL snapshot tests, engine routing tests (including mixed-candidate source-vs-sample behavior), a committed dbt fixture singular test, CLI isolation for
--tests-dir, and docs/rules/changelog updates.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/signalforge/ingest/_compiled_sql.py |
Adds is_prunable_count_scalar classifier for COUNT-only scalar SELECTs. |
src/signalforge/ingest/reader.py |
Promotes prunable COUNT-scalars to candidates; narrows aggregate skip detail. |
src/signalforge/prune/compiler.py |
Restructures ingested COUNT-scalars into failing-rows form; belt-and-braces invalidation for other scalars. |
tests/ingest/test_compiled_sql.py |
Adds unit matrix for is_prunable_count_scalar. |
tests/ingest/test_manifest_tests.py |
Updates ingest expectations: COUNT-scalars become candidates; non-count aggregates/nondeterminism still skip. |
tests/prune/test_compiler.py |
Pins compiler restructure output and guard-rails (row-returning unchanged, non-count scalar sentinel). |
tests/prune/test_engine.py |
Adds engineered-determinism tests + mixed-candidate routing pins for source vs sampled temp tables. |
tests/manifest/test_expectations_fixture_loads.py |
Extends fixture assertions to include the new singular COUNT-scalar test node. |
tests/cli/test_prune_existing.py |
Ensures manifest-on/off behavior is isolated from default tests-dir ingestion via an empty tests dir. |
tests/fixtures/dbt_project_expectations/tests/no_orders_above_threshold.sql |
Adds a singular COUNT-scalar test to exercise the manifest-ingest path. |
tests/fixtures/dbt_project_expectations/target/manifest.json |
Regenerated fixture manifest to include the new singular test node. |
docs/ingest-ops.md |
Documents COUNT-scalar graduation + 0=pass reinterpretation and its caveats. |
docs/prune-ops.md |
Documents compiler restructure and routing implications for ingested COUNT-scalars. |
CHANGELOG.md |
Records the behavior change in [Unreleased]. |
.claude/rules/ingest-layer.md |
Updates ingest-layer rules to reflect post-#267 behavior. |
.claude/rules/prune-engine.md |
Updates prune-engine rules to reflect COUNT-scalar restructure behavior. |
.claude/rules/business-rule-tests.md |
Updates business-rule test rules to include the new COUNT-scalar graduation path. |
plans/super/267-aggregate-macro-routing.md |
Adds the detailed super plan and DECs supporting the implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
PR Review SummaryAll review feedback addressed in Fixed (3 items)
False positivesNone. The |
|
Tip For best results, initiate chat on the files or code changes.
ᕦ(ò_óˇ)ᕤ ✏️ Learnings added
|
This was referenced Jul 14, 2026
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
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.
Summary
Super plan for #267 — graduate aggregate/scalar dbt manifest test bodies from skip-recorded to pruned.
Phase: detailing (awaiting approval)
Stories: 6 implementation + Quality Gate + Patterns & Memory
Decisions: 11 (DEC-001 … DEC-011)
Two reframing findings
_test_requires_source_tablealready routes everyfrom_manifestcustom_sql to source under all sample strategies. Follow-up (#154): aggregate/scalar dbt-expectations macros — source-table routing (Direction 2) #267 adds no new engine routing arm, only a behavioural mixed-candidate pin.COUNTbodies.Design (empirically de-risked)
is_prunable_count_scalar, sqlglot-AST, bool) — graduate single top-levelCOUNT/COUNT(col)/COUNT(DISTINCT); rejectAVG/SUM/MIN/MAX/COUNT+1/multi → keep skip-recording (SkipReasonstays 3-value).SELECT v FROM (SELECT (<body>) AS v) AS a WHERE v <> 0(no sqlglot in the compiler → no new confinement scan). Parses on BigQuery/Snowflake/Databricks. Adapter'sCOUNT(*) AS failureswrap → 0=pass / 1=fail (same contract asrow_count_between).0 = passsound under dbt's "returned rows = failures" convention. NoDropReason/SkipReason/audit-schema/_PROMPT_VERSIONchanges.Plan document
plans/super/267-aggregate-macro-routing.md— full Discovery / Architecture Review / Refinement (11 DECs) / Detailed Breakdown.Next steps
Summary by CodeRabbit
COUNT(*),COUNT(col), andCOUNT(DISTINCT ...)) and route them into the standard pruning pass/fail evaluation.0(pass) vs>= 1(fail).