#140: vendor-neutral get_row_count seam for sample-bucket sizing

[wjduenow]

Fixes #140 (bead bd_1-scaffolding-tft).

Problem

prune.engine._resolve_sample_bucket sized the deterministic-sample bucket by reaching for a BigQuery-only getattr(adapter, "_get_client") crack. SnowflakeAdapter has no _get_client (it exposes _get_num_rows via INFORMATION_SCHEMA.TABLES), so prune.scope: sample + prune.sample_strategy: oneshot raised PruneError on every Snowflake table — Snowflake oneshot prune was non-functional.

Fix

Add a vendor-neutral WarehouseAdapter.get_row_count(table) -> int | None seam, following the established materialise_sample / estimate_query_bytes precedent (base.py:107 deliberately prefers a concrete default-raise over a new abstract method, keeping the existing fake/stub adapters working unchanged):

• ABC default raises the new typed RowCountNotSupportedError (CLI tier 3) whose remediation points at prune.scope: full.
• BigQuery overrides it via the cached _get_table().num_rows.
• Snowflake overrides it via _get_num_rows → INFORMATION_SCHEMA.TABLES.ROW_COUNT.
• Engine routes _resolve_sample_bucket through the seam; the fail-loud unknown-count PruneError and BigQuery snapshot/behaviour parity are preserved.

RowCountNotSupportedError is wired through all required surfaces: warehouse.errors.__all__, warehouse.__init__, the CLI import + _EXCEPTION_TO_EXIT_CODE table (satisfying the 7th AST scan), and the errors-construction contract test.

Acceptance criteria

• WarehouseAdapter exposes a vendor-neutral row-count seam.
• _resolve_sample_bucket uses it (no _get_client getattr).
• Snowflake oneshot sample prune resolves the bucket and runs.
• BigQuery path unchanged (snapshot / behaviour parity — full suite green).
• Covered by a gated live @pytest.mark.snowflake oneshot test.

Tests

• ABC default-raise (test_base.py); BigQuery + Snowflake overrides via offline fakes; engine vendor-neutral routing regression (a non-BQ adapter with no _get_client resolves the bucket; scope=full skips the lookup; unknown count fails loud); gated live oneshot prune e2e (test_prune_drops_always_passes_not_null_live_oneshot_sample).
• Validation: ruff check ✓, ruff format --check ✓, pyright 0 errors ✓, pytest 2403 passed @ 97% ✓, pytest -m snowflake --no-cov 33 passed / 5 skipped (live skip cleanly) ✓.

Docs

docs/warehouse-adapter-ops.md and .claude/rules/warehouse-adapters.md updated to mark bd_1-scaffolding-tft fixed (oneshot now works alongside materialised).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Oneshot sample strategy is now available on live Snowflake.

• Bug Fixes

  • Sample-mode pruning on live Snowflake is no longer blocked.

• Documentation

  • Updated operations guide to reflect expanded Snowflake support and clarified remaining limitations.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (4)

• feature/.*
• bug/.*
• hotfix/.*
• feat/.*

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 48538f98-0daf-45ce-8312-51c0c2b62ec8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR fixes Snowflake prune.scope: sample + prune.sample_strategy: oneshot failures by introducing a vendor-neutral WarehouseAdapter.get_row_count(table) -> int | None seam and routing deterministic sample-bucket sizing through it (instead of the BigQuery-only _get_client crack).

Changes:

• Add WarehouseAdapter.get_row_count with a concrete default raise (RowCountNotSupportedError) and wire the new error through exports + CLI exit-code mapping.
• Implement get_row_count for BigQuery (cached Table.num_rows) and Snowflake (INFORMATION_SCHEMA.TABLES.ROW_COUNT via _get_num_rows).
• Update engine bucket sizing to use get_row_count and add/extend unit + gated live Snowflake tests covering the oneshot sample path.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/signalforge/warehouse/base.py	Adds the vendor-neutral get_row_count seam with default typed failure.
src/signalforge/warehouse/errors.py	Introduces RowCountNotSupportedError and exports it.
src/signalforge/warehouse/adapters/bigquery.py	Implements get_row_count using cached table metadata.
src/signalforge/warehouse/adapters/snowflake.py	Implements get_row_count via _get_num_rows / INFORMATION_SCHEMA.
src/signalforge/prune/engine.py	Routes _resolve_sample_bucket through adapter.get_row_count.
src/signalforge/cli/_helpers.py	Maps RowCountNotSupportedError to the CLI exit-code taxonomy.
src/signalforge/warehouse/__init__.py	Re-exports RowCountNotSupportedError as part of the public surface.
tests/warehouse/test_base.py	Adds contract test for ABC default raise behavior.
tests/warehouse/test_bigquery_unit.py	Adds unit tests for BigQuery get_row_count behavior.
tests/warehouse/test_snowflake_sampling.py	Adds unit tests for Snowflake get_row_count behavior.
tests/prune/test_engine.py	Adds regression tests proving _resolve_sample_bucket no longer requires _get_client.
tests/warehouse/test_errors.py	Updates error construction contract and __all__ count expectations.
tests/warehouse/test_snowflake_prune_live.py	Adds gated live e2e test certifying oneshot sample prune on Snowflake.
docs/warehouse-adapter-ops.md	Updates operator docs to reflect oneshot sample now works on Snowflake.
.claude/rules/warehouse-adapters.md	Updates adapter rules/findings to mark the issue fixed and document the new seam.

Comments suppressed due to low confidence (1)

src/signalforge/prune/engine.py:645

• The PruneError message here is now inconsistent with the new vendor-neutral get_row_count seam: it still says it “requires Table.num_rows” and always claims “the warehouse returned None”, even though this branch also triggers when num_rows == 0. This can mislead operators (especially for genuinely empty tables). Consider rewording to reference the row-count returned by adapter.get_row_count(...) and include whether it was None vs 0 (or interpolate the actual value) in the message/remediation.

    if num_rows is None or num_rows == 0:
        raise PruneError(
            f"sample-mode prune requires Table.num_rows for "
            f"{table_ref.qualified_name!r} but the warehouse returned None.",
            remediation=(
                "Verify the table is materialised and accessible. "
                "If it is genuinely empty (or num_rows is unavailable on the "
                "warehouse type), set `prune.scope: full` in signalforge.yml "
                "to bypass the deterministic-sample CTE."

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

tests/warehouse/test_bigquery_unit.py (1)

145-177: ⚡ Quick win

Mark new BigQuery row-count tests as unit-scoped.

Both new tests should be explicitly gated with @pytest.mark.unit.

Suggested change

+@pytest.mark.unit
 def test_get_row_count_returns_num_rows(
@@
+@pytest.mark.unit
 def test_get_row_count_returns_none_when_num_rows_unknown(

As per coding guidelines, tests/**/*.py should use @pytest.mark decorators for test gating, with unit-scoped tests explicitly marked.

🤖 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/warehouse/test_bigquery_unit.py` around lines 145 - 177, Add pytest
unit markers to the two new tests: decorate test_get_row_count_returns_num_rows
and test_get_row_count_returns_none_when_num_rows_unknown with `@pytest.mark.unit`
so they are explicitly unit-scoped; ensure pytest is imported (import pytest) at
top if not already present and place the decorator immediately above each
function definition.

tests/warehouse/test_base.py (1)

212-238: ⚡ Quick win

Add a unit marker to the new contract test.

Please decorate this new unit test with @pytest.mark.unit so it follows the test-gating convention.

Suggested change

+@pytest.mark.unit
 def test_get_row_count_default_impl_raises_not_supported() -> None:

As per coding guidelines, tests/**/*.py should use @pytest.mark decorators for test gating and keep unit-scoped tests explicitly marked.

🤖 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/warehouse/test_base.py` around lines 212 - 238, The test function
test_get_row_count_default_impl_raises_not_supported should be marked as a unit
test; add the `@pytest.mark.unit` decorator above the test definition (and ensure
pytest is imported in the file if not already) so the new contract test follows
the test-gating convention and is recognized as unit-scoped.

tests/warehouse/test_snowflake_sampling.py (1)

121-142: ⚡ Quick win

Add unit markers for the new Snowflake row-count tests.

Please add @pytest.mark.unit to both new tests to match test-gating standards.

Suggested change

+@pytest.mark.unit
 def test_get_row_count_returns_information_schema_row_count() -> None:
@@
+@pytest.mark.unit
 def test_get_row_count_returns_none_for_view_without_row_count() -> None:

As per coding guidelines, tests/**/*.py should use @pytest.mark decorators for test gating and keep unit tests explicitly marked.

🤖 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/warehouse/test_snowflake_sampling.py` around lines 121 - 142, Add the
pytest unit marker to both new tests by decorating
test_get_row_count_returns_information_schema_row_count and
test_get_row_count_returns_none_for_view_without_row_count with
`@pytest.mark.unit`; if pytest is not already imported in the file, add import
pytest at top. Ensure the decorator is placed immediately above each def so the
tests are gated as unit tests.

🤖 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 `@docs/warehouse-adapter-ops.md`:
- Around line 648-653: Update the earlier paragraph that still states
`prune.sample_strategy: oneshot` is blocked so it matches the later section
which says `oneshot` is fixed: locate the paragraph that mentions the oneshot
strategy being blocked and change the wording to indicate that `oneshot` is now
supported (when used with `safety: schema-only` + `prune.scope: full` or
`prune.scope: sample` with `prune.sample_strategy: materialised` or `oneshot`),
and similarly adjust the equivalent text around the other occurrence so both
places consistently reflect the fixed status.

In `@src/signalforge/prune/engine.py`:
- Around line 625-627: The fail-loud PruneError message currently says the count
"returned None" even when the value is 0; update the raise site that checks
row_count (the variable used to hold the table size) to include the actual value
(e.g., use f-string with row_count or explicitly say "returned None or 0") so
the error message accurately reflects both None and 0; locate the PruneError
raise (the code around the docstring and the check that raises PruneError when
row_count is None/0) and replace the static "returned None" text with a message
that interpolates row_count or mentions "None or 0".

In `@tests/prune/test_engine.py`:
- Around line 1453-1465: The stub class _RowCountOnlyAdapter currently silences
missing type signatures with "# type: ignore[no-untyped-def]" on dialect,
sample_rows, column_stats, and run_test_sql; replace those suppressions by
declaring each method with the explicit typed signature and return type expected
by WarehouseAdapter (e.g., dialect() -> DialectConstantType, sample_rows(self,
table: TableRef, n: int, *, partition_filter: Optional[PartitionFilter]) ->
List[RowType], column_stats(self, table: TableRef, column: ColumnRef) ->
ColumnStatsType, run_test_sql(self, sql: str, *, capture_failures: int = 0) ->
TestSqlResultType) so the stub matches the interface contract—remove the "#
type: ignore[no-untyped-def]" comments after updating signatures.

---

Nitpick comments:
In `@tests/warehouse/test_base.py`:
- Around line 212-238: The test function
test_get_row_count_default_impl_raises_not_supported should be marked as a unit
test; add the `@pytest.mark.unit` decorator above the test definition (and ensure
pytest is imported in the file if not already) so the new contract test follows
the test-gating convention and is recognized as unit-scoped.

In `@tests/warehouse/test_bigquery_unit.py`:
- Around line 145-177: Add pytest unit markers to the two new tests: decorate
test_get_row_count_returns_num_rows and
test_get_row_count_returns_none_when_num_rows_unknown with `@pytest.mark.unit` so
they are explicitly unit-scoped; ensure pytest is imported (import pytest) at
top if not already present and place the decorator immediately above each
function definition.

In `@tests/warehouse/test_snowflake_sampling.py`:
- Around line 121-142: Add the pytest unit marker to both new tests by
decorating test_get_row_count_returns_information_schema_row_count and
test_get_row_count_returns_none_for_view_without_row_count with
`@pytest.mark.unit`; if pytest is not already imported in the file, add import
pytest at top. Ensure the decorator is placed immediately above each def so the
tests are gated as unit tests.

🪄 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: 7479c087-1ede-4263-8a47-6c2cad299124

📥 Commits

Reviewing files that changed from the base of the PR and between d8db3f2 and 361d074.

📒 Files selected for processing (15)

• .claude/rules/warehouse-adapters.md
• docs/warehouse-adapter-ops.md
• src/signalforge/cli/_helpers.py
• src/signalforge/prune/engine.py
• src/signalforge/warehouse/__init__.py
• src/signalforge/warehouse/adapters/bigquery.py
• src/signalforge/warehouse/adapters/snowflake.py
• src/signalforge/warehouse/base.py
• src/signalforge/warehouse/errors.py
• tests/prune/test_engine.py
• tests/warehouse/test_base.py
• tests/warehouse/test_bigquery_unit.py
• tests/warehouse/test_errors.py
• tests/warehouse/test_snowflake_prune_live.py
• tests/warehouse/test_snowflake_sampling.py

[wjduenow]

PR Review Summary

All three CodeRabbit findings were real and are fixed in 05fa430.

Fixed (3 items)

File	Line	Issue	Fix
src/signalforge/prune/engine.py	637	Fail-loud message said "returned None" even when count was 0	Interpolate the actual observed value (None vs 0)
tests/prune/test_engine.py	1453+	_RowCountOnlyAdapter stub suppressed no-untyped-def	Gave every stub method an explicit ABC-matching signature + return type
docs/warehouse-adapter-ops.md	619-620	Earlier paragraph still said oneshot was blocked, contradicting the "Known limitations" section	Updated the earlier paragraph (and the test-coverage line) so all oneshot guidance is consistent

False positives (0 items)

Validation after fixes: ruff check ✓, ruff format --check ✓, pyright 0 errors ✓, pytest 2403 passed @ 97% ✓.