#258: Snowflake column_stats parity with Databricks - #262
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSnowflake ChangesSnowflake column_stats parity
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)
✨ Finishing Touches📝 Generate docstrings
Comment |
…_execute_to_dicts/run_test_sql) Wrap the execute/fetch in try/finally cursor.close() for the three cursor-opening methods that previously leaked server-side handles, mirroring _execute_scalar (the one already-correct path) and the Databricks PR #257 shape. _execute_to_dicts uses an OUTER try/finally (close) wrapping an INNER try/except (exception mapping) so the cursor closes AFTER _rows_to_dicts reads cursor.description. run_test_sql likewise closes only after description + rows are captured into locals. TDD: extend FakeSnowflakeConnection to track vended cursors (.cursors) and expose per-cursor .closed, then six new tests in tests/warehouse/ test_snowflake_adapter.py assert closed on both success and failure paths. Traces to DEC-007 of plans/super/258-snowflake-column-stats.md.
… catalog pre-filter) Replace the NotImplementedError stub with a BigQuery-style context-manager batched column_stats (issue #258 US-002; DEC-001..DEC-006): - __enter__/__exit__ open/reset per-table pending + results caches, coexisting with the connection-session state. - column_stats: validate identifier -> DEC-025 with-block guard -> cache hit -> queue + flush. - _flush_column_stats_batch: one INFORMATION_SCHEMA.COLUMNS catalog lookup (mirroring _get_num_rows escaping) for the data_type field + MIN/MAX skip decision, then ONE aggregate for every queued column (index-suffixed aliases; COUNT(*)-COUNT null count per DEC-005; MIN/MAX gated per DEC-003). - _get_column_types keyed by lower(COLUMN_NAME); a requested column absent from the catalog raises ColumnNotFoundError before the aggregate. - _COMPLEX_SNOWFLAKE_TYPES {ARRAY,OBJECT,VARIANT,GEOGRAPHY,GEOMETRY} + _is_complex_snowflake_type (BINARY orderable); Decimal->float min/max (DEC-004). Full default-suite unit coverage via the hand-fake (no drift detector - ColumnStats is frozen/in-process). Drops the obsolete stub NotImplementedError test.
…README/CHANGELOG)
…calar + complex skip-set + aggregate-only smoke)
…e column_stats Add five fakesnow-executed offline tests driving the real SnowflakeAdapter column_stats path end-to-end against DuckDB (US-003, DEC-008): - scalar NUMBER column: COUNT / COUNT(DISTINCT) / COUNT(*)-COUNT(col) null count / MIN / MAX execute; Decimal min/max coerced to float (DEC-004) - INFORMATION_SCHEMA.COLUMNS catalog lookup (_get_column_types) round-trip — fakesnow returns consumable Snowflake DATA_TYPE strings, so this is a full EXECUTION assertion (no parse-only degrade needed) - VARCHAR string MIN/MAX bounds pass through unchanged - VARIANT complex-type MIN/MAX skip decision (DEC-003) executes; live cert owns the analysis-time-rejection validation (#227 lesson) - all-NULL scalar column: null-count arithmetic + empty MIN/MAX None path Document in the module docstring that column_stats is fully executable under fakesnow (no parse-only degrade) and that ColumnStats needs NO drift detector (frozen, in-process, never read back from disk).
…INARY/TIME), doc GEOGRAPHY DISTINCT limit QG review (2 independent lenses) found BINARY (bytearray) and TIME (datetime.time) column MIN/MAX return types are outside ColumnStats.min/max's union, raising a non-WarehouseError ValidationError that fails the whole batch at the panic tier. Fix: _coerce_min_max nulls any out-of-union value (return-type analogue of the skip-set). Add BINARY/TIME coercion tests (offline + gated live columns). Document the pre-existing GEOGRAPHY/GEOMETRY COUNT(DISTINCT) limitation.
…ts to shipped (#258) Update warehouse-adapters.md: flip the 'column_stats stays NotImplementedError' / 'parity is the open follow-up #258' statements to shipped; add the § 'Snowflake column_stats (issue #258)' subsection capturing the two-tier MIN/MAX defence (SQL-raise skip-set vs out-of-union return-type coercion net), the GEOGRAPHY DISTINCT limitation, and the deferred live cert.
wjduenow
changed the title
wjduenow
marked this pull request as ready for review
July 2, 2026 19:09
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
tests/cli/test_e2e_snowflake_smoke.py (1)
348-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared profile/config setup into a helper.
The docstring notes this setup is "the same shape as the schema-only sibling" test — profile generation from env vars and
signalforge.ymlwriting look like they'd be duplicated across both e2e tests. Extracting a shared helper (e.g.,_write_snowflake_profile(project_dir, database, schema)) would reduce duplication as more Snowflake e2e smoke tests are added.🤖 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/cli/test_e2e_snowflake_smoke.py` around lines 348 - 397, The Snowflake e2e smoke test is duplicating profile and config setup that already matches the schema-only sibling. Extract the shared “write profile + write signalforge config” logic into a helper such as _write_snowflake_profile(...) and/or _write_snowflake_signalforge_config(...), then call it from this test and the schema-only test so the env-var mapping, yaml.safe_dump write, and common Snowflake config stay in one place.
🤖 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 @.claude/rules/warehouse-adapters.md:
- Line 49: Update the Snowflake cursor-leak note in the warehouse adapter docs
so it matches the current behavior: the warning should no longer say Snowflake
only closes cursors in `_execute_scalar`, since `_execute`, `_execute_to_dicts`,
and `run_test_sql` now also close cursors in `finally`. Edit the existing note
in the warehouse-adapters guidance to reflect the new convention that these
Snowflake methods already handle cursor cleanup, and remove the outdated
latent-leak warning so future maintenance isn’t misled.
In `@CHANGELOG.md`:
- Around line 11-13: The release note overstates Snowflake aggregate-only
support by implying the last unsupported combination is gone, but the documented
contract still excludes the `GEOGRAPHY` / `GEOMETRY` `COUNT(DISTINCT)` path.
Update the changelog entry in the `SnowflakeAdapter` / `column_stats` section to
qualify the claim so it only states support for the implemented aggregate-only
behavior and clearly preserves the unorderable-type exception handled by
`MIN`/`MAX` skipping and the existing `COUNT(DISTINCT)` limitation.
In `@docs/warehouse-adapter-ops.md`:
- Around line 726-728: The wrapped Snowflake ops note in the warehouse adapter
docs is starting a line with the issue marker text, which triggers MD018. Reflow
the sentence in the affected paragraph around the `column_stats` mention, or
escape the hash so the wrapped line does not begin with `#258` and the doc
remains lint-clean.
In `@README.md`:
- Line 95: Update the warehouse adapter capability summary in README so it no
longer claims Snowflake supports every safety/scope/strategy combination without
caveats. In the paragraph describing the Snowflake adapter, keep the reference
to `aggregate-only` support but qualify it with the documented `COUNT(DISTINCT)`
geospatial limitation for `GEOGRAPHY` and `GEOMETRY`, and make the wording
consistent with the ops docs.
- Around line 513-515: The wrapped sentence in README.md leaves “#258” at the
start of a line, which trips the markdown linter. Reflow the surrounding text in
that paragraph so the reference stays inline, or escape the hash so it is not
interpreted as a heading. Use the nearby “column_stats” / “safety:
aggregate-only” sentence to locate the affected wrap.
In `@src/signalforge/warehouse/adapters/snowflake.py`:
- Around line 1207-1283: The column-stats flush in the Snowflake adapter leaves
failed columns stuck in the pending queue, so one bad or unsupported column
poisons later `column_stats()` calls for the same table. Update the flush path
in the method that builds and runs the aggregate (the one using
`_column_stats_pending`, `_get_column_types`, and `_execute_to_dicts`) to drain
or snapshot the pending batch before resolving types and executing the query,
then only re-queue anything still needed after the attempt. Make sure failures
like `ColumnNotFoundError` or aggregate-query errors do not keep the stale
columns in `_column_stats_pending[table]`, so later valid columns can be
processed independently.
- Around line 1092-1144: The batching logic in `SnowflakeAdapter.column_stats()`
still flushes immediately, so each `aggregate_columns()` call drains the pending
queue one column at a time instead of batching per table. Update the
`column_stats`/`_flush_column_stats_batch` flow so requests are accumulated
across calls within the active `with adapter:` block and only flushed when the
batch is actually needed, preserving one catalog lookup and one aggregate query
per table. Use the existing `self._column_stats_pending` and
`self._column_stats_results` caches to keep later reads served from the batch
result rather than triggering another flush.
In `@tests/warehouse/test_snowflake_columnstats_live.py`:
- Around line 219-249: The setup in the Snowflake column stats test can fail
after CREATE TABLE and before teardown is entered, leaving orphaned tables
behind. Move the engineered-table setup in the test around the same try/finally
that already handles cleanup, so failures in the INSERT/SELECT UNION ALL path
still reach the DROP TABLE logic. Use the existing test flow around
setup_adapter, quoted, and the teardown block to keep creation and cleanup under
one protected scope.
---
Nitpick comments:
In `@tests/cli/test_e2e_snowflake_smoke.py`:
- Around line 348-397: The Snowflake e2e smoke test is duplicating profile and
config setup that already matches the schema-only sibling. Extract the shared
“write profile + write signalforge config” logic into a helper such as
_write_snowflake_profile(...) and/or _write_snowflake_signalforge_config(...),
then call it from this test and the schema-only test so the env-var mapping,
yaml.safe_dump write, and common Snowflake config stay in one place.
🪄 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: 811dde23-085e-4810-babe-2c6926b45c84
📒 Files selected for processing (12)
.claude/rules/warehouse-adapters.mdCHANGELOG.mdREADME.mddocs/warehouse-adapter-ops.mdplans/super/258-snowflake-column-stats.mdsrc/signalforge/warehouse/adapters/snowflake.pytests/cli/test_e2e_snowflake_smoke.pytests/warehouse/_fake_snowflake.pytests/warehouse/test_snowflake_adapter.pytests/warehouse/test_snowflake_adapter_fakesnow.pytests/warehouse/test_snowflake_columnstats_live.pytests/warehouse/test_snowflake_stub.py
There was a problem hiding this comment.
Pull request overview
Implements Snowflake column_stats to reach parity with Databricks/BigQuery and unlock safety: aggregate-only on Snowflake, while also hardening Snowflake cursor lifecycle management and updating docs/tests to cover the new behavior.
Changes:
- Implement
SnowflakeAdapter.column_statswith context-manager batching, catalog pre-filtering, complex-type MIN/MAX skipping, and min/max coercion for out-of-union Python return types. - Fix cursor leaks in Snowflake adapter helpers (
_execute,_execute_to_dicts,run_test_sql) and extend fakes/tests to assert cursor closure. - Add/extend offline + gated-live Snowflake test coverage and update user-facing documentation (README/ops doc/changelog) plus internal adapter rules.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/signalforge/warehouse/adapters/snowflake.py |
Adds Snowflake column_stats implementation with batching + catalog pre-filter; fixes cursor cleanup. |
tests/warehouse/_fake_snowflake.py |
Tracks issued cursors and exposes closed for leak-regression assertions. |
tests/warehouse/test_snowflake_adapter.py |
New unit tests for cursor closure and column_stats behavior/query shape/error mapping. |
tests/warehouse/test_snowflake_adapter_fakesnow.py |
Adds fakesnow-executed column_stats execution tests over engineered rows. |
tests/warehouse/test_snowflake_columnstats_live.py |
New gated live certification for complex-type skip-set and coercion behavior. |
tests/cli/test_e2e_snowflake_smoke.py |
Adds gated live E2E smoke for signalforge generate with aggregate-only on Snowflake TPCH. |
tests/warehouse/test_snowflake_stub.py |
Removes legacy “column_stats NotImplemented” stub test and updates module commentary. |
docs/warehouse-adapter-ops.md |
Updates Snowflake ops guidance to reflect aggregate-only support + known limitations. |
README.md |
Updates supported-warehouses narrative to include Snowflake column_stats support. |
CHANGELOG.md |
Adds unreleased entry describing Snowflake column_stats parity + cursor hygiene changes. |
plans/super/258-snowflake-column-stats.md |
New plan document capturing decisions and implementation/testing breakdown. |
.claude/rules/warehouse-adapters.md |
Updates internal adapter rules/memory with Snowflake column_stats parity details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- CRITICAL (CodeRabbit): drain column_stats pending batch UP FRONT so a failed column (ColumnNotFoundError / GEOGRAPHY COUNT(DISTINCT)) no longer poisons every subsequent column of the same table within the with-block. + regression test. - Live test: move CREATE/INSERT inside the outer try/finally (no orphaned table on setup failure). - Docs/README/CHANGELOG: qualify aggregate-only with the GEOGRAPHY/GEOMETRY COUNT(DISTINCT) limitation; soften 'certified'/'confirmed' to maintainer-run gated cert. - MD018: reflow lines so #258 is not at a wrapped-line start (ops doc + README). - warehouse-adapters.md: update the stale Snowflake cursor-leak note (only materialise_sample leaks now).
Owner
Author
PR Review Summary (CodeRabbit + Copilot)Addressed in Fixed (10 items)
False Positives (1 item)
|
… cert on MFA accounts) The #120 profile model + from_profile + SnowflakeAdapter.__init__ already parsed and stored private_key_path / private_key_passphrase / authenticator, but make_real_client dropped them at the connect() call (they were never used). Thread them through: key-pair auth when private_key_path is set (private_key_file [+ _pwd]), else password; authenticator passed through when set. This is the non-interactive path MFA-enforced accounts require. The gated live cert (_make_adapter + _skip_reason) now accepts key-pair OR password auth.
…air auth) Ran the gated column_stats live cert against a real Snowflake warehouse (key-pair / JWT auth — the restored account enforces MFA, which the headless password path can't satisfy). Result: scalar min/max populate; ARRAY/OBJECT/VARIANT return min=max=None without raising (DEC-003 skip-set confirmed complete); BINARY/TIME coerce to None. Since the cert profiles ARRAY/OBJECT/VARIANT (emitting COUNT(DISTINCT) on them) and passed, COUNT(DISTINCT) on semi-structured types does NOT raise — only GEOGRAPHY/GEOMETRY remain the documented limit. Docs/rules updated from 'pending a maintainer run' to 'live-certified'.
…ir into e2e smoke Exposed by the aggregate-only e2e: generate with safety=aggregate-only uses one adapter across two 'with adapter:' blocks (safety-aggregate column_stats, then prune_tests). Snowflake's __exit__ closes the connection (the connection IS the session) but #122 didn't null self._connection, so the 2nd block reused the closed connection -> 250002 (08003): Connection is closed. Fix: track self._owns_connection (True only on lazy build); cleanup nulls _connection after close only when owned, so the next block rebuilds; injected fakes stay intact. + 2 lifecycle regression tests. Also wire key-pair (JWT) auth into the e2e smoke profiles.yml (SNOWFLAKE_PRIVATE_KEY_PATH), so the aggregate-only generate e2e is now live-certified against a real MFA-enforced Snowflake account.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli/test_e2e_snowflake_smoke.py (1)
213-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated profile-construction block across both e2e tests.
The
outputdict build, optionalroleinjection,_auth_profile_fields()merge, andprofiles.ymlwrite (lines 213-226 and 381-394) are near-identical betweentest_e2e_signalforge_generate_against_tpch_sf1and the newtest_e2e_signalforge_generate_aggregate_only_against_tpch_sf1. A future auth/profile-shape change (e.g. adding OAuth) would need updating in two places with drift risk.♻️ Suggested consolidation
+def _write_snowflake_profile(project_dir: Path) -> None: + output: dict[str, object] = { + "type": "snowflake", + "account": os.environ["SNOWFLAKE_ACCOUNT"], + "user": os.environ["SNOWFLAKE_USER"], + "warehouse": os.environ["SNOWFLAKE_WAREHOUSE"], + "database": "SNOWFLAKE_SAMPLE_DATA", + "schema": "TPCH_SF1", + "threads": 1, + } + if role := os.environ.get("SNOWFLAKE_ROLE"): + output["role"] = role + output.update(_auth_profile_fields()) + profile = {"tpch": {"target": "dev", "outputs": {"dev": output}}} + (project_dir / "profiles.yml").write_text(yaml.safe_dump(profile, sort_keys=False))Then call
_write_snowflake_profile(project_dir)in both tests instead of repeating the block.Also applies to: 381-394
🤖 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/cli/test_e2e_snowflake_smoke.py` around lines 213 - 226, The Snowflake profile setup is duplicated across the two e2e tests, so consolidate the repeated `output` construction, optional `SNOWFLAKE_ROLE` handling, `_auth_profile_fields()` merge, and `profiles.yml` write into a shared helper such as `_write_snowflake_profile(project_dir)`. Update both `test_e2e_signalforge_generate_against_tpch_sf1` and `test_e2e_signalforge_generate_aggregate_only_against_tpch_sf1` to call that helper instead of inlining the same block.
🤖 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.
Nitpick comments:
In `@tests/cli/test_e2e_snowflake_smoke.py`:
- Around line 213-226: The Snowflake profile setup is duplicated across the two
e2e tests, so consolidate the repeated `output` construction, optional
`SNOWFLAKE_ROLE` handling, `_auth_profile_fields()` merge, and `profiles.yml`
write into a shared helper such as `_write_snowflake_profile(project_dir)`.
Update both `test_e2e_signalforge_generate_against_tpch_sf1` and
`test_e2e_signalforge_generate_aggregate_only_against_tpch_sf1` to call that
helper instead of inlining the same block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98fe3af4-935f-4628-8220-fab84bdc434e
📒 Files selected for processing (4)
.claude/rules/warehouse-adapters.mdsrc/signalforge/warehouse/adapters/snowflake.pytests/cli/test_e2e_snowflake_smoke.pytests/warehouse/test_snowflake_adapter.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/warehouse/test_snowflake_adapter.py
- .claude/rules/warehouse-adapters.md
- src/signalforge/warehouse/adapters/snowflake.py
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
Implements
SnowflakeAdapter.column_stats, enablingsafety: aggregate-onlyon Snowflake (parity with Databricks + BigQuery). Closes #258.This PR contains the full implementation + tests + docs (not just the plan — the plan doc under
plans/super/258-*.mdis included for reference, phasedevolved).Changes
column_stats(full BigQuery-style batching + catalog pre-filter): Snowflake has no runtimetypeof(), so it reads types fromINFORMATION_SCHEMA.COLUMNS.DATA_TYPEup front and pre-filters MIN/MAX for unorderable types.{ARRAY,OBJECT,VARIANT,GEOGRAPHY,GEOMETRY}for SQL-analysis-raise types +_coerce_min_maxnet nulling out-of-union return types (BINARY→bytearray,TIME→datetime.time,NUMBER→float)._execute/_execute_to_dicts/run_test_sqlnow close intry/finally(DEC-007).COUNT(DISTINCT)limitation documented.Testing
fakesnowexecution +sqlglotparse — all in the default suite (4502 passed).@pytest.mark.snowflake) written; pending a maintainer run (Snowflake trial account currently suspended) — it validates the DEC-003 skip-set completeness + DISTINCT-on-semi-structured behaviour.Compounding Update
.claude/rules/warehouse-adapters.md§ "Snowflakecolumn_stats(issue Decide whether Snowflake should backfill column_stats for parity with Databricks #258)" — the two-tier MIN/MAX defence pattern (return-type ≠ SQL-runs).Summary by CodeRabbit
column_statsforsafety: aggregate-only, aligning with Databricks parity.column_statsaccuracy (counts, distincts, nulls, min/max) including correct skipping/coercion for complex/unsupported types.GEOGRAPHY/GEOMETRYexception and#258status.aggregate-only column_statsand cursor-closure checks.