#122: Snowflake sampling + materialise_sample (plan) - #129
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements Snowflake deterministic sampling and materialisation by introducing shared cross-adapter run-id generation helpers, Snowflake exception mapping, connection-bound session lifecycle management, deterministic sample_rows SQL construction with sizing guards, CTAS-based materialise_sample with temp-table pinning, and run_test_sql with inline test wrapping. Complete test coverage includes a custom FakeSnowflakeConnection fake and comprehensive suites for sampling, materialisation, cleanup, and cross-adapter plumbing. ChangesSnowflake Sampling + Materialisation Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…apper (#122 US-001) Foundations for Snowflake sampling — pure plumbing, no adapter behaviour. - Relocate _compute_run_id / _canonical_partition_filter / _hash_session_id VERBATIM out of adapters/bigquery.py into a new shared signalforge/warehouse/_sample_id.py (DEC-008) so BigQuery and Snowflake produce byte-identical run_ids. BigQuery imports them; recipe bytes unchanged (all existing materialise-sample / prune snapshots byte-identical). - Add `description` to _SnowflakeCursorProtocol (DEC-010, DB-API 2.0 column descriptors) so the future adapter builds dict rows without a DictCursor. - Add map_snowflake_exception(exc, *, context) to _snowflake_client.py (DEC-009): lazy snowflake.connector.errors import (one-shim-per-vendor), minimal v0.2 taxonomy — ProgrammingError -> QuerySyntaxError, auth/forbidden -> WarehouseAuthError, else passthrough. Mirrors map_bq_exception shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion lifecycle (#122 US-002) Wire the SnowflakeAdapter connection seam and fail-soft __exit__ cleanup, plus a hand-rolled FakeSnowflakeConnection test double. Sampling / materialise / run_test_sql stay NotImplementedError (US-003/US-004). - tests/warehouse/_fake_snowflake.py: explicit FakeSnowflakeConnection + cursor satisfying the _SnowflakeClientProtocol / _SnowflakeCursorProtocol; expect_execute() queues round-trips, close_raises drives the cleanup-failure path. No MagicMock (testing-signal.md). - adapters/snowflake.py: injectable connection= kwarg + lazy _get_connection() build (DEC-001), connection-bound _active_session state (DEC-002), and a fail-soft _cleanup_active_session() that closes the connection (reaping session-scoped temp tables), swallows-and-warns on failure with an operator-actionable Snowflake-shaped WARNING naming the raw session_id + server-side reap fallback (DEC-003/DEC-014); success logs the hashed id only. __repr__ unchanged (account + warehouse only). - tests: new test_snowflake_lifecycle.py covers injection, lazy build, __repr__ redaction regression, single-close + idempotent second __exit__, hashed-id INFO, and the WARNING failure path; updated the stub's context-manager test for the now-real cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…le_rows (#122 US-003) Implements deterministic hash-mod sampling for Snowflake, mirroring BigQuery's sample_rows semantics (DEC-005/006/009/010): - _get_num_rows: ROW_COUNT from INFORMATION_SCHEMA.TABLES, case-insensitive, escaped string literals, CURRENT_DATABASE() fallback when project is None. - Fail-loud sizing identical to BigQuery: UnknownTableSizeError / SamplingRequiresPartitionFilterError; bucket=1000 fallback on unknown size with a filter; bucket=max(num_rows//n,1) otherwise. _LARGE_TABLE_THRESHOLD re-declared (100M) to avoid importing the BigQuery adapter. - SQL reuses SNOWFLAKE_DIALECT.sample_row_hash_expr (ABS(HASH(*))) + the timestamp/date literal templates so it stays byte-consistent with the prune compiler's sample CTE; per-component double-quoting; ORDER BY for deterministic LIMIT truncation. - Tuple fetchall() rows shaped into dicts via cursor.description (DEC-010); SDK exceptions routed through map_snowflake_exception (DEC-009). column_stats / run_test_sql / materialise_sample stay NotImplementedError. Removed the now-stale test_sample_rows_raises_not_implemented stub test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t_sql (#122 US-004) Implement the two remaining sampling-consumer surfaces on SnowflakeAdapter: - materialise_sample: CTAS into a session-scoped TEMPORARY TABLE (_sf_sample_<run_id>, run_id from the shared _sample_id recipe so it is byte-identical to BigQuery; DEC-008), colocated with + fully-qualified via the source DB/schema (DEC-007), deterministic MOD(ABS(HASH(*)),bucket)<1 + ORDER BY ABS(HASH(*)) read from SNOWFLAKE_DIALECT (DEC-006), pins the live connection as _active_session so a follow-up run_test_sql reaches the temp table (DEC-002). SDK failures route through map_snowflake_exception then wrap in MaterialisationFailedError (DEC-009); INFO log emits the hashed session id only (DEC-003). n<=0 -> ValueError. - run_test_sql: validate_test_sql -> COUNT(*) wrap (ARRAY_AGG(OBJECT_CONSTRUCT(*)) sample capture when capture_failures>0) on the active connection, returns a typed TestResult; case-insensitive alias resolution for Snowflake's upper-folded FAILURES/SAMPLES columns (DEC-004). - Factor the fail-loud sizing pathway into a shared _resolve_sample_bucket helper reused by sample_rows + materialise_sample (DEC-005, no duplicated logic). column_stats stays NotImplementedError (#118); estimate_query_bytes stays the ABC not-supported degrade (#123). Remove the now-stale run_test_sql / materialise_sample NotImplemented stub tests; add tests/warehouse/test_snowflake_materialise.py pinning CTAS shape, fully-qualified temp TableRef, _active_session pinning, run_test_sql reachability on one connection, the #116 substitution AC (compiler emits the temp-table name, NOT the source), and the failure modes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…US-005) - BUG: _cleanup_active_session no longer nulls self._connection (discarded an injected fake / forced a real lazy-rebuild on re-entry; mirrors BigQuery which resets only _active_session, never the client). - #116 substitution test now exercises the bypassable path: CandidateTestCustomSQL with {{ this }} at scope="full" (not_null trivially FROMs table_ref and can never bypass substitution). - Add num_rows==0 sizing test (was untested; mirrors the None pathway). - Fix stale/misleading docstrings (module/class "still raises", _session_started_at "auto-expire" text Snowflake never emits). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ng conventions (#122 US-006) - warehouse-adapters.md: new "Snowflake sampling + connection-bound session" section (connection-as-session, dialect-field reuse, INFORMATION_SCHEMA sizing, shared _sample_id hoist, Snowflake-shaped fail-soft cleanup); update the cleanup-boundary forward-note + Reference. - CLAUDE.md: public-API surface note for the implemented sampling methods. - plan: mark complete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov flagged 15 uncovered lines in adapters/snowflake.py (93%). Add tests for the previously-untested branches — all now 100%: - _execute / _execute_to_dicts / run_test_sql SDK-error mapping: both the mapped (ProgrammingError -> QuerySyntaxError) and unmapped-passthrough (mapped is exc -> raise original) branches, on size + sample + count queries. - _rows_to_dicts dict-row passthrough (DictCursor-style mapping rows). - materialise_sample with a PartitionFilter (CTAS WHERE rendering). - _get_connection lazy real-client build when no connection= is injected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements deterministic Snowflake sampling for SnowflakeAdapter: sample_rows (HASH-mod), materialise_sample (session TEMP TABLE), and run_test_sql (COUNT(*) wrap), plus a fail-soft __exit__ cleanup and a shared cross-adapter _sample_id helper module hoisted from BigQuery.
Changes:
- Implements
sample_rows/materialise_sample/run_test_sqlonSnowflakeAdapter, with INFORMATION_SCHEMA-based fail-loud sizing and a connection-bound_active_session. - Hoists
_compute_run_id/_hash_session_id/_canonical_partition_filterto a sharedsignalforge.warehouse._sample_idmodule so BigQuery and Snowflake produce byte-identical run_ids. - Extends
_SnowflakeCursorProtocolwithdescriptionand adds a minimalmap_snowflake_exceptionmapper in the SDK shim.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/signalforge/warehouse/adapters/snowflake.py | Implements sampling/materialise/run_test_sql + connection seam + fail-soft cleanup. |
| src/signalforge/warehouse/_sample_id.py | New shared module with relocated deterministic-sample-id helpers. |
| src/signalforge/warehouse/adapters/bigquery.py | Removes local helpers and imports from _sample_id. |
| src/signalforge/warehouse/adapters/_snowflake_client.py | Adds description to cursor protocol and map_snowflake_exception. |
| tests/warehouse/_fake_snowflake.py | New hand-rolled fake with expect_execute API. |
| tests/warehouse/test_snowflake_sampling.py | Sample_rows behavior, sizing branches, partition rendering, error mapping tests. |
| tests/warehouse/test_snowflake_materialise.py | Materialise + run_test_sql + #116 substitution coverage. |
| tests/warehouse/test_snowflake_lifecycle.py | Connection seam, repr redaction, fail-soft cleanup tests. |
| tests/warehouse/test_snowflake_stub.py | Removes stub assertions for implemented methods. |
| tests/warehouse/test_sample_id.py | Tests for relocated helpers + cursor protocol + error mapper. |
| tests/warehouse/test_snowflake_client.py | Adds description and map_snowflake_exception to fake/exports. |
| plans/super/122-snowflake-sampling.md | New plan document. |
| CLAUDE.md, .claude/rules/warehouse-adapters.md | Documentation updates capturing #122 conventions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @wjduenow. * #129 (comment) The following files were modified: * `src/signalforge/warehouse/_sample_id.py` * `src/signalforge/warehouse/adapters/_snowflake_client.py` * `src/signalforge/warehouse/adapters/bigquery.py` * `src/signalforge/warehouse/adapters/snowflake.py` * `tests/warehouse/_fake_snowflake.py` * `tests/warehouse/test_sample_id.py` * `tests/warehouse/test_snowflake_client.py` * `tests/warehouse/test_snowflake_lifecycle.py` * `tests/warehouse/test_snowflake_materialise.py` * `tests/warehouse/test_snowflake_sampling.py` * `tests/warehouse/test_snowflake_stub.py`
…lookup Copilot PR-review catch: CURRENT_DATABASE().INFORMATION_SCHEMA.TABLES is invalid Snowflake — CURRENT_DATABASE() is a scalar function, not a namespace qualifier, and would fail on a live account (the hand-rolled fake matched it by regex; live execution is deferred to #124). When table.project is None, leave the lookup unqualified (INFORMATION_SCHEMA.TABLES), which Snowflake resolves against the session's current database. Update the test + rule + plan notes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR Review SummaryFixed (1 item)
False Positives (0 items)Good catch — the hand-rolled fake matched the bad SQL by regex, so the unit tests passed; this would only have surfaced in #124's live-Snowflake run. CI green (lint-test 3.11/3.12/3.13, docs-build); Codecov patch coverage 100%. |
Summary
Super plan for #122 — Snowflake deterministic
sample_rows(HASH-mod) +materialise_sample(session TEMP TABLE). Part of epic #118.Phase: detailing (awaiting approval)
Stories: 4 implementation + Quality Gate + Patterns & Memory
Decisions: 10 (DEC-001 … DEC-010)
Key decisions
connection=), connection-bound_active_session(Adopt Q4=C (temp-table-materialised sample) for v0.2 sample-mode prune #22 DEC-002 adapted — the connector holds the session, nosession_idrouting dance).sample_rowsreusesSNOWFLAKE_DIALECTfields (ABS(HASH(*)), literal templates) for byte-parity with the feat: prune compiler Snowflake dialect support (quote_char, QUALIFY, identifier_case=upper) #121 prune compiler; sizing via INFORMATION_SCHEMA.TABLES.ROW_COUNT (mirrors BigQuery fail-loud guards).run_test_sqlimplemented in feat: Snowflake deterministic sampling (sample_rows HASH-mod) + materialise_sample (TEMP TABLE) #122 (AC requires temp-table reachability);column_statsstays stubbed.__exit__cleanup reshaped for Snowflake's session-local temp tables (nobq-style manual command; server-side session reap is the durable fallback)._sample_idmodule hoisted from the BigQuery adapter so therun_idrecipe is byte-identical across vendors.FakeSnowflakeClientnow (fakesnow harness + live e2e + ops docs remain test+docs: fakesnow harness + FakeSnowflakeClient + gated live e2e (TPCH_SF1) + ops docs #124).Plan document
See
plans/super/122-snowflake-sampling.md.Next steps
Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests