diff --git a/.claude/rules/warehouse-adapters.md b/.claude/rules/warehouse-adapters.md index 7d61a6fd..90cdbd19 100644 --- a/.claude/rules/warehouse-adapters.md +++ b/.claude/rules/warehouse-adapters.md @@ -44,15 +44,30 @@ The ABC is warehouse-agnostic. v0.2 Snowflake/Postgres slot under `adapters/` wi - **Fold-then-quote parity, applied to the partition-filter column too (DEC-008).** `_quote`/`_quote_identifier`/`_fold` lower-fold then backtick-quote per component, identical to the prune compiler (the #124 CREATE-vs-REFERENCE lesson) — and `_render_partition_filter` folds the PF column, which the **Snowflake adapter does NOT** (a latent Snowflake divergence the Databricks code avoids). - **`materialise_sample` = qualified `CREATE OR REPLACE TABLE` + explicit DROP at cleanup (DEC-004, CORRECTED by #226's live cert).** The #224 plan assumed a qualified `CREATE TEMPORARY TABLE .._sf_sample_` (mirroring Snowflake). #226's live cert proved Databricks rejects a *qualified* temp name (`[TEMP_TABLE_CREATION_REQUIRES_SINGLE_PART_NAME]`), and a `TableRef` cannot express a bare single-part name (`dataset` is required) — so the adapter materialises into a **real** `CREATE OR REPLACE TABLE .._sf_sample_` colocated with the source. A real table does NOT auto-reap with the session, so the adapter appends each materialised `TableRef` to `self._materialised_tables` **BEFORE issuing the CTAS** (a real `CREATE OR REPLACE TABLE` can commit before `cursor.execute` returns — client read-timeout / network blip — so post-execute tracking could orphan an untracked real table with no DROP; `DROP IF EXISTS` is a harmless no-op if the table never landed) and drops each at `_cleanup_active_session`. `run_id` via the shared `_compute_run_id`; returns the qualified `TableRef`; `MaterialisationFailedError(cause=mapped)` on cursor error. The qualified-ref shape still lets the prune compiler's Dialect-quoting consume it with **zero TableRef/compiler change**. **Lesson: a vendor that rejects a qualified TEMP name forces a real table, and a real table is a cleanup liability — track-before-create (not after) and DROP at the cleanup boundary.** - **`run_test_sql` capture: `to_json(struct(*))` returns JSON STRINGS → `json.loads` per row (DEC-007).** Per-row `to_json(struct(*))` + `LIMIT k` (NOT `collect_list`, avoiding `ARRAY` marshalling assumptions) — **distinct from Snowflake's single-VARIANT `OBJECT_CONSTRUCT` decode** (one outer `json.loads`); here each row's column is its own JSON string needing its own decode. -- **`column_stats` shipped AHEAD of Snowflake (DEC-011).** A single aggregate (`COUNT`/`COUNT(DISTINCT)`/`COUNT_IF(... IS NULL)`/`MIN`/`MAX`/`MAX(typeof(col))` for `data_type`), no context-manager batching. Snowflake's parity is the open follow-up **[#258](https://github.com/wjduenow/SignalForge/issues/258)**. **Complex-type MIN/MAX now honours the `ColumnStats` contract (resolved by #227 — see the follow-ups § below).** BigQuery skips `MIN`/`MAX` (→ `None`) for complex types because it reads the schema *before* building the query; Databricks derives `data_type` inline via `typeof()` in the same aggregate, so it can't omit MIN/MAX up front. #227 closes the gap in two paths: **orderable** complex types (`array`/`struct`/`binary`) return a MIN/MAX value → a pure post-process nulls them; **non-orderable** types (`map`/`variant`) reject MIN/MAX at Spark *analysis* time (`INVALID_ORDERING_TYPE`, failing the whole aggregate) → the call catches that specific `QuerySyntaxError` and re-runs a **reduced** aggregate (no MIN/MAX), returning `None` bounds. The scalar happy path stays a single query. +- **`column_stats` shipped AHEAD of Snowflake (DEC-011).** A single aggregate (`COUNT`/`COUNT(DISTINCT)`/`COUNT_IF(... IS NULL)`/`MIN`/`MAX`/`MAX(typeof(col))` for `data_type`), no context-manager batching. Snowflake reached parity in **[#258](https://github.com/wjduenow/SignalForge/issues/258)** (full BigQuery-style batching + catalog pre-filter — see § "Snowflake `column_stats` (issue #258)"). **Complex-type MIN/MAX now honours the `ColumnStats` contract (resolved by #227 — see the follow-ups § below).** BigQuery skips `MIN`/`MAX` (→ `None`) for complex types because it reads the schema *before* building the query; Databricks derives `data_type` inline via `typeof()` in the same aggregate, so it can't omit MIN/MAX up front. #227 closes the gap in two paths: **orderable** complex types (`array`/`struct`/`binary`) return a MIN/MAX value → a pure post-process nulls them; **non-orderable** types (`map`/`variant`) reject MIN/MAX at Spark *analysis* time (`INVALID_ORDERING_TYPE`, failing the whole aggregate) → the call catches that specific `QuerySyntaxError` and re-runs a **reduced** aggregate (no MIN/MAX), returning `None` bounds. The scalar happy path stays a single query. - **Full error taxonomy in the shim (DEC-009).** `map_databricks_exception` maps auth → `WarehouseAuthError`, table-not-found → `TableNotFoundError`, invalid-identifier → `ColumnNotFoundError`, residual programming → `QuerySyntaxError`, else passthrough — reusing existing typed errors (no new class), lazy SDK-error import confined to `_databricks_client.py` (the confinement test stays green). -- **Close the cursor in `try/finally` after every query (PR #257 review).** `_execute` / `_execute_to_dicts` / `materialise_sample` open a cursor on the long-lived connection and MUST release it on both the success and failure paths, or repeated queries leak server-side cursor handles. The `_execute_to_dicts` close happens **after** `_rows_to_dicts` reads `cursor.description` (the description is needed to shape tuple rows into dicts) — so the cleanup is an outer `try/finally` wrapping an inner `try/except` that scopes the exception mapping to `execute`/`fetchall` only. Pinned by `tests/warehouse/test_databricks_adapter.py::test_execute_closes_cursor_on_{success,failure}` + the `_execute_to_dicts` / materialise variants (the fake's `_FakeDatabricksCursor.closed` + `FakeDatabricksConnection.cursors`). **Latent-leak note:** the Snowflake adapter only closes its cursor in `_execute_scalar` — its `_execute` / `_execute_to_dicts` / `materialise_sample` have the **same leak** and should get the same `try/finally` when next touched (out of #224 scope; the new convention is "every cursor-opening adapter method closes in `try/finally`"). +- **Close the cursor in `try/finally` after every query (PR #257 review).** `_execute` / `_execute_to_dicts` / `materialise_sample` open a cursor on the long-lived connection and MUST release it on both the success and failure paths, or repeated queries leak server-side cursor handles. The `_execute_to_dicts` close happens **after** `_rows_to_dicts` reads `cursor.description` (the description is needed to shape tuple rows into dicts) — so the cleanup is an outer `try/finally` wrapping an inner `try/except` that scopes the exception mapping to `execute`/`fetchall` only. Pinned by `tests/warehouse/test_databricks_adapter.py::test_execute_closes_cursor_on_{success,failure}` + the `_execute_to_dicts` / materialise variants (the fake's `_FakeDatabricksCursor.closed` + `FakeDatabricksConnection.cursors`). **Latent-leak note (updated by #258):** the Snowflake adapter's `_execute` / `_execute_to_dicts` / `run_test_sql` now close their cursor in `try/finally` too (fixed in #258, mirroring this Databricks shape — DEC-007); `_execute_scalar` already did. The one remaining Snowflake leak is **`materialise_sample`** (opens a cursor without a `try/finally` close) — fix it the same way when next touched. The convention is "every cursor-opening adapter method closes in `try/finally`". **Testing tiers + the #226 live cert — shape was certified, then the live run found three real bugs.** Shape is certified by a hand-rolled `FakeDatabricksConnection` (behaviour) + an **ungated** `sqlglot` `databricks`-dialect parse-guard (`tests/warehouse/test_databricks_sql_parse.py`, default suite, planted-violation self-check + per-shape `len(statements)` floor) over every emitted statement, plus the #116 `custom_sql {{ this }}` materialised-substitution test. **#226 then ran the gated live Free-Edition cert** (`@pytest.mark.databricks` + `SF_RUN_DATABRICKS=1`): `estimate_live` (`EXPLAIN COST` → positive int), prune-live `materialised` (real `CREATE OR REPLACE TABLE` created, persisted across queries on the pinned connection, reachable from a follow-up test, dropped at cleanup + scalar `column_stats`), and a full-pipeline `generate` smoke. **The live run surfaced THREE bugs the entire shape tier (snapshot + `sqlglot` parse + fakes) passed clean:** (1) qualified `CREATE TEMPORARY TABLE ..` is rejected (`[TEMP_TABLE_CREATION_REQUIRES_SINGLE_PART_NAME]`) → `CREATE OR REPLACE TABLE` + DROP (L45); (2) `ORDER BY (xxhash64(...struct(*)...))` is rejected in a Sort node (`[INVALID_USAGE_OF_STAR_OR_REGEX]`) → projection-subquery shape (L42); (3) the shared `QuerySyntaxError` rendered "BigQuery rejected the query" for Databricks errors → made vendor-neutral. All three are `sqlglot`-parseable AND fake-acceptable — **only a live warehouse rejects them.** The three **shape-only** residuals #226 left (`column_stats` MIN/MAX on complex types, the `to_json(struct(*))` capture branch, the `str`-partition-filter escape) were all closed by **#227 — see § "Databricks known-quirk follow-ups (#227)" below.** **Load-bearing lesson (sharpens the #121/#124/#171 rule): `sqlglot`-parse + snapshot + fakes certify SHAPE, NOT that a live warehouse ACCEPTS the SQL — and the gap is not exotic. Three first-try-plausible constructs (a qualified TEMP name, an inline `struct(*)` Sort key, a cross-vendor error string) all passed the entire offline tier and failed live. Budget a live-cert pass for every new adapter, and treat the live run as the merge gate, never the parse-guard.** -**Databricks known-quirk follow-ups (#227) — epic #219 closer.** #227 closed the four residuals #224/#225/#226 deferred; nothing silently dropped. R1 — `column_stats` now honours the complex-type MIN/MAX skip contract (the orderable-post-process + non-orderable reduced-aggregate retry above). R2 — the `to_json(struct(*))` failing-row capture branch is now **live-certified** (a gated test drives an engineered failing candidate). R3 — the `str`-valued partition-filter escape uses an adapter-local `_escape_spark_string_literal` (Spark-correct `''` + `\\`), not the borrowed `escape_bq_string_literal`. R4 — `run_stats_query` overrides the ABC `StatsQueryNotSupportedError` degrade (thin `validate_test_sql` + `_execute_to_dicts`, mirroring BigQuery), so `row_count_anomaly_by_period` **evaluates** on Databricks; the anomaly two-query Spark SQL (only sqlglot-parsed by #223) is now **live-certified**. All 5 gated `-m databricks` tests pass against the real Free-Edition warehouse. **The R1 lesson generalises the "shape ≠ acceptance" rule to a *Python-side* assumption: a post-process that nulls MIN/MAX for complex types silently assumed the aggregate always *returns* — but non-orderable Spark types (`map`/`variant`) fail `MIN`/`MAX` at *analysis* time, killing the whole query before the post-process runs. The fake returned canned bounds, so offline tests were green; only the live warehouse (or a live-informed unit test) exposes it. When a stage's correctness rests on "the query will succeed and I'll fix up the result," verify that assumption live — a fake that returns a value can't disprove a query the warehouse rejects.** (Surfaced by the #227 QG correctness review; fixed + live-verified in the same gate.) #258 (Snowflake `column_stats` parity) stays separate. +**Databricks known-quirk follow-ups (#227) — epic #219 closer.** #227 closed the four residuals #224/#225/#226 deferred; nothing silently dropped. R1 — `column_stats` now honours the complex-type MIN/MAX skip contract (the orderable-post-process + non-orderable reduced-aggregate retry above). R2 — the `to_json(struct(*))` failing-row capture branch is now **live-certified** (a gated test drives an engineered failing candidate). R3 — the `str`-valued partition-filter escape uses an adapter-local `_escape_spark_string_literal` (Spark-correct `''` + `\\`), not the borrowed `escape_bq_string_literal`. R4 — `run_stats_query` overrides the ABC `StatsQueryNotSupportedError` degrade (thin `validate_test_sql` + `_execute_to_dicts`, mirroring BigQuery), so `row_count_anomaly_by_period` **evaluates** on Databricks; the anomaly two-query Spark SQL (only sqlglot-parsed by #223) is now **live-certified**. All 5 gated `-m databricks` tests pass against the real Free-Edition warehouse. **The R1 lesson generalises the "shape ≠ acceptance" rule to a *Python-side* assumption: a post-process that nulls MIN/MAX for complex types silently assumed the aggregate always *returns* — but non-orderable Spark types (`map`/`variant`) fail `MIN`/`MAX` at *analysis* time, killing the whole query before the post-process runs. The fake returned canned bounds, so offline tests were green; only the live warehouse (or a live-informed unit test) exposes it. When a stage's correctness rests on "the query will succeed and I'll fix up the result," verify that assumption live — a fake that returns a value can't disprove a query the warehouse rejects.** (Surfaced by the #227 QG correctness review; fixed + live-verified in the same gate.) #258 (Snowflake `column_stats` parity) **shipped** — see § "Snowflake `column_stats` (issue #258)". -**Snowflake sampling + connection-bound session (issue #122).** `SnowflakeAdapter` implements the sampling surface — `sample_rows`, `materialise_sample`, `run_test_sql` (`column_stats` stays `NotImplementedError`; `estimate_query_bytes` was the ABC degrade pending #123 at the time of #122, then graduated to a real `EXPLAIN USING JSON` override in #130 — see the `estimate_query_bytes` graduation note below). Five load-bearing conventions, most diverging from BigQuery because Snowflake's session model differs: +**Snowflake `column_stats` (issue #258) — the aggregate-only backfill; the two-tier MIN/MAX defence.** #258 implemented `SnowflakeAdapter.column_stats` (the last v0.2 Snowflake gap), enabling `safety: aggregate-only`. It deliberately followed **BigQuery, not Databricks**: Snowflake has **no runtime `typeof()`** for a base column, so it reads the declared type from `INFORMATION_SCHEMA.COLUMNS.DATA_TYPE` **up front** (mirroring `_get_num_rows`'s `escape_bq_string_literal` + `UPPER(...)=UPPER('...')` + `project=None`-unqualified pattern) — which means it can **pre-filter** MIN/MAX like BigQuery instead of Databricks' post-process-null + reduced-aggregate-retry dance (that dance exists only because Spark computes `typeof` at runtime). Full BigQuery-style context-manager batching (`_column_stats_pending`/`_column_stats_results` opened in `__enter__`, reset in `__exit__`'s `finally`; the `with adapter:` `RuntimeError` guard falls out for free); one catalog query + one aggregate per table; `COUNT(*) - COUNT(col)` null count (portable + fakesnow-executable, avoids `COUNT_IF`). + +The load-bearing lesson is the **two-tier MIN/MAX defence** — two *distinct* failure modes need two *distinct* guards, and a single skip-set conflates them: + +1. **SQL-analysis-RAISE tier → the skip-set.** `_COMPLEX_SNOWFLAKE_TYPES = {ARRAY, OBJECT, VARIANT, GEOGRAPHY, GEOMETRY}` — types where emitting `MIN`/`MAX` *raises at Snowflake analysis time*; the aggregate simply omits MIN/MAX for them. Conservative superset (over-skip → harmless `None`; under-skip → the whole batch raises). The gated live complex-type cert validates completeness (the #227 lesson). +2. **Out-of-union-RETURN tier → `_coerce_min_max`.** Types that are SQL-orderable (so MIN/MAX *runs* and they are deliberately **NOT** in the skip-set) but whose connector return type is outside `ColumnStats.min`/`max`'s `int|float|str|bool|datetime|date|None` union: **`BINARY` → `bytearray`**, **`TIME` → `datetime.time`** (and `NUMBER` → `Decimal`, coerced to `float`). `_coerce_min_max` nulls any out-of-union value on read-back. **Why this is load-bearing:** the resulting `ColumnStats(...)` `pydantic.ValidationError` is **NOT a `WarehouseError`**, so it bypasses the conservative-degrade path and fails the whole payload build at the **CLI panic tier** — strictly worse than a typed degrade. Both sibling adapters already skip their binary type for exactly this reason (BigQuery `BYTES`, Databricks `binary` → `min=max=None`); #258's first cut made `BINARY` "orderable" (conflating SQL-orderability with return-type-representability) and the QG's two independent reviewers (correctness + semantics) both caught it. **Generalise: when a value crosses a typed-model boundary, "the SQL runs" ≠ "the Python return type is representable" — guard the return type separately from the SQL-emission decision, and prefer a broad `isinstance`-against-the-union net over enumerating every offending type (it catches future types like `VECTOR` for free).** + +**Known limitation (pre-existing, shared with BigQuery):** the aggregate emits `COUNT(DISTINCT col)` unconditionally; Snowflake forbids `DISTINCT` on `GEOGRAPHY`/`GEOMETRY`, so a model carrying such a column can't be profiled via `aggregate-only` (a typed `WarehouseError`, not the panic crash the coercion net prevents). Documented in `docs/warehouse-adapter-ops.md`; use `safety: schema-only` for geospatial models. The distinct-on-`VARIANT`/`ARRAY`/`OBJECT` behaviour is settled by the gated live cert. + +**Cursor-leak hygiene (DEC-007):** #258 also fixed the three Snowflake adapter cursor leaks the rules had flagged — `_execute`, `_execute_to_dicts`, `run_test_sql` now close in `try/finally` (only `_execute_scalar` did before), mirroring the Databricks PR #257 shape (`_execute_to_dicts` closes *after* `_rows_to_dicts` reads `cursor.description`). The convention "every cursor-opening adapter method closes in `try/finally`" is now upheld across both connection-bound adapters. + +**Live-certified (2026-07-03).** The gated `@pytest.mark.snowflake` live cert (`tests/warehouse/test_snowflake_columnstats_live.py` — scalar + complex skip-set + BINARY/TIME coercion columns) **passed against a real Snowflake warehouse**: scalar columns populate min/max; `ARRAY`/`OBJECT`/`VARIANT` return `min=max=None` without raising (DEC-003 skip-set **confirmed complete**); `BINARY`/`TIME` coerce to `None`. Because the cert profiles ARRAY/OBJECT/VARIANT columns (emitting `COUNT(DISTINCT)` on them) and passed, **`COUNT(DISTINCT)` on semi-structured types does NOT raise on Snowflake** — the only residual `COUNT(DISTINCT)` limitation is `GEOGRAPHY`/`GEOMETRY` (by Snowflake's documented restriction; not in the fixture). **Auth lesson: the restored account enforced MFA, which the headless password path can't satisfy — the cert runs via key-pair (JWT) auth, which is MFA-exempt.** #258 also closed a real key-pair gap: the `private_key_path`/`private_key_passphrase`/`authenticator` fields were plumbed `profile → from_profile → __init__` (#120) but **`make_real_client` dropped them at the `connect()` call** — #258 threads them through (`private_key_file` [+ `_pwd`], `authenticator`), key-pair taking precedence over password. (An **empty** passphrase is treated as no-passphrase by the adapter's `if private_key_passphrase:` guard, so an empty-passphrase-encrypted key must be decrypted to an unencrypted PKCS#8 key — the standard `-nocrypt` posture — rather than passed `""`.) + +**Connection-lifecycle fix — owned-connection rebuild across `with` blocks (#258, exposed by the aggregate-only e2e).** `generate` with `safety: aggregate-only` uses ONE adapter instance across TWO `with adapter:` blocks — the safety-aggregate `column_stats` pass, then `prune_tests`. Snowflake's `_cleanup_active_session` **closes the connection** on `__exit__` (the connection IS the session), and #122 deliberately did NOT null `self._connection` — so the second block reused the now-closed connection and raised `250002 (08003): Connection is closed`. (BigQuery never hit this: its `__exit__` resets caches but keeps a reusable client.) Fix: track ownership — `self._owns_connection` is set `True` only when `_get_connection` **lazily builds** a real connection; `_cleanup_active_session` nulls `self._connection` after close **only when owned**, so the next block rebuilds a fresh connection. An **injected** connection (tests) stays non-nulled (the #122 concern — a fake is reused across blocks, never silently rebuilt from empty creds). This is why the **aggregate-only e2e is live-certified** (`test_e2e_signalforge_generate_aggregate_only_against_tpch_sf1` — full `generate` against read-only TPCH via key-pair, 2026-07-03). **General lesson: an adapter whose `__exit__` closes a persistent connection MUST rebuild-on-next-use; "don't null the connection" is only safe for an adapter (like BigQuery) that keeps a reusable client across `with` blocks. Any multi-`with` flow on one instance (aggregate-only = safety + prune) is the thing that exposes it — and only a live e2e does, since fakes don't error on reuse-after-close.** + +**Snowflake sampling + connection-bound session (issue #122).** `SnowflakeAdapter` implements the sampling surface — `sample_rows`, `materialise_sample`, `run_test_sql` (`column_stats` was left `NotImplementedError` at the time of #122, then **shipped in #258** — see § "Snowflake `column_stats` (issue #258)" below; `estimate_query_bytes` was the ABC degrade pending #123 at the time of #122, then graduated to a real `EXPLAIN USING JSON` override in #130 — see the `estimate_query_bytes` graduation note below). Five load-bearing conventions, most diverging from BigQuery because Snowflake's session model differs: - **The connection IS the session (no `session_id` string).** BigQuery threads a server-side `session_id` via `connection_properties` on every query (§ "Session/connection state"); Snowflake's *connection* holds the session, so `self._active_session` stores the **connection object** itself and every op runs on the one connection. A `connection=` injection seam on `__init__` (mirrors BigQuery's `client=`) + lazy `_get_connection()` (builds via the shim's `make_real_client` on first use) is the testability seam. `_get_connection()` sets `_active_session` on first open. - **Reuse the `SNOWFLAKE_DIALECT` SQL-fragment fields — never hard-code `HASH(*)`.** `sample_rows` and the `materialise_sample` CTAS delegate to `signalforge.warehouse._sample_sql.render_sample_select(..., order_by_hash=True)` (shared with the prune compiler's sample CTE — issue #139), which reads `dialect.sample_row_hash_expr`, `dialect.sample_hash_in_projection`, `dialect.sample_hash_alias`; partition filters are rendered by the caller via `dialect.timestamp_literal_template` / `date_literal_template` and passed in as `extra_where`. Reading the dialect (not hard-coding) keeps the adapter's sample SQL byte-consistent with the prune compiler's sample CTE (Architectural Commitment #5). **Since #139 Snowflake emits the projection-subquery shape** — `SELECT * EXCLUDE (_sf_sample_hash) FROM (SELECT t.*, ABS(HASH(*)) AS _sf_sample_hash FROM AS t) WHERE MOD(_sf_sample_hash, b) < 1 [AND ] ORDER BY _sf_sample_hash LIMIT n` — because `HASH(*)` is rejected as a `WHERE`/`ORDER BY` predicate (`002079`); BigQuery keeps the inline form (`sample_hash_in_projection=False`). @@ -274,8 +289,8 @@ When introducing a new fail-soft cleanup boundary (Postgres temp-table cleanup, The gated `snowflake` marker now spans offline (`fakesnow`/`sqlglot`, run with no env vars) AND live (`SF_RUN_SNOWFLAKE=1` + conn vars, +`SNOWFLAKE_DATABASE`/`SCHEMA` writable for the engineered prune-live table, +`ANTHROPIC_API_KEY` for the full pipeline) tests under one `uv run pytest -m snowflake --no-cov`. The `docs/warehouse-adapter-ops.md` § "Snowflake adapter (v0.2)" carries the consolidated operator story incl. the **cost guidance** (resource monitor FIRST, XS warehouse, aggressive auto-suspend) that any live Snowflake test must surface in its module docstring. -**Live-harness findings — fakesnow/fakes hid multiple live-only adapter bugs (the #124 payoff).** Running the gated suite against a real warehouse surfaced bugs that the offline `fakesnow` + hand-fakes could not, because fakesnow's DuckDB cursor/result shapes diverge from the real connector's. The lesson generalises the #121 "parser/executor in the loop" rule: a vendor adapter is NOT certified until a live run exercises every path; fakes prove behaviour-against-a-contract, not behaviour-against-the-vendor. Concretely #124's live run found, in order: (1) **`_quote` case-folding** (fixed here — the adapter must fold-then-quote identically to the compiler, else CREATE-vs-REFERENCE diverge); (2) **`ARRAY_AGG(OBJECT_CONSTRUCT(*))` capture-failures samples come back as a JSON *string*** (a VARIANT), not a Python list — `run_test_sql` must `json.loads` it before iterating (fakesnow returned a list, so offline passed); (3) **`HASH(*)` is invalid in WHERE/ORDER BY** on Snowflake → all sample-mode SQL was rejected (bead `bd_1-scaffolding-cdp`) — **FIXED by #139**: the shared `_sample_sql.render_sample_select` now emits the projection-subquery shape (`SELECT * EXCLUDE (_sf_sample_hash) FROM (SELECT t.*, ABS(HASH(*)) AS _sf_sample_hash …)`) driven by `Dialect.sample_hash_in_projection`, so `materialise_sample` / `sample_rows` / the compiler sample CTE all compute the hash in the SELECT projection rather than the rejected predicate position; (4) **oneshot sample-bucket row-count routed through a BigQuery-only `_get_client`** the engine never made vendor-neutral (bead `bd_1-scaffolding-tft`) — **FIXED by #140**: a vendor-neutral `WarehouseAdapter.get_row_count(table) -> int | None` seam (ABC concrete-default-raise `RowCountNotSupportedError`, mirroring `materialise_sample` / `estimate_query_bytes`; BigQuery wraps its cached `_get_table().num_rows`, Snowflake wraps `_get_num_rows`) replaced the `getattr(adapter, "_get_client")` crack in `prune.engine._resolve_sample_bucket`, so `oneshot` sample-mode prune now works on any adapter that implements the seam (certified by the gated live `test_prune_drops_always_passes_not_null_live_oneshot_sample`). Net operator contract: **live Snowflake works today with `safety: schema-only` + `prune.scope: full` OR `prune.scope: sample` + either `prune.sample_strategy: materialised` (the #139 projection-subquery fix) or `oneshot` (the #140 row-count seam)** — only `aggregate-only` (`column_stats`) remains deferred/beaded. When the next vendor adapter ships, budget a live-debugging pass; expect VARIANT/array columns as JSON strings, identifier case-folding mismatches, AND vendor clause-position constraints (a SQL fragment legal in one clause may be rejected in another — the #139 `HASH(*)` lesson) as the first things to break. +**Live-harness findings — fakesnow/fakes hid multiple live-only adapter bugs (the #124 payoff).** Running the gated suite against a real warehouse surfaced bugs that the offline `fakesnow` + hand-fakes could not, because fakesnow's DuckDB cursor/result shapes diverge from the real connector's. The lesson generalises the #121 "parser/executor in the loop" rule: a vendor adapter is NOT certified until a live run exercises every path; fakes prove behaviour-against-a-contract, not behaviour-against-the-vendor. Concretely #124's live run found, in order: (1) **`_quote` case-folding** (fixed here — the adapter must fold-then-quote identically to the compiler, else CREATE-vs-REFERENCE diverge); (2) **`ARRAY_AGG(OBJECT_CONSTRUCT(*))` capture-failures samples come back as a JSON *string*** (a VARIANT), not a Python list — `run_test_sql` must `json.loads` it before iterating (fakesnow returned a list, so offline passed); (3) **`HASH(*)` is invalid in WHERE/ORDER BY** on Snowflake → all sample-mode SQL was rejected (bead `bd_1-scaffolding-cdp`) — **FIXED by #139**: the shared `_sample_sql.render_sample_select` now emits the projection-subquery shape (`SELECT * EXCLUDE (_sf_sample_hash) FROM (SELECT t.*, ABS(HASH(*)) AS _sf_sample_hash …)`) driven by `Dialect.sample_hash_in_projection`, so `materialise_sample` / `sample_rows` / the compiler sample CTE all compute the hash in the SELECT projection rather than the rejected predicate position; (4) **oneshot sample-bucket row-count routed through a BigQuery-only `_get_client`** the engine never made vendor-neutral (bead `bd_1-scaffolding-tft`) — **FIXED by #140**: a vendor-neutral `WarehouseAdapter.get_row_count(table) -> int | None` seam (ABC concrete-default-raise `RowCountNotSupportedError`, mirroring `materialise_sample` / `estimate_query_bytes`; BigQuery wraps its cached `_get_table().num_rows`, Snowflake wraps `_get_num_rows`) replaced the `getattr(adapter, "_get_client")` crack in `prune.engine._resolve_sample_bucket`, so `oneshot` sample-mode prune now works on any adapter that implements the seam (certified by the gated live `test_prune_drops_always_passes_not_null_live_oneshot_sample`). Net operator contract: **live Snowflake works today with `safety: schema-only` + `prune.scope: full` OR `prune.scope: sample` + either `prune.sample_strategy: materialised` (the #139 projection-subquery fix) or `oneshot` (the #140 row-count seam)**; `aggregate-only` (`column_stats`) **shipped in #258** (§ "Snowflake `column_stats` (issue #258)"). When the next vendor adapter ships, budget a live-debugging pass; expect VARIANT/array columns as JSON strings, identifier case-folding mismatches, AND vendor clause-position constraints (a SQL fragment legal in one clause may be rejected in another — the #139 `HASH(*)` lesson) as the first things to break. ## Reference -`plans/super/124-snowflake-test-docs.md` — DEC-001 … DEC-011 (test harness + full taxonomy + two-strategy live e2e + ops docs). `plans/super/3-bigquery-adapter.md` — DEC-001 … DEC-028. `plans/super/22-temp-table-sample.md` — v0.2 materialised-sample additions (`materialise_sample` ABC, BigQuery session-state pattern, cleanup-WARNING shape). `plans/super/36-estimate-cost-preview.md` — `estimate_query_bytes` ABC addition. `plans/super/120-snowflake-profile.md` — DEC-001 … DEC-010 (unified `DbtProfileTarget` + per-type cross-field validator, `IncompleteProfileError`, `validate_snowflake_account`, auth scope, drift detector). `plans/super/122-snowflake-sampling.md` — DEC-001 … DEC-010 (Snowflake `sample_rows` / `materialise_sample` / `run_test_sql`, connection-bound session state, shared `_sample_id` hoist, INFORMATION_SCHEMA sizing, Snowflake-shaped cleanup WARNING). `plans/super/130-snowflake-estimate-explain.md` — DEC-001 … DEC-009 (Snowflake `estimate_query_bytes` via `EXPLAIN USING JSON`, `EstimateUnavailableError` typed degrade, planner-estimate accuracy caveat). `plans/super/224-databricks-sampling.md` — DEC-001 … DEC-012 (Databricks `sample_rows` / `get_row_count` / `materialise_sample` / `run_test_sql` / `column_stats`, `validate_catalog_or_project` for Unity Catalog catalogs, inline-predicate sample shape, `COUNT(*)` sizing, `to_json` per-row capture, the #226 live-cert ledger; `tests/warehouse/{_fake_databricks.py,test_databricks_adapter.py,test_databricks_sql_parse.py}`). `plans/super/225-databricks-estimate.md` — DEC-001 … DEC-013 (Databricks `estimate_query_bytes` via `EXPLAIN COST`: the pure `_parse_explain_cost_bytes` text parser, the `8.0 EiB` / `Long.MaxValue` no-stats sentinel degrade, the reused `EstimateUnavailableError` — no new error class, `_PRIMITIVE_LABEL_BY_ADAPTER["DatabricksAdapter"] = "Databricks EXPLAIN COST"`, live cert deferred to #226; `tests/warehouse/test_databricks_estimate.py`, `tests/fixtures/warehouse/databricks/explain_cost_*.txt`). `plans/super/226-databricks-test-docs.md` — the epic-closer (test harness + gated live Free-Edition cert + ops docs); the live run REVERSED #224's inline-sample DEC-002 (→ projection-subquery) and CORRECTED its `CREATE TEMPORARY TABLE` DEC-004 (→ `CREATE OR REPLACE TABLE` + DROP), and made `QuerySyntaxError` vendor-neutral — three bugs the offline shape tier passed clean; `tests/warehouse/test_databricks_{prune,estimate}_live.py`, `tests/cli/test_e2e_databricks_smoke.py`. `plans/super/227-databricks-followups.md` — the epic-#219 closer (residual disposition): R1 `column_stats` complex-type MIN/MAX skip incl. the non-orderable `map`/`variant` reduced-aggregate retry, R2 `to_json` capture live-cert, R3 Spark str-literal partition escape, R4 `run_stats_query` override → anomaly live-cert; `column_stats(map)` reduced-retry live-verified in the QG. `src/signalforge/warehouse/_sample_id.py` — shared deterministic-sample-id seam. `src/signalforge/warehouse/` — current implementation. `tests/warehouse/_fake.py` — `FakeBigQueryClient` + `expect_*` API. `docs/warehouse-adapter-ops.md` — operational reference. +`plans/super/124-snowflake-test-docs.md` — DEC-001 … DEC-011 (test harness + full taxonomy + two-strategy live e2e + ops docs). `plans/super/3-bigquery-adapter.md` — DEC-001 … DEC-028. `plans/super/22-temp-table-sample.md` — v0.2 materialised-sample additions (`materialise_sample` ABC, BigQuery session-state pattern, cleanup-WARNING shape). `plans/super/36-estimate-cost-preview.md` — `estimate_query_bytes` ABC addition. `plans/super/120-snowflake-profile.md` — DEC-001 … DEC-010 (unified `DbtProfileTarget` + per-type cross-field validator, `IncompleteProfileError`, `validate_snowflake_account`, auth scope, drift detector). `plans/super/122-snowflake-sampling.md` — DEC-001 … DEC-010 (Snowflake `sample_rows` / `materialise_sample` / `run_test_sql`, connection-bound session state, shared `_sample_id` hoist, INFORMATION_SCHEMA sizing, Snowflake-shaped cleanup WARNING). `plans/super/130-snowflake-estimate-explain.md` — DEC-001 … DEC-009 (Snowflake `estimate_query_bytes` via `EXPLAIN USING JSON`, `EstimateUnavailableError` typed degrade, planner-estimate accuracy caveat). `plans/super/258-snowflake-column-stats.md` — DEC-001 … DEC-009 (Snowflake `column_stats` parity: catalog pre-filter + full BigQuery-style batching, the two-tier MIN/MAX defence — skip-set for SQL-raise types + `_coerce_min_max` net for out-of-union return types BINARY/TIME, `Decimal`→`float`, `COUNT(*)-COUNT(col)` null count, cursor-leak fix, GEOGRAPHY/GEOMETRY `COUNT(DISTINCT)` limitation, gated live cert; `tests/warehouse/{test_snowflake_adapter.py,test_snowflake_adapter_fakesnow.py,test_snowflake_columnstats_live.py}`). `plans/super/224-databricks-sampling.md` — DEC-001 … DEC-012 (Databricks `sample_rows` / `get_row_count` / `materialise_sample` / `run_test_sql` / `column_stats`, `validate_catalog_or_project` for Unity Catalog catalogs, inline-predicate sample shape, `COUNT(*)` sizing, `to_json` per-row capture, the #226 live-cert ledger; `tests/warehouse/{_fake_databricks.py,test_databricks_adapter.py,test_databricks_sql_parse.py}`). `plans/super/225-databricks-estimate.md` — DEC-001 … DEC-013 (Databricks `estimate_query_bytes` via `EXPLAIN COST`: the pure `_parse_explain_cost_bytes` text parser, the `8.0 EiB` / `Long.MaxValue` no-stats sentinel degrade, the reused `EstimateUnavailableError` — no new error class, `_PRIMITIVE_LABEL_BY_ADAPTER["DatabricksAdapter"] = "Databricks EXPLAIN COST"`, live cert deferred to #226; `tests/warehouse/test_databricks_estimate.py`, `tests/fixtures/warehouse/databricks/explain_cost_*.txt`). `plans/super/226-databricks-test-docs.md` — the epic-closer (test harness + gated live Free-Edition cert + ops docs); the live run REVERSED #224's inline-sample DEC-002 (→ projection-subquery) and CORRECTED its `CREATE TEMPORARY TABLE` DEC-004 (→ `CREATE OR REPLACE TABLE` + DROP), and made `QuerySyntaxError` vendor-neutral — three bugs the offline shape tier passed clean; `tests/warehouse/test_databricks_{prune,estimate}_live.py`, `tests/cli/test_e2e_databricks_smoke.py`. `plans/super/227-databricks-followups.md` — the epic-#219 closer (residual disposition): R1 `column_stats` complex-type MIN/MAX skip incl. the non-orderable `map`/`variant` reduced-aggregate retry, R2 `to_json` capture live-cert, R3 Spark str-literal partition escape, R4 `run_stats_query` override → anomaly live-cert; `column_stats(map)` reduced-retry live-verified in the QG. `src/signalforge/warehouse/_sample_id.py` — shared deterministic-sample-id seam. `src/signalforge/warehouse/` — current implementation. `tests/warehouse/_fake.py` — `FakeBigQueryClient` + `expect_*` API. `docs/warehouse-adapter-ops.md` — operational reference. diff --git a/CHANGELOG.md b/CHANGELOG.md index bede70c3..9f553481 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,9 @@ All notable changes to SignalForge are documented here. The format is loosely ba - **Databricks warehouse adapter (epic #219 — #221–#227).** The third concrete `WarehouseAdapter`, graduating the ABC + factory seam through a second non-BigQuery vendor (Architectural Commitment #3 — warehouse-agnostic by design). `WarehouseAdapter.from_profile` dispatches `type: databricks` dbt profiles via the unified `DbtProfileTarget` with a conditional required-key set keyed on `auth_type` (PAT → `token`, OAuth-M2M → `client_id` + `client_secret`; PAT is the only v0.x connection path) and empty-string-as-missing credential validation (#222); the `databricks-sql-connector` shim is confined to `adapters/_databricks_client.py` (one-shim-per-vendor) and ships under the `[databricks]` optional extra so the base install stays connector-free (#221). The prune compiler emits valid Databricks/Spark-SQL purely from `DATABRICKS_DIALECT` — backtick `quote_char`, `identifier_case='lower'` (Unity Catalog lower-folds — the opposite of Snowflake), per-component qualified-name quoting, and the sign-bit-masked `(xxhash64(to_json(struct(*))) & 9223372036854775807)` sampling hash — never branching on dialect name; certified by 32 byte-exact fixtures and an **ungated** `sqlglot` `databricks`-dialect parse-guard (Databricks has no offline execution fake, so the parse-guard is the sole automated validity gate; #223). Deterministic `sample_rows` (projection-subquery hash-mod — Spark rejects `struct(*)` in a Sort node, so the masked `xxhash64` hash is computed in an inner projection alias referenced by `WHERE`/`ORDER BY`; corrected from the inline shape by #226's live cert) + `get_row_count` (`SELECT COUNT(*)`, since `DESCRIBE DETAIL` has no reliable `numRows`) + `materialise_sample` (qualified `CREATE OR REPLACE TABLE` colocated with the source — Databricks rejects a qualified temp name — dropped fail-soft at session cleanup, since a real table does not auto-reap) + `run_test_sql` (per-row `to_json(struct(*))` capture, `json.loads`-ed) + `column_stats`, all on a connection-bound session with fail-soft cleanup; the `validate_catalog_or_project` union validator unblocks short Unity Catalog catalogs (`main` / `workspace`) without weakening identifier hygiene (#224). `estimate_query_bytes` runs `EXPLAIN COST` and parses the maximum Spark CBO `Statistics(sizeInBytes=…)` across plan nodes, reusing the typed `EstimateUnavailableError` for both the no-parseable-stats case and the present-but-sentinel `8.0 EiB` (`Long.MaxValue` / `spark.sql.defaultSizeInBytes`) no-stats shape — never reporting the ~9-exabyte sentinel (#225). **The #226 live-certification test harness + ops docs:** a hand-rolled `FakeDatabricksConnection` (with cursor-leak assertions) drives session / cleanup / error-mapping / sampling / capture behaviour offline; a hand-crafted manifest seed (`tests/fixtures/databricks/`, `samples.nyctaxi.trips`-shaped) plus a credential-free loads-only test certify `signalforge.manifest.load(...)` against it; three gated `@pytest.mark.databricks` live e2e tests run against a real Free-Edition `2X-Small` warehouse — `estimate_live` (`EXPLAIN COST` returns a positive int), prune-live `materialised` (always-passes drop against a writable-catalog temp table), and a full-pipeline `generate` smoke against `samples.nyctaxi.trips` — each gated by `SF_RUN_DATABRICKS=1` + connection env vars on top of the marker; and a consolidated `docs/warehouse-adapter-ops.md § Databricks adapter` carries the operator-facing surface (install, profile keys, dialect, connection-bound session + sampling, `EXPLAIN COST` estimate, error taxonomy, Free-Edition cost guidance, and the live-certification ledger), with the Free-Edition setup walkthrough in `docs/research/databricks-test-environment.md`. **The live pass surfaced three real adapter bugs — a qualified-temp-name rejection (`materialise_sample` → `CREATE OR REPLACE TABLE` + explicit DROP), `struct(*)` rejected in a Sort node (sampling → projection-subquery shape), and a cross-vendor `QuerySyntaxError` message (made vendor-neutral) — all fixed inline and re-certified live.** -> **`column_stats` IS available for Databricks** — so `safety: aggregate-only` works, a deliberate divergence from Snowflake (whose `column_stats` is not yet implemented; Snowflake parity tracked as #258, out of scope for epic #219). Both `prune.sample_strategy: materialised` (needs a writable source catalog) and `oneshot` (no CTAS) are functional. The #221–#225 surface was certified for SQL *shape* (fake connection + ungated `sqlglot` parse-guard + a maintainer-captured `EXPLAIN COST` fixture) and #226 added the gated live Free-Edition certification. **#227 (the epic-#219 closer) reconciled the Databricks residual set — nothing is silently dropped:** complex-type `column_stats` `MIN`/`MAX` is now skipped and `None`d for Spark `array` / `struct` / `map` / `binary` / `variant` types, honouring the `ColumnStats` DEC-016 contract like BigQuery (implemented); the `str`-valued partition-filter escape now uses a Spark-correct `_escape_spark_string_literal` (`''` doubling + backslash) instead of the reused BigQuery helper (fixed); and `run_stats_query` now overrides the ABC `StatsQueryNotSupportedError` degrade so `row_count_anomaly_by_period` **evaluates** on Databricks rather than routing to `kept-without-evidence` (implemented) — with the anomaly two-query stats path and the `to_json(struct(*))` failing-row capture branch both now **live-certified** against the real Free-Edition warehouse. The complete gated live suite is green: `SF_RUN_DATABRICKS=1 uv run pytest -m databricks --no-cov` → **5 tests pass** (`estimate_live` via `EXPLAIN COST`, materialised-sample prune, the anomaly two-query cert, the `to_json` capture cert, and the full-pipeline `generate` smoke) — no inline fixes needed. +> **`column_stats` IS available for Databricks** — so `safety: aggregate-only` works; Snowflake reached the same parity in #258 (also in this release), so this is no longer a divergence. Both `prune.sample_strategy: materialised` (needs a writable source catalog) and `oneshot` (no CTAS) are functional. The #221–#225 surface was certified for SQL *shape* (fake connection + ungated `sqlglot` parse-guard + a maintainer-captured `EXPLAIN COST` fixture) and #226 added the gated live Free-Edition certification. **#227 (the epic-#219 closer) reconciled the Databricks residual set — nothing is silently dropped:** complex-type `column_stats` `MIN`/`MAX` is now skipped and `None`d for Spark `array` / `struct` / `map` / `binary` / `variant` types, honouring the `ColumnStats` DEC-016 contract like BigQuery (implemented); the `str`-valued partition-filter escape now uses a Spark-correct `_escape_spark_string_literal` (`''` doubling + backslash) instead of the reused BigQuery helper (fixed); and `run_stats_query` now overrides the ABC `StatsQueryNotSupportedError` degrade so `row_count_anomaly_by_period` **evaluates** on Databricks rather than routing to `kept-without-evidence` (implemented) — with the anomaly two-query stats path and the `to_json(struct(*))` failing-row capture branch both now **live-certified** against the real Free-Edition warehouse. The complete gated live suite is green: `SF_RUN_DATABRICKS=1 uv run pytest -m databricks --no-cov` → **5 tests pass** (`estimate_live` via `EXPLAIN COST`, materialised-sample prune, the anomaly two-query cert, the `to_json` capture cert, and the full-pipeline `generate` smoke) — no inline fixes needed. + +- **Snowflake `column_stats` — `safety: aggregate-only` now works on Snowflake (#258).** The `SnowflakeAdapter` implements `column_stats` (previously a deferred `NotImplementedError`), bringing Snowflake to parity with Databricks and BigQuery for column profiling — so `safety.mode: aggregate-only` runs end-to-end on Snowflake, closing the last major mode/scope/strategy gap (with one documented geospatial exception, below). A single `INFORMATION_SCHEMA.COLUMNS` catalog pre-filter resolves each queued column's declared type, then a full BigQuery-style per-table batched aggregate computes `count` / `distinct` / `nulls` / `min` / `max` in one query, with `MIN`/`MAX` skipped (→ `None`) for unorderable Snowflake types (`ARRAY` / `OBJECT` / `VARIANT` / `GEOGRAPHY` / `GEOMETRY`); `BINARY` / `TIME` (SQL-orderable but returning connector types outside the `ColumnStats` union) have their `min`/`max` nulled on read-back, honouring the `ColumnStats` contract. One documented limitation: a model carrying a `GEOGRAPHY` / `GEOMETRY` column cannot be profiled via `aggregate-only` — Snowflake forbids `COUNT(DISTINCT)` on those types — so use `safety: schema-only` for geospatial models. Also hardens the adapter's cursor hygiene: `_execute` / `_execute_to_dicts` / `run_test_sql` now close their cursor in `try/finally` on both the success and failure paths (mirroring `_execute_scalar` and the Databricks PR #257 fix), so repeated queries no longer leak server-side cursor handles. ## [0.7.0] — 2026-06-17 diff --git a/README.md b/README.md index 740554cc..2d3787cf 100644 --- a/README.md +++ b/README.md @@ -92,7 +92,7 @@ Full reference: [Claude Code skill](docs/skills.md) — covers the install path ## Supported warehouses -SignalForge ships three production warehouse adapters today: **BigQuery** (the original target — exercised end-to-end by `signalforge init-demo` and the quick start below), **Snowflake** (full sampling, materialised-sample CTAS, and `EXPLAIN`-based bytes estimation; one combination — `safety: aggregate-only` / Snowflake `column_stats` — is not yet implemented, every other mode/scope/strategy combination is functional), and **Databricks** (full sampling, both materialised + oneshot strategies, `EXPLAIN COST`-based bytes estimation, and `column_stats` — so `safety: aggregate-only` works too, unlike Snowflake; shipped across #224–#225 and live-certified against Databricks Free Edition in #226). **Postgres** ships as a typed `NotImplementedError` stub; **Redshift** remains on the roadmap. +SignalForge ships three production warehouse adapters today: **BigQuery** (the original target — exercised end-to-end by `signalforge init-demo` and the quick start below), **Snowflake** (full sampling, materialised-sample CTAS, `EXPLAIN`-based bytes estimation, and `column_stats` — `safety: aggregate-only` implemented in #258, functional across every mode/scope/strategy combination except that a model with a `GEOGRAPHY`/`GEOMETRY` column can't use `aggregate-only`), and **Databricks** (full sampling, both materialised + oneshot strategies, `EXPLAIN COST`-based bytes estimation, and `column_stats` — so `safety: aggregate-only` works too; shipped across #224–#225 and live-certified against Databricks Free Edition in #226). **Postgres** ships as a typed `NotImplementedError` stub; **Redshift** remains on the roadmap. The architecture is warehouse-agnostic — adapters plug in behind a thin sampling/profiling interface (`WarehouseAdapter.from_profile`), so new vendors slot in without touching the draft / prune / grade / diff stages. Per-warehouse setup (auth, cost guardrails, profile-field requirements) lives in [Configuration](#configuration). @@ -486,8 +486,11 @@ reference (sampling, session cleanup, `EXPLAIN`-based bytes estimation, known limitations) is in [docs/warehouse-adapter-ops.md § Snowflake adapter](docs/warehouse-adapter-ops.md). -> **Known limitation:** `safety: aggregate-only` (Snowflake `column_stats`) -> is not yet implemented. Every other combination is functional. +> **Aggregate-only support (#258):** `safety: aggregate-only` is supported on +> Snowflake — `column_stats` is implemented. One documented exception: a model +> carrying a `GEOGRAPHY` / `GEOMETRY` column can't be profiled via +> `aggregate-only` (Snowflake forbids `COUNT(DISTINCT)` on those types) — use +> `safety: schema-only` for geospatial models. ### Databricks @@ -509,8 +512,9 @@ schema: my_schema Install the adapter dependency with the `[databricks]` extra (`pip install "signalforge-dbt[databricks]"`); the base install never pulls -`databricks-sql-connector` in. Unlike Snowflake, **`column_stats` is -available**, so `safety: aggregate-only` is functional. Both +`databricks-sql-connector` in. **`column_stats` is available**, so +`safety: aggregate-only` is functional (Snowflake reached the same +parity in #258). Both `prune.sample_strategy` values work — `materialised` (a `CREATE OR REPLACE TABLE` in the source catalog — Databricks rejects a qualified temp name — so it needs a writable catalog; the table is dropped at session cleanup) and diff --git a/docs/warehouse-adapter-ops.md b/docs/warehouse-adapter-ops.md index c5370b2e..8cb7cb24 100644 --- a/docs/warehouse-adapter-ops.md +++ b/docs/warehouse-adapter-ops.md @@ -393,7 +393,8 @@ the v0.2 stop-gap, not a permanent surface. > sample-mode prune now works on live Snowflake.** The `oneshot` strategy works > too since #140 routed its sample row-count through the vendor-neutral > `WarehouseAdapter.get_row_count` seam (it previously reached a BigQuery-only -> `_get_client`); see "Known limitations on live Snowflake" below. +> `_get_client`); see "Live Snowflake (v0.2) — all safety modes supported" +> below. ## Query-bytes estimation (v0.2, issue #36) @@ -722,17 +723,38 @@ passes through unchanged. No `BytesBilledExceededError` equivalent — Snowflake has no bytes-billed cap (cost is governed by warehouse size + auto-suspend, see below). -**Known limitations on live Snowflake (v0.2) — use `safety: schema-only`.** One -deferred path remains after #139 fixed the `HASH(*)`-in-predicate bug and #140 -added the vendor-neutral row-count seam. Both `prune.sample_strategy` values now -work; the combinations certified green by the gated live e2e are +**Live Snowflake (v0.2) — all safety modes supported.** After #139 fixed the +`HASH(*)`-in-predicate bug, #140 added the vendor-neutral row-count seam, and +issue #258 implemented `column_stats`, every `safety` × `scope` × +`sample_strategy` combination is functional. The combinations certified by the +**maintainer-run gated live e2e** suite (opt-in — deselected from normal CI) are `safety: schema-only` + `prune.scope: full`, or `prune.scope: sample` with -either `prune.sample_strategy: materialised` or `oneshot`: - -- **`safety: aggregate-only` — unsupported.** Profiles columns via - `adapter.column_stats`, which `SnowflakeAdapter` leaves as a deferred - `NotImplementedError` (the one v0.2 method not yet implemented). - `generate` with `safety.mode: aggregate-only` fails (exit 1). +either `prune.sample_strategy: materialised` or `oneshot`; the `aggregate-only` +`column_stats` path is **live-certified** too (#258, via key-pair auth — see below): + +- **`safety: aggregate-only` — supported as of #258.** Profiles columns via + `adapter.column_stats`, now implemented on `SnowflakeAdapter` (parity with + Databricks): a catalog pre-filter over `INFORMATION_SCHEMA.COLUMNS` resolves + each column's declared type, then a full BigQuery-style per-table batched + aggregate computes `count` / `distinct` / `nulls` / `min` / `max`, with + `MIN`/`MAX` skipped (→ `None`) for unorderable Snowflake types (`ARRAY` / + `OBJECT` / `VARIANT` / `GEOGRAPHY` / `GEOMETRY`). Types that are SQL-orderable + but whose connector return type is outside the `ColumnStats.min`/`max` union + (`BINARY` → `bytearray`, `TIME` → `datetime.time`) have their `min`/`max` + nulled on read-back rather than raising. `generate` with + `safety.mode: aggregate-only` now runs on Snowflake. + - **Known limitation (#258):** the aggregate emits `COUNT(DISTINCT )` for + every column (mirroring the BigQuery adapter). Snowflake forbids `DISTINCT` + on `GEOGRAPHY` / `GEOMETRY`, so a model carrying such a column cannot be + profiled via `aggregate-only` — the aggregate fails with a typed + `WarehouseError`. Use `safety: schema-only` for models with geospatial + columns. `DISTINCT` on `VARIANT` / `ARRAY` / `OBJECT` does **not** raise — + confirmed by the gated live complex-type cert + (`tests/warehouse/test_snowflake_columnstats_live.py`), which profiles those + three types and asserts `min=max=None` without error against a real + warehouse (run 2026-07-03 via key-pair auth). The `GEOGRAPHY`/`GEOMETRY` + `COUNT(DISTINCT)` limit is by inspection of Snowflake's documented + restriction, not exercised by the cert (no geospatial column in the fixture). **Fixed by #140:** `prune.scope: sample` + `prune.sample_strategy: oneshot` on a non-BigQuery adapter no longer raises at the engine seam. The sample row-count is @@ -933,12 +955,11 @@ supplementary-source boundary and renders ``, falling back to a price-only preview. **`column_stats` is AVAILABLE for Databricks — `safety: aggregate-only` -works.** This is a **deliberate divergence from Snowflake**, whose -`column_stats` (and therefore `safety.mode: aggregate-only`) is **not yet -implemented**. The Databricks adapter ships `column_stats` (issue #224, +works.** Snowflake reached the same capability in #258, so this is no longer +a divergence. The Databricks adapter ships `column_stats` (issue #224, DEC-011) as a single aggregate query — `count` / `distinct` / `nulls` / `min` / `max` / `data_type` (the last via `MAX(typeof())`) — over the -fold-then-quoted column. The Snowflake-parity follow-up is tracked as issue #258. +fold-then-quoted column. Snowflake's `column_stats` parity shipped in issue #258. **Complex-type `MIN`/`MAX` parity (#227 US-001):** like BigQuery, Databricks now skips `MIN`/`MAX` (→ `None`) for complex Spark types (`array` / `struct` / `map` / `binary` / `variant`), honouring the `ColumnStats` DEC-016 contract. @@ -1020,8 +1041,8 @@ path + the `to_json(struct(*))` capture branch are live-certified (US-004, above). The remaining **shape-only** (not live-exercised) path is `column_stats` `MIN`/`MAX` on complex-typed columns — the skip-and-`None` post-process is unit-tested but the live pass exercised only scalar columns. -(Snowflake `column_stats` parity is separate — issue #258, out of scope for -epic #219.) +(Snowflake `column_stats` parity shipped separately in issue #258 — out of +scope for epic #219.) **Cost guidance — read before running any live Databricks test.** The live target is **Databricks Free Edition** (serverless-only). Its single SQL diff --git a/plans/super/258-snowflake-column-stats.md b/plans/super/258-snowflake-column-stats.md new file mode 100644 index 00000000..213072bc --- /dev/null +++ b/plans/super/258-snowflake-column-stats.md @@ -0,0 +1,337 @@ +# 258 — Snowflake `column_stats` parity with Databricks + +## Meta + +- **Ticket:** https://github.com/wjduenow/SignalForge/issues/258 +- **Phase:** devolved +- **PR:** https://github.com/wjduenow/SignalForge/pull/262 +- **Branch:** `feature/258-snowflake-column-stats` +- **Worktree:** `../worktrees/SignalForge/258-snowflake-column-stats` +- **Base:** `dev` +- **Sessions:** 1 (2026-07-01) + +## What / Why + +Decision ticket, follow-up from #224. The Databricks adapter shipped `column_stats` +(aggregate-only profiling) in #224/#227; the Snowflake adapter (#122) deliberately +left it as `NotImplementedError`. This creates an asymmetry: Databricks supports +`safety: aggregate-only`, Snowflake does not. + +**Decide:** backfill `column_stats` for Snowflake (parity), or document the gap as +intentional. Architectural Commitment #3 ("warehouse-agnostic by design") and the +epic-#118 intent both point toward parity. + +## Discovery findings + +### The `ColumnStats` contract (unchanged — the shape to produce) +`signalforge.warehouse.models.ColumnStats` — `frozen=True`, no `extra="forbid"`: +- `count: int` (non-null count), `distinct: int`, `nulls: int` +- `min: ColumnMinMax = None`, `max: ColumnMinMax = None` (`int|float|str|bool|datetime|date|None`) +- `data_type: str` (raw warehouse type string) + +ABC signature (`base.py`): `column_stats(self, table, column) -> ColumnStats`. ABC +docstring promises context-manager batching (DEC-008: multiple columns of one table +flush as one query at first read; `RuntimeError` outside `with adapter:` per DEC-025). + +**Single consumer:** `signalforge.safety.aggregate.aggregate_columns` — called inside +`with adapter:`, loops per column, stores each `ColumnStats` under the real column +name (or `None` for redacted). Feeds `LLMRequest.aggregates` → drafter's +`_render_aggregate_section`. Drives `safety.mode: aggregate-only`. + +### Databricks impl (the ticket's suggested model) +- **One query per column** (does NOT honor the ABC batching contract — the connection-bound + adapters diverge from BigQuery here). `typeof(col)` inline gives `data_type`. +- SQL: `COUNT(col)`, `COUNT(DISTINCT col)`, `COUNT_IF(col IS NULL)`, `MIN`/`MAX`, + `MAX(typeof(col)) AS data_type` — single aggregate. +- Complex-type MIN/MAX skip (#227): orderable (`array`/`struct`/`binary`) → run with + MIN/MAX, **post-process nulls** them; non-orderable (`map`/`variant`) → Spark rejects + MIN/MAX at analysis time (`INVALID_ORDERING_TYPE`) → **catch `QuerySyntaxError`, retry + reduced aggregate** (no MIN/MAX). Databricks needs this dance because `typeof()` is only + known *after* the query runs. +- `try/finally` cursor close (fixed in PR #257). + +### BigQuery impl (the original reference) +- Context-manager **batching** (pending dict + results dict, flush on first read). +- Reads schema **up front** → pre-filters MIN/MAX for complex types (`GEOGRAPHY`, `JSON`, + `BYTES`, `ARRAY<>`, `STRUCT<>`, `RANGE<>`) — no failed-query retry needed. + +### Snowflake current state +- `column_stats` → `NotImplementedError` (references epic #118). +- Connection-bound session: `_get_connection()` lazily builds + pins `_active_session`; + `connection=` injection seam for tests. +- Cursor helpers: `_execute_scalar` closes in `try/finally` (correct); **`_execute`, + `_execute_to_dicts`, `run_test_sql` LEAK** (no `finally` close). Documented latent gap + in `warehouse-adapters.md`; convention is "every cursor-opening method closes in + try/finally" (Databricks fixed its equivalents in #257). +- Fold-then-quote: `_quote(ref)` / `_fold(id)` — `identifier_case="upper"`, per-component + quoting. No single-column `_quote_identifier` helper yet. +- `run_test_sql` decodes `ARRAY_AGG(OBJECT_CONSTRUCT(*))` VARIANT via `json.loads`; + uppercase aliases resolved case-insensitively (`{k.lower(): v}`). +- **Snowflake has NO runtime `typeof()` for base columns.** Data type comes from + `INFORMATION_SCHEMA.COLUMNS.DATA_TYPE` (catalog) — returns `NUMBER`, `TEXT`, `DATE`, + `TIMESTAMP_NTZ`, `ARRAY`, `OBJECT`, `VARIANT`, `GEOGRAPHY`, `GEOMETRY`, `BINARY`, … +- `map_snowflake_exception`: `TableNotFoundError` / `ColumnNotFoundError` / + `QuerySyntaxError` / `WarehouseAuthError`. + +### Key design divergence surfaced by discovery +Because Snowflake exposes the column type via the catalog **before** the aggregate runs, +it can **pre-filter MIN/MAX (BigQuery-style)** rather than doing Databricks' +post-process-nulling + reduced-aggregate-retry. A literal "mirror Databricks" is possible +but arguably *worse* for Snowflake — the retry dance exists only because Databricks lacks +up-front type info. See DEC-002. + +### Testing tiers (from #124 precedent) +- Offline: hand-fake behavior tests + `fakesnow` execution (INFORMATION_SCHEMA + basic + aggregates execute) + `sqlglot`-parse for the sub-cases fakesnow can't run. +- **Live gated `@pytest.mark.snowflake` + `SF_RUN_SNOWFLAKE=1`** — the #124/#227 lesson: + fakes/parse certify SHAPE, live certifies ACCEPTANCE. Complex-type MIN/MAX skip + especially (the #227 map/variant analysis-time failure a fake can't disprove). + +### Parity surfaces to flip (backfill case) +- `docs/warehouse-adapter-ops.md` (§ Known limitations lines ~732–735; § Snowflake ~935–938). +- `README.md` (lines 95, 489, 512 — the "one combination not yet implemented" language). +- `.claude/rules/warehouse-adapters.md` (the "left `column_stats` as `NotImplementedError`" + statements + #258 references). +- `CHANGELOG.md` `[Unreleased]`. + +## Scoping answers (session 1, 2026-07-01) + +- **Backfill?** → **Backfill (implement)** `column_stats` for Snowflake, enabling + `safety: aggregate-only`. +- **data_type source / complex-type skip?** → **Catalog pre-filter (BigQuery-style).** + Query `INFORMATION_SCHEMA.COLUMNS.DATA_TYPE` up front; omit MIN/MAX from the aggregate + for unorderable types. One lookup serves both the `data_type` field and the skip + decision. NO Databricks-style retry-on-failure. (See DEC-002.) +- **Cursor-leak fix scope?** → **Fix all three** (`_execute`, `_execute_to_dicts`, + `run_test_sql`) with `try/finally` in the same PR — documented convention, one-liners. + (See DEC-005.) +- **Test tiers?** → **Offline + gated live (full).** Hand-fake + fakesnow/sqlglot offline + PLUS gated `@pytest.mark.snowflake` + `SF_RUN_SNOWFLAKE=1` live cert (scalar + + complex-type `column_stats` + aggregate-only `generate` smoke), maintainer-run before + merge. (See DEC-006.) + +## Architecture Review (session 1) + +| Area | Rating | Finding | +|---|---|---| +| Security / SQL-injection | **pass** | Mirror `_get_num_rows` verbatim: `validate_identifier("column", column)` first; catalog WHERE embeds schema/table/column as string literals via `escape_bq_string_literal` (Snowflake uses backslash escaping — the BQ helper is correct, per the adapter's own `_render_partition_filter` note) with `UPPER(COLUMN_NAME)=UPPER('...')` case-insensitive match; `project=None` → unqualified `INFORMATION_SCHEMA`. `data_type` from catalog is used only for control-flow + the returned field + lazy-JSON logs — never re-interpolated into SQL. No injection vectors. | +| Correctness / Snowflake semantics | **concern → resolved** | (a) NULL count via **`COUNT(*) - COUNT(col)`** (standard, fakesnow-executable, portable). (b) **Decimal coercion** — Snowflake `NUMBER` → Python `Decimal`, NOT in `ColumnStats.min/max`'s `int\|float\|str\|bool\|datetime\|date` union → coerce `Decimal`→`float` before constructing (must-fix; see DEC-004). (c) Skip-set is a **conservative superset** — over-skip loses signal (harmless), under-skip raises; the gated live cert refines it (DEC-003). (d) The batching-vs-per-column axis is the one open decision → refinement Q. (e) `with adapter:` guard follows from the batching decision. | +| Performance | **concern → resolved by DEC-002** | Snowflake MUST catalog-lookup the type (no runtime `typeof`), so pure per-column = 1+2N queries. Mitigated by batching the catalog lookup per-table (all column types in one `INFORMATION_SCHEMA.COLUMNS` query) and — if full BQ-style batching is chosen — one aggregate for all columns (2 queries/table total). See refinement Q. | +| Data Model | **pass** | `ColumnStats` shape unchanged. Frozen, produced in-process, never read back from disk → **no drift detector** (mirror ingest-layer rule; document in test module docstring). | +| API Design | **pass** | ABC `column_stats(table, column) -> ColumnStats` signature unchanged. | +| Observability | **pass** | If batching: optional large-batch WARNING mirroring BQ's `_COLUMN_BATCH_WARN_AT` (DEC-023), lazy-JSON. No new log surfaces otherwise. Logger grep-gate already scans `warehouse/`. | +| Testing Strategy | **pass** | fakesnow executes `COUNT`/`COUNT(DISTINCT)`/`MIN`/`MAX`/`INFORMATION_SCHEMA` → default suite covers every branch (scalar, complex-skip, empty, error-map, folding, identifier-validation) via hand-fake + fakesnow. Gated `@pytest.mark.snowflake` live cert: scalar `column_stats` + complex-type + aggregate-only `generate` smoke against read-only TPCH (SELECT-only, no CTAS). No drift detector. | + +**No blockers.** One open architecture decision (batching model) → refinement. Everything else resolved with the decisions below. + +## Refinement Log — Decisions + +- **DEC-001 — Backfill `column_stats` for Snowflake.** Implement it (enable `safety: + aggregate-only` on Snowflake), rather than documenting the gap. *Rationale:* + Architectural Commitment #3 (warehouse-agnostic by design), epic-#118 parity intent, + Databricks precedent. Closes the last v0.2 Snowflake gap. + +- **DEC-002 — Catalog pre-filter for `data_type` + MIN/MAX skip (BigQuery-style), NOT + Databricks retry-on-failure.** Snowflake has no runtime `typeof()`; read the column type + from `INFORMATION_SCHEMA.COLUMNS.DATA_TYPE` up front, then omit MIN/MAX from the + aggregate for unorderable types. One lookup serves both the `data_type` field and the + skip decision. *Rationale:* deterministic, no failed-query round-trip; Snowflake knows + the type before running (unlike Databricks, whose retry dance exists only because Spark + computes `typeof` at runtime). + +- **DEC-003 — Conservative skip-set, validated live.** `_is_complex_snowflake_type` + returns True for `{ARRAY, OBJECT, VARIANT, GEOGRAPHY, GEOMETRY}` (UPPER-compared against + the catalog `DATA_TYPE`; strip any `<...>` parametric tail). BINARY treated as orderable + (Snowflake MIN/MAX supports it). *Rationale:* with pre-filter, an UNDER-skipped + unorderable column raises and (under full-batch) fails the whole batch; an OVER-skipped + orderable column merely returns `min=max=None` (harmless — geo min/max is meaningless + anyway). So bias to a superset. Mirrors BigQuery's `_is_complex_type` posture exactly; + correctness rests on the set being complete, which the **gated live complex-type cert + validates before merge** (the #227 lesson: only a live run settles which types the + warehouse actually rejects). *Escape hatch if live/production surfaces a mis-classified + type:* extend `_is_complex_snowflake_type` (preferred) or add a batch-level + reduced-aggregate retry — do NOT reach for per-column retry. + +- **DEC-004 — Coerce `Decimal` → `float` for min/max.** Snowflake `NUMBER` surfaces as + Python `Decimal`, which is NOT in `ColumnStats.min/max`'s `int|float|str|bool|datetime| + date|None` union. Coerce `isinstance(v, Decimal)` → `float(v)` before constructing + `ColumnStats`. *Rationale:* satisfies the type contract; float precision is ample for a + min/max shown in an LLM prompt. (BigQuery avoids this because its client pre-coerces + NUMERIC→float.) + +- **DEC-005 — NULL count via `COUNT(*) - COUNT(col)`.** *Rationale:* standard SQL, + fakesnow/DuckDB-executable offline, portable; avoids leaning on `COUNT_IF` + vendor-specifics. + +- **DEC-006 — Full BigQuery-style batching.** `__enter__`/`__exit__` initialise + `_column_stats_pending: dict[TableRef, list[str]]` + `_column_stats_results: + dict[TableRef, dict[str, ColumnStats]]` (reset to `None` in `__exit__`'s `finally`, + coexisting with the existing connection-session state). `column_stats` validates the + identifier, raises `RuntimeError` if pending/results is `None` (**DEC-025 guard — free + under this model**), returns a cache hit, else queues the column and calls + `_flush_column_stats_batch(table)`. The flush runs **1** `INFORMATION_SCHEMA.COLUMNS` + query for all the table's column types + **1** aggregate for every queued column (MIN/MAX + gated per-column by DEC-003) = 2 queries/table. Honors the ABC DEC-008 contract; fewest + Snowflake round-trips. Copy the `bigquery.py` `_flush_column_stats_batch` structure. + Optional large-batch WARNING mirroring BQ's `_COLUMN_BATCH_WARN_AT` (DEC-023), lazy-JSON. + +- **DEC-007 — Fix all three cursor leaks in the same PR.** Add `try/finally` cursor + `close()` to `_execute`, `_execute_to_dicts`, and `run_test_sql` (only `_execute_scalar` + currently closes). *Rationale:* documented convention ("every cursor-opening adapter + method closes in try/finally"; Databricks fixed its equivalents in PR #257); one-liners; + `column_stats` consumes `_execute` + `_execute_to_dicts` so the fix is on the critical + path anyway. + +- **DEC-008 — Offline covers all branches; gated live certifies acceptance.** Default + suite: hand-fake unit tests (every branch — scalar / complex-skip / empty / error-map / + folding / identifier-validation / Decimal-coercion) + fakesnow offline execution + (real DuckDB runs the aggregate + `INFORMATION_SCHEMA`) + sqlglot-parse for any + sub-case fakesnow can't run. Gated `@pytest.mark.snowflake` + `SF_RUN_SNOWFLAKE=1`: + scalar + complex-type `column_stats` (against an engineered **writable** table, since + read-only TPCH has no ARRAY/OBJECT/VARIANT column) + an aggregate-only `generate` smoke + (against read-only TPCH — `column_stats` is SELECT-only, no CTAS). **No drift detector** + (`ColumnStats` is frozen, produced in-process, never read back from disk — mirror the + ingest-layer rule; document in the test module docstring). Gated tests run `--no-cov`, so + the impl body must be fully covered by the default suite. + +- **DEC-009 — Flip the parity surfaces + close #258.** `docs/warehouse-adapter-ops.md` + (§ Known limitations ~732–735; § Snowflake ~935–938), `README.md` (lines ~95, ~489, + ~512 — the "one combination not yet implemented" language), `.claude/rules/ + warehouse-adapters.md` (the "left `column_stats` as `NotImplementedError`" statements + + the `#258` references), `CHANGELOG.md` `[Unreleased]`. The rules-file edit is + **orchestrator-only-writable** (Ralph workers can't write `.claude/`), so it lands in the + Patterns & Memory story; the worker-writable doc surfaces (ops doc / README / CHANGELOG) + land in US-005. + +## Detailed Breakdown + +**Files (all under the worktree):** +`src/signalforge/warehouse/adapters/snowflake.py`, +`tests/warehouse/test_snowflake_adapter.py`, +`tests/warehouse/_fake_snowflake.py`, +`tests/warehouse/test_snowflake_adapter_fakesnow.py`, +`tests/warehouse/test_snowflake_columnstats_live.py` (new, gated), +`tests/cli/test_e2e_snowflake_smoke.py` (extend), +`docs/warehouse-adapter-ops.md`, `README.md`, `CHANGELOG.md`, +`.claude/rules/warehouse-adapters.md` (orchestrator-only, P&M story). + +Validation command (every story): `uv sync --dev && uv run ruff check . && uv run ruff +format --check . && uv run pyright && uv run pytest`. + +--- + +### US-001 — Cursor-leak fix (`_execute` / `_execute_to_dicts` / `run_test_sql`) +- **Traces to:** DEC-007. +- **Description:** Wrap each of the three cursor-opening methods' execute/fetch in + `try/finally cursor.close()` (mirror `_execute_scalar` + the Databricks PR #257 shape). + For `_execute_to_dicts`, the close must happen AFTER `_rows_to_dicts` reads + `cursor.description` — outer `try/finally` (close) wrapping inner `try/except` (exception + mapping), per the Databricks pattern. +- **TDD:** extend `FakeSnowflakeConnection`/`_FakeSnowflakeCursor` to track `cursors` + + `closed` (mirror `_fake_databricks.py`); tests + `test_execute_closes_cursor_on_{success,failure}`, + `test_execute_to_dicts_closes_cursor_on_{success,failure}`, + `test_run_test_sql_closes_cursor_on_{success,failure}`. +- **Done when:** all three methods close the cursor on success AND failure; new tests pass; + validation green. +- **Depends on:** none. + +### US-002 — `column_stats` full-batch implementation + unit tests +- **Traces to:** DEC-001, DEC-002, DEC-003, DEC-004, DEC-005, DEC-006. +- **Description:** Replace the `NotImplementedError`. Add `__enter__`/`__exit__` batching + state; `column_stats` (validate → guard → cache → queue → flush); `_flush_column_stats_ + batch` (one `_get_column_types` catalog query mirroring `_get_num_rows` escaping + one + aggregate for all queued columns, MIN/MAX gated); `_get_column_types(table) -> + dict[str,str]`; module-level `_COMPLEX_SNOWFLAKE_TYPES` frozenset + + `_is_complex_snowflake_type`; `COUNT(*)-COUNT(col)` null count; `Decimal`→`float` + coercion; case-insensitive result-alias resolution (`{k.lower(): v}`); optional + large-batch WARNING (lazy-JSON). Copy the `bigquery.py` flush structure. +- **TDD (hand-fake, all default-suite for coverage):** `returns_populated_columnstats`, + `carries_through_string_and_none_minmax`, `query_shape_and_folding` (catalog + aggregate + SQL shape, `"`-quoting, UPPER-fold), `resolves_aliases_case_insensitively`, + `none/absent_data_type_coerces_to_empty_string`, `validates_column_identifier` (DEC-013, + no query issued), `two_part_table_quoting` (`project=None` → unqualified + `INFORMATION_SCHEMA`), `programming_error_maps_to_query_syntax_error`, + `column_not_found_maps_with_context`, `complex_type_nulls_min_max` (parametrized + ARRAY/OBJECT/VARIANT/GEOGRAPHY/GEOMETRY), `scalar_type_preserves_min_max`, + `empty_table` (count=0, min/max NULL), `decimal_min_max_coerced_to_float`, + `batches_all_queued_columns_in_one_flush`, `raises_runtime_error_outside_with_block` + (DEC-025 guard). +- **Done when:** `column_stats` returns a populated `ColumnStats`; every branch covered by + the default suite; validation green. +- **Depends on:** US-001. + +### US-003 — fakesnow offline execution tests +- **Traces to:** DEC-008. +- **Description:** Add fakesnow-executed cases to `test_snowflake_adapter_fakesnow.py` + driving the real adapter through DuckDB: scalar aggregate + `INFORMATION_SCHEMA.COLUMNS` + round-trip execute; `COUNT(DISTINCT)`/`MIN`/`MAX`/`COUNT(*)-COUNT(col)` execute end-to-end + on engineered rows (rule-semantic assertions, not value pins). sqlglot-parse fallback for + any sub-case fakesnow can't run (comment the gap inline). Add the "no drift detector — + frozen, in-process, never read back" note to the test module docstring. +- **Done when:** fakesnow cases pass under `uv run pytest -m snowflake --no-cov`; parse + fallbacks (if any) documented; validation green. +- **Depends on:** US-002. + +### US-004 — Gated live certification +- **Traces to:** DEC-008, DEC-003 (validates the skip-set). +- **Description:** New `tests/warehouse/test_snowflake_columnstats_live.py` + (`@pytest.mark.snowflake`, `SF_RUN_SNOWFLAKE=1` + conn-var `_skip_reason()` gating, + `--no-cov`): create an engineered **writable** table (mirror `test_snowflake_prune_live.py`) + with scalar + ARRAY/OBJECT/VARIANT columns; assert scalar `column_stats` populates + count/distinct/nulls/min/max/data_type and complex-type columns return `min=max=None` + without error (**this is the #227-style skip-set validation**). Extend + `test_e2e_snowflake_smoke.py` with an aggregate-only `generate` smoke against read-only + TPCH (`safety.mode: aggregate-only`, `prune.scope: full`) asserting exit 0 + diff sidecar + + no traceback. +- **Done when:** the gated live suite passes when run by the maintainer with creds; + self-skips cleanly without them; default suite unaffected. +- **Depends on:** US-002. + +### US-005 — Parity docs (ops doc + README + CHANGELOG) +- **Traces to:** DEC-009. +- **Description:** Flip `docs/warehouse-adapter-ops.md` (§ Known limitations ~732–735 and + § Snowflake ~935–938) from "unsupported" to "supported (issue #258)"; update `README.md` + lines ~95, ~489, ~512 (drop the "one combination not yet implemented" Snowflake carve-out + — `aggregate-only` now works on Snowflake too); add a `CHANGELOG.md` `[Unreleased]` entry. + (The `.claude/rules/warehouse-adapters.md` update is orchestrator-only → P&M story.) +- **Done when:** no doc/README/CHANGELOG surface still states Snowflake `column_stats` is + unimplemented; validation green (skill-parity gate unaffected — no CLI change). +- **Depends on:** US-002. + +### US-006 — Quality Gate — code review ×4 + CodeRabbit +- **Description:** Run the code reviewer 4× across the full changeset, fixing every real + bug each pass; run CodeRabbit; ensure validation passes after all fixes. Diverse angles: + correctness (SQL shape, Decimal coercion, batch-fails-on-bad-column interaction), + Snowflake semantics (skip-set completeness), tests (branch coverage + gated-live shape), + docs/parity. +- **Done when:** 4 review passes complete, all real findings fixed, validation green. +- **Depends on:** US-003, US-004, US-005. + +### US-007 — Patterns & Memory (priority 99) +- **Description:** Update `.claude/rules/warehouse-adapters.md` — flip the "Snowflake left + `column_stats` as `NotImplementedError`" statements + `#258` references to "shipped + (#258)"; record the durable lessons (catalog pre-filter vs Databricks retry; + full-batch-fails-on-one-bad-column → conservative skip-set validated live; `Decimal`→ + `float` coercion; `COUNT(*)-COUNT(col)` null count; the #258 close-out of the Snowflake + aggregate-only gap). Add a memory file if a cross-cutting lesson warrants it. **Done by + the orchestrator** (Ralph workers can't write `.claude/`). +- **Done when:** rules file reflects shipped parity; validation green. +- **Depends on:** US-006. + + + + +## Beads Manifest (devolved, session 1) + +- **Epic:** `bd_1-scaffolding-om0` +- **Tasks:** + - `bd_1-scaffolding-om0.1` — US-001 Cursor-leak fix (ready) + - `bd_1-scaffolding-om0.2` — US-002 column_stats full-batch impl + unit tests (dep .1) + - `bd_1-scaffolding-om0.3` — US-003 fakesnow offline execution tests (dep .2) + - `bd_1-scaffolding-om0.4` — US-004 Gated live certification (dep .2) + - `bd_1-scaffolding-om0.5` — US-005 Parity docs — ops/README/CHANGELOG (dep .2) + - `bd_1-scaffolding-om0.6` — US-006 Quality Gate (dep .3, .4, .5) + - `bd_1-scaffolding-om0.7` — US-007 Patterns & Memory — orchestrator-run (dep .6) +- **Worktree:** `../worktrees/SignalForge/258-snowflake-column-stats` +- **PR:** https://github.com/wjduenow/SignalForge/pull/262 diff --git a/src/signalforge/warehouse/adapters/_snowflake_client.py b/src/signalforge/warehouse/adapters/_snowflake_client.py index 3c5aa112..c3347379 100644 --- a/src/signalforge/warehouse/adapters/_snowflake_client.py +++ b/src/signalforge/warehouse/adapters/_snowflake_client.py @@ -94,11 +94,14 @@ def make_real_client( *, account: str, user: str, - password: str, + password: str | None = None, role: str | None = None, warehouse: str | None = None, database: str | None = None, schema: str | None = None, + private_key_path: str | None = None, + private_key_passphrase: str | None = None, + authenticator: str | None = None, ) -> _SnowflakeClientProtocol: # pragma: no cover - requires the SDK + live creds """Construct a real ``snowflake.connector`` connection. @@ -107,18 +110,36 @@ def make_real_client( ``snowflake-connector-python`` ships only under the ``[snowflake]`` optional-dependency extra. The single ``# type: ignore[import-not-found]`` for the SDK import is confined here per DEC-005. + + Auth precedence: **key-pair (JWT) auth** when ``private_key_path`` is set — + the connector reads the PEM private key from that path (``private_key_file``) + plus an optional ``private_key_passphrase`` (``private_key_file_pwd``); this + is the non-interactive path that works with MFA-enforced accounts (a bare + password login is rejected there). Otherwise password auth. An explicit + ``authenticator`` (e.g. ``externalbrowser`` for SSO) is passed through when + set. The key-pair / authenticator fields are the ones the #120 profile model + already parses and ``from_profile`` already threads onto the adapter. """ import snowflake.connector # type: ignore[import-not-found] - return snowflake.connector.connect( # type: ignore[no-any-return] - account=account, - user=user, - password=password, - role=role, - warehouse=warehouse, - database=database, - schema=schema, - ) + connect_kwargs: dict[str, Any] = { + "account": account, + "user": user, + "role": role, + "warehouse": warehouse, + "database": database, + "schema": schema, + } + if private_key_path: + connect_kwargs["private_key_file"] = private_key_path + if private_key_passphrase: + connect_kwargs["private_key_file_pwd"] = private_key_passphrase + elif password: + connect_kwargs["password"] = password + if authenticator: + connect_kwargs["authenticator"] = authenticator + + return snowflake.connector.connect(**connect_kwargs) # type: ignore[no-any-return] def map_snowflake_exception(exc: Exception, *, context: dict[str, Any] | None = None) -> Exception: diff --git a/src/signalforge/warehouse/adapters/snowflake.py b/src/signalforge/warehouse/adapters/snowflake.py index a5ccef65..c5311baa 100644 --- a/src/signalforge/warehouse/adapters/snowflake.py +++ b/src/signalforge/warehouse/adapters/snowflake.py @@ -42,21 +42,19 @@ ``COUNT(*)`` aggregate (plus ``ARRAY_AGG(OBJECT_CONSTRUCT(*))`` sample-row capture when ``capture_failures > 0``) and returns a typed :class:`TestResult`. -* :meth:`column_stats` still raises :class:`NotImplementedError` naming the - epic (#118) so the remaining v0.2 implementation work has a single grep - target (DEC-008). +* :meth:`column_stats` is implemented (#258) — BigQuery-style context-manager + batching: :meth:`__enter__` opens per-table pending / results caches, the + first read flushes ONE ``INFORMATION_SCHEMA.COLUMNS`` catalog lookup (the + ``data_type`` field + the MIN/MAX pre-filter) plus ONE aggregate for every + queued column, and :meth:`__exit__` resets the caches. Enables ``safety: + aggregate-only`` on Snowflake, closing the last v0.2 parity gap with the + Databricks adapter. * :meth:`estimate_query_bytes` is implemented (#130) — it runs ``EXPLAIN USING JSON `` and parses ``GlobalStats.bytesAssigned`` via the pure :func:`_parse_explain_json_bytes`, no longer inheriting the ABC degrade. * :meth:`WarehouseAdapter.from_profile` dispatches ``profile.type == - "snowflake"`` here so an operator with a Snowflake profile sees a - ``NotImplementedError`` rather than the v0.1 - :class:`UnsupportedProfileTypeError`. - -Still pending (NOT implemented here): - -* :meth:`column_stats` — raises :class:`NotImplementedError` naming the epic - (#118); the per-column profiling path lands in a later v0.2 issue. + "snowflake"`` here so an operator with a Snowflake profile is routed to this + adapter rather than the v0.1 :class:`UnsupportedProfileTypeError`. The ``snowflake.connector`` import stays confined to :mod:`signalforge.warehouse.adapters._snowflake_client` (the one-shim-per-vendor @@ -70,6 +68,7 @@ import logging import time from datetime import date, datetime +from decimal import Decimal from typing import TYPE_CHECKING, Any from signalforge.warehouse._sample_id import _compute_run_id, _hash_session_id @@ -77,6 +76,7 @@ from signalforge.warehouse._sql_safety import validate_identifier, validate_test_sql from signalforge.warehouse.base import WarehouseAdapter from signalforge.warehouse.errors import ( + ColumnNotFoundError, EstimateUnavailableError, MaterialisationFailedError, SamplingRequiresPartitionFilterError, @@ -116,7 +116,73 @@ # compute, so ``_session_started_at`` is recorded for provenance only. _monotonic = time.monotonic -_V02_REMEDIATION = "SnowflakeAdapter is a v0.2 skeleton (issue #118) — full implementation pending." +_COLUMN_BATCH_WARN_AT: int = 500 +"""DEC-006 soft-warning threshold (mirrors BigQuery's ``_COLUMN_BATCH_WARN_AT``). +Queued ``column_stats`` columns above this count produce one ``WARNING`` per +flush. Module-level so tests can patch it down to (e.g.) 5 columns.""" + +_COMPLEX_SNOWFLAKE_TYPES: frozenset[str] = frozenset( + {"ARRAY", "OBJECT", "VARIANT", "GEOGRAPHY", "GEOMETRY"} +) +"""DEC-003 complex Snowflake types where ``MIN`` / ``MAX`` RAISES at SQL analysis +and is omitted from the aggregate. Conservative superset (DEC-003): an OVER-skipped +orderable column merely returns ``min=max=None`` (harmless), whereas an UNDER-skipped +unorderable column would raise and fail the whole batch. The gated live complex-type +cert validates the set is complete (the #227 lesson: only a live run settles which +types the warehouse rejects). + +This set is the *SQL-analysis-raise* tier. A distinct hazard — types that are +SQL-orderable (so ``MIN`` / ``MAX`` runs) but return a Python value outside the +``ColumnStats.min`` / ``max`` union (``BINARY`` → ``bytearray``, ``TIME`` → +``datetime.time``) — is handled by :func:`_coerce_min_max` nulling the value on +read-back, NOT by this set. So ``BINARY`` / ``TIME`` are deliberately absent here.""" + + +def _is_complex_snowflake_type(type_str: str) -> bool: + """Return True for Snowflake types where ``MIN`` / ``MAX`` is skipped (DEC-003). + + Compares ``type_str.upper().strip()`` against :data:`_COMPLEX_SNOWFLAKE_TYPES`, + then strips any ``<...>`` parametric tail and re-checks the head — defensive + against a future parametric shape (Snowflake's ``INFORMATION_SCHEMA`` today + returns bare names like ``ARRAY`` / ``VARIANT``, but the strip keeps the + check resilient). ``BINARY`` is orderable and returns ``False``. + """ + upper = type_str.upper().strip() + if upper in _COMPLEX_SNOWFLAKE_TYPES: + return True + head = upper.split("<", 1)[0].strip() + return head in _COMPLEX_SNOWFLAKE_TYPES + + +def _coerce_min_max(value: Any) -> Any: + """Coerce a min/max value into the ``ColumnStats.min`` / ``max`` union, or ``None``. + + ``ColumnStats.min`` / ``max`` is typed ``int|float|str|bool|datetime|date|None``. + A ``MIN`` / ``MAX`` result outside that union would raise a Pydantic + ``ValidationError`` at ``ColumnStats(...)`` construction — and because that is + NOT a :class:`~signalforge.warehouse.errors.WarehouseError`, it bypasses the + conservative-degrade path and fails the WHOLE batch (CLI panic tier). This is + the return-type analogue of :data:`_COMPLEX_SNOWFLAKE_TYPES`: the skip-set omits + ``MIN`` / ``MAX`` for types that RAISE at SQL analysis; this net nulls the values + of SQL-orderable types whose Python return type is unrepresentable. + + Three cases: + + * ``NUMBER`` → :class:`~decimal.Decimal` (DEC-004) → ``float`` (ample precision + for an LLM-prompt min/max). + * ``BINARY`` → ``bytearray`` and ``TIME`` → :class:`datetime.time` (and any other + future out-of-union type) → ``None``. Both types are SQL-orderable (so ``MIN`` / + ``MAX`` runs and they are deliberately NOT in :data:`_COMPLEX_SNOWFLAKE_TYPES`), + but neither is in the union. Nulling them matches the sibling adapters' posture + for their binary type (BigQuery ``BYTES`` / Databricks ``binary`` → + ``min=max=None``). Surfaced by the #258 quality gate (two independent reviewers). + * everything already in the union → passed through unchanged. + """ + if isinstance(value, Decimal): + return float(value) + if value is None or isinstance(value, (bool, int, float, str, datetime, date)): + return value + return None def _parse_explain_json_bytes(cell: str | dict[str, Any]) -> int: @@ -202,8 +268,8 @@ class SnowflakeAdapter(WarehouseAdapter): ``TEMPORARY TABLE``), and :meth:`run_test_sql` (``COUNT(*)`` failing-rows wrap), all on a connection wired via :meth:`_get_connection` with a fail-soft ``__exit__`` cleanup. :meth:`estimate_query_bytes` is implemented - (#130) via ``EXPLAIN USING JSON``. :meth:`column_stats` still raises - :class:`NotImplementedError` (a later v0.2 issue). + (#130) via ``EXPLAIN USING JSON``. :meth:`column_stats` is implemented + (#258) with BigQuery-style context-manager batching + a catalog pre-filter. """ def __init__( @@ -225,6 +291,11 @@ def __init__( # ``client=``). ``None`` triggers a lazy ``make_real_client(...)`` build # on first :meth:`_get_connection`; tests inject a fake. self._connection = connection + # True once we LAZILY BUILD a real connection — so cleanup may null it + # for a rebuild on the next ``with`` block. An INJECTED connection stays + # ``False`` and is never nulled (a fake is reused across blocks). See + # :meth:`_cleanup_active_session` (#258 — the aggregate-only double-`with`). + self._owns_connection = False self._account = account self._user = user self._password = password @@ -250,6 +321,16 @@ def __init__( self._active_session: _SnowflakeClientProtocol | None = None self._session_started_at: float | None = None + # DEC-006 of #258 — BigQuery-style context-manager batching state for + # ``column_stats``. ``None`` outside an active ``with`` block; populated + # to empty dicts on ``__enter__`` and reset to ``None`` on ``__exit__``. + # Coexists with the connection-session state above (which the + # ``__exit__`` cleanup tears down independently). The ``None`` sentinel + # is the DEC-025 guard: ``column_stats`` outside a ``with`` block sees + # ``None`` and raises ``RuntimeError``. + self._column_stats_pending: dict[TableRef, list[str]] | None = None + self._column_stats_results: dict[TableRef, dict[str, ColumnStats]] | None = None + def __repr__(self) -> str: # DEC-003: render ONLY non-credential identifying fields. NEVER user, # password, role, database, schema, private_key_path, @@ -280,12 +361,21 @@ def _get_connection(self) -> _SnowflakeClientProtocol: warehouse=self._warehouse, database=self._database, schema=self._schema, + private_key_path=self._private_key_path, + private_key_passphrase=self._private_key_passphrase, + authenticator=self._authenticator, ) + # We built it, so we own it — cleanup may null it for a rebuild. + self._owns_connection = True if self._active_session is None: self._active_session = self._connection return self._connection def __enter__(self) -> WarehouseAdapter: + # DEC-006 of #258 — open the column_stats batching caches for this + # ``with`` block (empty dicts; reset to ``None`` on ``__exit__``). + self._column_stats_pending = {} + self._column_stats_results = {} return self def __exit__(self, exc_type: object, exc: object, tb: object) -> None: @@ -293,7 +383,14 @@ def __exit__(self, exc_type: object, exc: object, tb: object) -> None: # connection ends the Snowflake session and reaps its session-scoped # temp tables. Failure is swallowed-and-warned; state always resets so # a subsequent ``__exit__`` is a no-op. - self._cleanup_active_session() + try: + self._cleanup_active_session() + finally: + # DEC-006 of #258 — reset the column_stats batching state so the + # next ``with`` block starts from a known-empty cache and a stray + # ``column_stats`` call outside a block re-trips the DEC-025 guard. + self._column_stats_pending = None + self._column_stats_results = None def _cleanup_active_session(self) -> None: """DEC-003 of #122 — best-effort, fail-soft session cleanup. @@ -344,16 +441,23 @@ def _cleanup_active_session(self) -> None: payload["session_id_hash"] = _hash_session_id(str(raw_session_id)) _LOGGER.info("session closed: %s", json.dumps(payload)) finally: - # Reset only the session-tracking state — NOT ``self._connection``. # Idempotency comes from the ``_active_session is None`` early-return - # above, so a second ``__exit__`` is a no-op regardless. Nulling - # ``self._connection`` here would be wrong: a later call would route - # back through ``_get_connection()``'s lazy-build branch and - # silently construct a *real* connection from (possibly empty) - # creds, discarding a test-injected fake — mirrors BigQuery's - # cleanup, which resets ``_active_session_id`` but never the client. + # above, so a second ``__exit__`` is a no-op regardless. self._active_session = None self._session_started_at = None + # For a connection we OWN (lazily built), null it after the close so + # a subsequent ``with adapter:`` on the SAME instance rebuilds a + # fresh connection instead of reusing the now-closed one (which + # raises ``250002 (08003): Connection is closed``). This is + # load-bearing for aggregate-only end-to-end (#258): ``generate`` + # with ``safety: aggregate-only`` enters two ``with adapter:`` blocks + # on one instance — the safety-aggregate ``column_stats`` pass, then + # ``prune_tests``. An INJECTED connection (tests) is deliberately + # left intact + non-nulled so a fake is reused across blocks, never + # silently rebuilt from (possibly empty) creds — the #122 concern. + if self._owns_connection: + self._connection = None + self._owns_connection = False def dialect(self) -> Dialect: return SNOWFLAKE_DIALECT @@ -552,6 +656,12 @@ def _execute(self, sql: str, *, table: TableRef) -> list[Any]: if mapped is exc: raise raise mapped from exc + finally: + # Release the server-side cursor handle on both the success and + # failure paths so repeated queries on the long-lived connection + # don't leak cursors (#258 US-001, mirroring _execute_scalar + + # the Databricks PR #257 shape). + cursor.close() def _execute_to_dicts(self, sql: str, *, table: TableRef) -> list[dict[str, Any]]: """Run ``sql`` and shape tuple ``fetchall()`` rows into dicts (DEC-010). @@ -564,14 +674,19 @@ def _execute_to_dicts(self, sql: str, *, table: TableRef) -> list[dict[str, Any] cursor = self._get_connection().cursor() try: - cursor.execute(sql) - rows = list(cursor.fetchall()) - except Exception as exc: - mapped = map_snowflake_exception(exc, context={"table": table.qualified_name}) - if mapped is exc: - raise - raise mapped from exc - return self._rows_to_dicts(cursor, rows) + try: + cursor.execute(sql) + rows = list(cursor.fetchall()) + except Exception as exc: + mapped = map_snowflake_exception(exc, context={"table": table.qualified_name}) + if mapped is exc: + raise + raise mapped from exc + # _rows_to_dicts reads cursor.description, so shape the rows BEFORE + # the finally closes the cursor (#258 US-001). + return self._rows_to_dicts(cursor, rows) + finally: + cursor.close() @staticmethod def _rows_to_dicts(cursor: _SnowflakeCursorProtocol, rows: list[Any]) -> list[dict[str, Any]]: @@ -855,14 +970,20 @@ def run_test_sql(self, sql: str, *, capture_failures: int = 0) -> TestResult: cursor = self._get_connection().cursor() try: - cursor.execute(wrapped) - rows = list(cursor.fetchall()) - description = cursor.description - except Exception as exc: - mapped = map_snowflake_exception(exc, context={}) - if mapped is exc: - raise - raise mapped from exc + try: + cursor.execute(wrapped) + rows = list(cursor.fetchall()) + description = cursor.description + except Exception as exc: + mapped = map_snowflake_exception(exc, context={}) + if mapped is exc: + raise + raise mapped from exc + finally: + # Release the server-side cursor handle on both paths (#258 US-001). + # ``rows`` / ``description`` are captured inside the inner try, so + # the post-processing below reads only locals — safe after close. + cursor.close() if not rows: # pragma: no cover - aggregate always returns one row raise RuntimeError("run_test_sql wrapper returned no rows") @@ -980,8 +1101,214 @@ def estimate_query_bytes(self, sql: str) -> int: raise EstimateUnavailableError(detail="EXPLAIN USING JSON returned no rows") return _parse_explain_json_bytes(cell) + # ------------------------------------------------------------------ + # column_stats — DEC-001 / DEC-002 / DEC-003 / DEC-004 / DEC-005 / + # DEC-006 of issue #258. + # ------------------------------------------------------------------ + def column_stats(self, table: TableRef, column: str) -> ColumnStats: - raise NotImplementedError(f"column_stats: {_V02_REMEDIATION}") + """Return a per-column profile, batched per-table inside a ``with``. + + Overrides the v0.2 ``NotImplementedError`` stub (issue #258 — Snowflake + parity with the Databricks ``column_stats``, DEC-001). Enables + ``safety: aggregate-only`` on Snowflake. + + Public contract per the ABC (DEC-008 of #22): one column at a time. + Inside an active context (``with adapter:``) calls accumulate per-table + and the first read flushes a single batched aggregate query for every + column queued for that table. Outside a context, raises + :class:`RuntimeError` (DEC-025 / DEC-006 of #258 — the ``None`` batching + sentinel is the guard). + + Mirrors :meth:`BigQueryAdapter.column_stats`'s batching model — the + divergence from Databricks (which runs one query per column with inline + ``typeof``) is deliberate: Snowflake exposes the column type via the + catalog (``INFORMATION_SCHEMA.COLUMNS.DATA_TYPE``) BEFORE the aggregate + runs, so it pre-filters ``MIN`` / ``MAX`` for unorderable types + (BigQuery-style, DEC-002) rather than Databricks' post-process-null + + reduced-aggregate retry dance. + """ + validate_identifier("column", column) + + if self._column_stats_pending is None or self._column_stats_results is None: + raise RuntimeError("column_stats must be called inside a `with adapter:` block") + + # Cache hit from a prior flush in this ``with`` block. + cached = self._column_stats_results.get(table, {}).get(column) + if cached is not None: + return cached + + pending = self._column_stats_pending.setdefault(table, []) + if column not in pending: + pending.append(column) + if len(pending) > _COLUMN_BATCH_WARN_AT: + # Lazy-format JSON per the warehouse-layer logger convention (the + # grep-gate forbids f-strings in ``_LOGGER`` calls). + _LOGGER.warning( + "Large column_stats batch: %s", + json.dumps({"columns": len(pending), "table": table.qualified_name}), + ) + + # "First read flushes every column queued for `table`." Subsequent calls + # in the same block hit the cache above; columns queued after a flush are + # flushed on the next call in turn (mirrors BigQuery's simplified + # first-access-flushes semantics). + self._flush_column_stats_batch(table) + + result = self._column_stats_results.get(table, {}).get(column) + if result is None: # pragma: no cover - defensive; flush populates this + raise RuntimeError(f"column_stats internal error: {column!r} missing from flush result") + return result + + def _get_column_types(self, table: TableRef) -> dict[str, str]: + """Look up ``{lower(COLUMN_NAME): DATA_TYPE}`` from + ``INFORMATION_SCHEMA.COLUMNS`` (DEC-002 / DEC-006 of #258). + + Mirrors :meth:`_get_num_rows`'s escaping / qualification exactly: the + schema / name are embedded as single-quoted STRING LITERALS via + :func:`escape_bq_string_literal` (backslash escaping inside single + quotes is correct for Snowflake) even though they are already + identifier-validated on the :class:`TableRef`; the ```` prefix + is quoted per the dialect, and when ``table.project`` is ``None`` the + query is left **unqualified** (resolved against the connection's current + database — ``CURRENT_DATABASE().INFORMATION_SCHEMA`` is invalid because + ``CURRENT_DATABASE()`` is a scalar function, not a namespace qualifier). + + The map is keyed by ``lower(COLUMN_NAME)`` so a requested manifest column + (dbt lowercases identifiers) resolves against the real upper-folded + Snowflake catalog entry case-insensitively. Rows with a NULL / absent + ``COLUMN_NAME`` or ``DATA_TYPE`` are skipped. + """ + from signalforge.warehouse._sql_safety import escape_bq_string_literal + + qc = SNOWFLAKE_DIALECT.quote_char + db_prefix = "" if table.project is None else f"{qc}{self._fold(table.project)}{qc}." + schema_lit = escape_bq_string_literal(table.dataset) + name_lit = escape_bq_string_literal(table.name) + sql = ( + f"SELECT COLUMN_NAME, DATA_TYPE FROM {db_prefix}INFORMATION_SCHEMA.COLUMNS " + f"WHERE UPPER(TABLE_SCHEMA) = UPPER('{schema_lit}') " + f"AND UPPER(TABLE_NAME) = UPPER('{name_lit}')" + ) + rows = self._execute_to_dicts(sql, table=table) + result: dict[str, str] = {} + for row in rows: + lowered = {str(k).lower(): v for k, v in row.items()} + name = lowered.get("column_name") + dtype = lowered.get("data_type") + if name is None or dtype is None: + continue + result[str(name).lower()] = str(dtype) + return result + + def _flush_column_stats_batch(self, table: TableRef) -> None: + """Issue the batched aggregate for every column queued for ``table``. + + Two queries per table per flush (DEC-006): one + ``INFORMATION_SCHEMA.COLUMNS`` catalog lookup for the column types (the + MIN/MAX pre-filter + the ``data_type`` field), then one aggregate over + the fold-then-quoted table computing ``COUNT`` / ``COUNT(DISTINCT)`` / + ``COUNT(*) - COUNT`` (null count, DEC-005) for every queued column, plus + ``MIN`` / ``MAX`` only for orderable (non-complex, DEC-003) columns. + + Aliases carry a stable per-column INDEX suffix (``count_0``, ``min_1``, + …) rather than the raw column name — two columns sharing a prefix (or a + column named ``count``) can't collide on an alias, and the index avoids + re-embedding a case-folded identifier into the result-key lookup. + + A requested column absent from the catalog map raises + :class:`ColumnNotFoundError` BEFORE the aggregate is issued — a stale + ``schema.yml`` referencing a dropped column fails loud rather than + silently profiling nothing. + """ + if self._column_stats_pending is None or self._column_stats_results is None: + return # pragma: no cover - guarded by caller + columns = list(self._column_stats_pending.get(table, [])) + if not columns: + return + + # Drain the attempted batch UP FRONT (#258 QG — Critical). If a column + # fails to resolve (``ColumnNotFoundError``, below) OR the aggregate + # raises (e.g. the documented ``COUNT(DISTINCT)`` restriction on + # ``GEOGRAPHY`` / ``GEOMETRY``), the exception must NOT leave the + # offending column in the pending queue — otherwise every subsequent + # ``column_stats()`` call for a *different, valid* column of the same + # table would re-include the stuck column and fail identically, so one + # unprofilable column would silently poison ``column_stats`` for the + # whole rest of the table within the ``with`` block. Clearing before the + # query scopes any failure to the offending call alone. + self._column_stats_pending[table] = [] + + # DEC-002 — catalog pre-filter: one lookup serves both the ``data_type`` + # field and the MIN/MAX skip decision. + type_by_column = self._get_column_types(table) + + # Resolve every queued column's type up front so a missing column raises + # ColumnNotFoundError BEFORE the (billable) aggregate is issued. + resolved: list[tuple[str, str]] = [] + for col in columns: + dtype = type_by_column.get(col.lower()) + if dtype is None: + raise ColumnNotFoundError(table=table.qualified_name, column=col) + resolved.append((col, dtype)) + + qc = SNOWFLAKE_DIALECT.quote_char + quoted_table = self._quote(table) + select_fragments: list[str] = ["COUNT(*) AS row_count"] + for i, (col, col_type) in enumerate(resolved): + quoted_col = f"{qc}{self._fold(col)}{qc}" + select_fragments.extend( + [ + f"COUNT({quoted_col}) AS count_{i}", + # KNOWN LIMITATION (#258 QG): COUNT(DISTINCT) is emitted + # unconditionally (mirrors the BigQuery adapter). Snowflake + # forbids DISTINCT on GEOGRAPHY / GEOMETRY, so profiling a + # model that carries such a column fails the whole aggregate + # (a typed WarehouseError, not the panic-tier crash the + # _coerce_min_max net prevents) — those columns are not + # profilable via `safety: aggregate-only` today. Whether + # DISTINCT on VARIANT / ARRAY / OBJECT also raises is + # unsettled offline; the gated live complex-type cert + # (test_snowflake_columnstats_live.py) is where it is + # confirmed. See docs/warehouse-adapter-ops.md. + f"COUNT(DISTINCT {quoted_col}) AS distinct_{i}", + # DEC-005 — null count via COUNT(*) - COUNT(col) (standard + # SQL, fakesnow-executable, portable; avoids COUNT_IF). + f"(COUNT(*) - COUNT({quoted_col})) AS nulls_{i}", + ] + ) + if not _is_complex_snowflake_type(col_type): + select_fragments.extend( + [ + f"MIN({quoted_col}) AS min_{i}", + f"MAX({quoted_col}) AS max_{i}", + ] + ) + + sql = f"SELECT {', '.join(select_fragments)} FROM {quoted_table}" + rows = self._execute_to_dicts(sql, table=table) + if not rows: # pragma: no cover - aggregate always returns one row + raise RuntimeError(f"column_stats aggregate returned no rows for table {table}") + + # Snowflake folds unquoted aliases to UPPER (``COUNT_0`` etc.); resolve + # case-insensitively so the index-suffixed aliases map regardless of + # folding (and a DictCursor-style passthrough that preserved case). + lowered = {str(k).lower(): v for k, v in rows[0].items()} + results = self._column_stats_results.setdefault(table, {}) + for i, (col, col_type) in enumerate(resolved): + is_complex = _is_complex_snowflake_type(col_type) + results[col] = ColumnStats( + count=int(lowered[f"count_{i}"]), + distinct=int(lowered[f"distinct_{i}"]), + nulls=int(lowered[f"nulls_{i}"]), + # DEC-004 — coerce a NUMBER Decimal min/max to float. + min=None if is_complex else _coerce_min_max(lowered.get(f"min_{i}")), + max=None if is_complex else _coerce_min_max(lowered.get(f"max_{i}")), + data_type=col_type, + ) + # Pending was already drained up front (see the top of this method), so + # a follow-up call queues a fresh batch without re-flushing these. + _LOGGER.debug("Flushed column_stats batch for %s: %s", table, columns) __all__ = ["SNOWFLAKE_DIALECT", "SnowflakeAdapter"] diff --git a/tests/cli/test_e2e_snowflake_smoke.py b/tests/cli/test_e2e_snowflake_smoke.py index 1a08a36d..be4dc86c 100644 --- a/tests/cli/test_e2e_snowflake_smoke.py +++ b/tests/cli/test_e2e_snowflake_smoke.py @@ -117,13 +117,13 @@ _TRUTHY = frozenset({"1", "true", "yes", "on"}) -# The connection env vars the Snowflake adapter needs for password auth, plus -# the warehouse so the prune/sample queries have compute context. Mirrors -# ``tests/warehouse/test_snowflake_estimate_live.py``. +# The connection env vars the Snowflake adapter always needs (compute context + +# namespace). Auth is a separate axis: EITHER ``SNOWFLAKE_PASSWORD`` OR +# ``SNOWFLAKE_PRIVATE_KEY_PATH`` (key-pair / JWT — the non-interactive path an +# MFA-enforced account requires). Mirrors ``test_snowflake_columnstats_live.py``. _REQUIRED_CONN_VARS = ( "SNOWFLAKE_ACCOUNT", "SNOWFLAKE_USER", - "SNOWFLAKE_PASSWORD", "SNOWFLAKE_WAREHOUSE", ) @@ -134,6 +134,23 @@ def _snowflake_runs_enabled() -> bool: return os.environ.get("SF_RUN_SNOWFLAKE", "").lower() in _TRUTHY +def _auth_profile_fields() -> dict[str, object]: + """Auth fields for the generated ``profiles.yml``: key-pair (JWT) when + ``SNOWFLAKE_PRIVATE_KEY_PATH`` is set (MFA-exempt — required for + MFA-enforced accounts, where a bare password login is rejected), else + password. Threads through ``load_profile`` → ``DbtProfileTarget`` (#120) → + ``from_profile`` → the adapter's key-pair connect path (#258).""" + key_path = os.environ.get("SNOWFLAKE_PRIVATE_KEY_PATH") + if key_path: + fields: dict[str, object] = {"private_key_path": key_path} + if passphrase := os.environ.get("SNOWFLAKE_PRIVATE_KEY_PASSPHRASE"): + fields["private_key_passphrase"] = passphrase + if authenticator := os.environ.get("SNOWFLAKE_AUTHENTICATOR"): + fields["authenticator"] = authenticator + return fields + return {"password": os.environ["SNOWFLAKE_PASSWORD"]} + + def _skip_reason() -> str | None: """Return a skip-reason string if any required prerequisite is missing. @@ -154,6 +171,13 @@ def _skip_reason() -> str | None: for var in _REQUIRED_CONN_VARS: if not os.environ.get(var): return f"{var} required (Snowflake connection parameter for the live pipeline run)" + if not os.environ.get("SNOWFLAKE_PASSWORD") and not os.environ.get( + "SNOWFLAKE_PRIVATE_KEY_PATH" + ): + return ( + "SNOWFLAKE_PASSWORD or SNOWFLAKE_PRIVATE_KEY_PATH required " + "(key-pair / JWT auth is the non-interactive path for MFA-enforced accounts)" + ) return None @@ -190,7 +214,6 @@ def test_e2e_signalforge_generate_against_tpch_sf1( "type": "snowflake", "account": os.environ["SNOWFLAKE_ACCOUNT"], "user": os.environ["SNOWFLAKE_USER"], - "password": os.environ["SNOWFLAKE_PASSWORD"], "warehouse": os.environ["SNOWFLAKE_WAREHOUSE"], "database": "SNOWFLAKE_SAMPLE_DATA", "schema": "TPCH_SF1", @@ -198,6 +221,7 @@ def test_e2e_signalforge_generate_against_tpch_sf1( } 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)) @@ -301,3 +325,121 @@ def test_e2e_signalforge_generate_against_tpch_sf1( assert "Traceback" not in captured.err, ( f"stderr leaked a Python traceback (DEC-016 violation):\n{captured.err}" ) + + +@pytest.mark.snowflake +def test_e2e_signalforge_generate_aggregate_only_against_tpch_sf1( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Run ``signalforge generate`` with ``safety.mode: aggregate-only`` against + TPCH_SF1 — the live cert that Snowflake ``column_stats`` (issue #258) drives + the aggregate-only draft path end to end. + + ``aggregate-only`` invokes :meth:`SnowflakeAdapter.column_stats` for every + column sampled into the LLM payload — the v0.2 method the Snowflake adapter + left as ``NotImplementedError`` until #258 backfilled it (DEC-001). Before + #258 this configuration raised; this test proves the implemented + ``column_stats`` (BigQuery-style catalog pre-filter + batched aggregate, + DEC-002/DEC-006) profiles the read-only ``TPCH_SF1.CUSTOMER`` columns and + the pipeline completes clean. + + ``prune.scope: full`` is REQUIRED for the same reasons as the schema-only + sibling above: the read-only ``SNOWFLAKE_SAMPLE_DATA`` share rejects the + ``materialised`` ``CREATE TEMPORARY TABLE`` and the ``oneshot`` row-count + seam is out of scope here. ``column_stats`` itself is SELECT-only (an + ``INFORMATION_SCHEMA.COLUMNS`` lookup + a ``COUNT``/``MIN``/``MAX`` + aggregate) — no CTAS — so it works against the read-only share. + + Same FIVE-prerequisite gating as the schema-only sibling (this is a + full-stack warehouse + LLM test). Skips cleanly under ``pytest -m snowflake`` + when any prerequisite is missing; the maintainer runs it once before merge. + + Asserts the three invariants the plan (#258 US-004) pins for the + aggregate-only smoke: + + 1. ``signalforge.cli.main(...)`` returns ``0`` (the full draft → prune → + grade → diff pipeline completed with ``column_stats`` in the loop). + 2. ``/.signalforge/diff.json`` exists (a diff sidecar was + written). + 3. ``"Traceback" not in stderr`` (DEC-016 of ``cli-layer.md`` — no traceback + ever leaks). + + Traces to: #258 US-004 (aggregate-only ``generate`` smoke), DEC-008. + """ + if reason := _skip_reason(): + pytest.skip(reason) + + # Copy the read-only seed to ``tmp_path`` so the audit JSONLs + diff sidecar + # land in the per-run temp dir, not the committed fixture (DEC-008 of #10). + project_dir = copy_fixture_to_tmp(_FIXTURE_DIR, tmp_path) + + # Rewrite the per-run profile from env vars (same shape as the schema-only + # sibling — a structured mapping via ``yaml.safe_dump``, never raw-string + # interpolation of credentials). ``database`` / ``schema`` point at the + # read-only shared sample database; ``column_stats`` is SELECT-only so the + # read-only share is fine. + 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)) + + # The seed ships no ``signalforge.yml``; write one pinning + # ``safety.mode: aggregate-only`` (the #258 path under test) + + # ``prune.scope: full`` (both sample strategies are non-functional against + # the read-only share — see the schema-only sibling's rationale). + # ``column_stats`` runs a SELECT-only catalog lookup + aggregate, so it works + # against read-only TPCH. ``total_budget_seconds`` is bumped above the 300s + # default so the sequential grade calls fit at p99 LLM latency. + (project_dir / "signalforge.yml").write_text( + textwrap.dedent( + """\ + # Snowflake aggregate-only live e2e config (issue #258, US-004). + # ``safety.mode: aggregate-only`` invokes the #258 column_stats impl; + # ``prune.scope: full`` is load-bearing (sample-mode prune is not + # functional against the read-only SNOWFLAKE_SAMPLE_DATA share). + llm: + model: claude-sonnet-4-6 + safety: + mode: aggregate-only + prune: + scope: full + grade: + total_budget_seconds: 600 + """ + ) + ) + + exit_code = main( + [ + "generate", + _MODEL_UNIQUE_ID, + "--project-dir", + str(project_dir), + ] + ) + + # 1. Exit code 0 — the full pipeline completed with column_stats + # (aggregate-only) in the loop, without a typed-error escape. + assert exit_code == 0, f"expected clean exit; got exit_code={exit_code}" + + # 2. Diff sidecar landed at the default path — proves the pipeline ran all + # the way through diff after the aggregate-only draft. + sidecar = project_dir / ".signalforge" / "diff.json" + assert sidecar.is_file(), f"diff sidecar missing at {sidecar}" + + # 3. No traceback in stderr (DEC-016 of cli-layer.md — no traceback ever + # leaks even if the pipeline raised internally). + captured = capsys.readouterr() + assert "Traceback" not in captured.err, ( + f"stderr leaked a Python traceback (DEC-016 violation):\n{captured.err}" + ) diff --git a/tests/warehouse/_fake_snowflake.py b/tests/warehouse/_fake_snowflake.py index 5f8788cf..7374d925 100644 --- a/tests/warehouse/_fake_snowflake.py +++ b/tests/warehouse/_fake_snowflake.py @@ -68,6 +68,11 @@ def __init__(self, connection: FakeSnowflakeConnection) -> None: def description(self) -> list[Any] | None: return self._description + @property + def closed(self) -> bool: + """Whether :meth:`close` has been called (cursor-leak regression check).""" + return self._closed + def execute(self, command: str, *args: Any, **kwargs: Any) -> _FakeSnowflakeCursor: rows, description = self._connection._consume_execute(command) self._fetch_rows = rows @@ -103,6 +108,8 @@ def __init__( self._close_raises = close_raises self.close_call_count = 0 self._execute_expectations: list[_ExecuteExpectation] = [] + # Every cursor this connection vends, for cursor-leak regression checks. + self.cursors: list[_FakeSnowflakeCursor] = [] # ---- expectation API -------------------------------------------------- @@ -137,7 +144,9 @@ def assert_all_expectations_met(self) -> None: # ---- snowflake.connector connection surface --------------------------- def cursor(self) -> _FakeSnowflakeCursor: - return _FakeSnowflakeCursor(self) + cur = _FakeSnowflakeCursor(self) + self.cursors.append(cur) + return cur def close(self) -> None: self.close_call_count += 1 diff --git a/tests/warehouse/test_snowflake_adapter.py b/tests/warehouse/test_snowflake_adapter.py new file mode 100644 index 00000000..6400fec5 --- /dev/null +++ b/tests/warehouse/test_snowflake_adapter.py @@ -0,0 +1,797 @@ +"""Cursor-handle release + ``column_stats`` tests for :class:`SnowflakeAdapter` +(#258 US-001 / US-002). + +The ``column_stats`` section (US-002, DEC-001..DEC-006) drives the real adapter +through the injected :class:`FakeSnowflakeConnection`, queuing BOTH the +``INFORMATION_SCHEMA.COLUMNS`` catalog lookup AND the aggregate query per flush. +No drift detector: :class:`ColumnStats` is frozen, produced in-process, and never +read back from disk (mirrors the ingest-layer rule). + +Traces to DEC-007 of ``plans/super/258-snowflake-column-stats.md``. Before this +fix only :meth:`SnowflakeAdapter._execute_scalar` closed its cursor in a +``try/finally``; :meth:`_execute`, :meth:`_execute_to_dicts`, and +:meth:`run_test_sql` opened a cursor on the long-lived connection and never +released it — leaking server-side cursor handles across repeated queries. This +mirrors the Databricks PR #257 fix (``tests/warehouse/test_databricks_adapter.py`` +§ "Cursor-handle release"). + +Each test drives one of the three methods and asserts the handed-out cursor's +``closed`` is ``True`` afterwards — on the success path AND the failure path +(``expect_execute(returns=)``), proving the ``finally`` arm fires. + +Uses :class:`FakeSnowflakeConnection` (``tests/warehouse/_fake_snowflake.py``), +which tracks every cursor it vends via :attr:`cursors` and exposes ``closed`` +per cursor — never a ``MagicMock`` (``testing-signal.md``). +""" + +from __future__ import annotations + +import logging +from datetime import time +from decimal import Decimal +from typing import Any + +import pytest + +from signalforge.warehouse import SnowflakeAdapter +from signalforge.warehouse.adapters.snowflake import _is_complex_snowflake_type +from signalforge.warehouse.errors import ( + ColumnNotFoundError, + InvalidIdentifierError, + QuerySyntaxError, +) +from signalforge.warehouse.models import ColumnStats, TableRef +from tests.warehouse._fake_snowflake import FakeSnowflakeConnection + +# ``project`` follows :class:`TableRef`'s GCP-style project-id grammar +# (lowercase start, >= 6 chars); Snowflake quotes it verbatim per-component. +_TABLE = TableRef(project="mydatabase", dataset="SCH", name="ORDERS") + +_SIZE_QUERY = r"INFORMATION_SCHEMA\.TABLES" +_SAMPLE_QUERY = r"_sf_sample_hash" +_COUNT_QUERY = r"SELECT COUNT\(\*\) AS failures" + + +def _make_adapter(conn: FakeSnowflakeConnection) -> SnowflakeAdapter: + return SnowflakeAdapter(connection=conn) + + +# --------------------------------------------------------------------------- +# _execute — via get_row_count / _get_num_rows (the INFORMATION_SCHEMA lookup) +# --------------------------------------------------------------------------- + + +def test_execute_closes_cursor_on_success() -> None: + """``_execute`` releases the cursor after a successful query so repeated + queries on the long-lived connection don't leak server-side handles.""" + conn = FakeSnowflakeConnection() + conn.expect_execute(matching=_SIZE_QUERY, returns=[(10,)]) + adapter = _make_adapter(conn) + + adapter.get_row_count(_TABLE) + + assert conn.cursors, "expected the adapter to open at least one cursor" + assert all(c.closed for c in conn.cursors) + + +def test_execute_closes_cursor_on_failure() -> None: + """The cursor is released even when the query raises (the ``finally`` arm).""" + conn = FakeSnowflakeConnection() + conn.expect_execute(matching=_SIZE_QUERY, returns=RuntimeError("boom")) + adapter = _make_adapter(conn) + + with pytest.raises(RuntimeError): + adapter.get_row_count(_TABLE) + + assert conn.cursors and all(c.closed for c in conn.cursors) + + +# --------------------------------------------------------------------------- +# _execute_to_dicts — via sample_rows (the projection-subquery sample query) +# --------------------------------------------------------------------------- + + +def test_execute_to_dicts_closes_cursor_on_success() -> None: + """``sample_rows`` (via ``_execute_to_dicts``) closes the cursor only AFTER + ``cursor.description`` has been read to shape the rows (outer ``try/finally`` + wrapping the inner exception-mapping ``try/except``).""" + conn = FakeSnowflakeConnection() + conn.expect_execute(matching=_SIZE_QUERY, returns=[(1000,)]) + conn.expect_execute( + matching=_SAMPLE_QUERY, + returns=[(1, 10)], + description=[("ID",), ("AMOUNT",)], + ) + adapter = _make_adapter(conn) + + rows = adapter.sample_rows(_TABLE, 100) + + assert rows == [{"ID": 1, "AMOUNT": 10}] + assert conn.cursors and all(c.closed for c in conn.cursors) + + +def test_execute_to_dicts_closes_cursor_on_failure() -> None: + """The cursor is released even when the sample query raises.""" + conn = FakeSnowflakeConnection() + conn.expect_execute(matching=_SIZE_QUERY, returns=[(1000,)]) + conn.expect_execute(matching=_SAMPLE_QUERY, returns=RuntimeError("boom")) + adapter = _make_adapter(conn) + + with pytest.raises(RuntimeError): + adapter.sample_rows(_TABLE, 100) + + assert conn.cursors and all(c.closed for c in conn.cursors) + + +# --------------------------------------------------------------------------- +# run_test_sql — the COUNT(*) failing-rows wrap +# --------------------------------------------------------------------------- + + +def test_run_test_sql_closes_cursor_on_success() -> None: + """``run_test_sql`` releases its cursor after reading ``description`` and + the result rows (the ``finally`` arm added by #258 US-001).""" + conn = FakeSnowflakeConnection() + conn.expect_execute(matching=_COUNT_QUERY, returns=[(0,)], description=[("FAILURES",)]) + adapter = _make_adapter(conn) + + adapter.run_test_sql('SELECT "ID" FROM "DB"."SCH"."T" WHERE "ID" IS NULL') + + assert conn.cursors and all(c.closed for c in conn.cursors) + + +def test_run_test_sql_closes_cursor_on_failure() -> None: + """The cursor is released even when the wrapped query raises.""" + conn = FakeSnowflakeConnection() + conn.expect_execute(matching=_COUNT_QUERY, returns=RuntimeError("boom")) + adapter = _make_adapter(conn) + + with pytest.raises(RuntimeError): + adapter.run_test_sql('SELECT "ID" FROM "DB"."SCH"."T" WHERE "ID" IS NULL') + + assert conn.cursors and all(c.closed for c in conn.cursors) + + +# =========================================================================== +# column_stats (#258 US-002, DEC-001..DEC-006) — full-batch profiling. +# +# BigQuery-style context-manager batching with a Snowflake catalog pre-filter: +# ONE ``INFORMATION_SCHEMA.COLUMNS`` lookup (data_type + MIN/MAX skip decision) +# plus ONE aggregate over the fold-then-quoted table per flush. +# =========================================================================== + +_COLUMNS_QUERY = r"INFORMATION_SCHEMA\.COLUMNS" +_AGG_QUERY = r"COUNT\(DISTINCT" +_CATALOG_DESCRIPTION = [("COLUMN_NAME",), ("DATA_TYPE",)] + + +class _RecordingSnowflakeConnection(FakeSnowflakeConnection): + """A :class:`FakeSnowflakeConnection` that records every executed SQL string + (for query-shape / one-aggregate-per-flush assertions).""" + + def __init__(self, **kwargs: Any) -> None: + super().__init__(**kwargs) + self.executed: list[str] = [] + + def _consume_execute(self, sql: str) -> tuple[list[Any], list[Any] | None]: + self.executed.append(sql) + return super()._consume_execute(sql) + + +def _scalar_agg_description(n_columns: int) -> list[tuple[str]]: + """Aggregate DB-API descriptor for an all-scalar batch of ``n_columns``. + + Mirrors ``_flush_column_stats_batch``'s SELECT order: a leading + ``ROW_COUNT`` then five index-suffixed aliases per column (count / distinct / + nulls / min / max). Snowflake folds aliases to UPPER, so the descriptor uses + the upper-cased forms (the adapter lowercases result keys before reading).""" + desc: list[tuple[str]] = [("ROW_COUNT",)] + for i in range(n_columns): + desc.extend( + [(f"COUNT_{i}",), (f"DISTINCT_{i}",), (f"NULLS_{i}",), (f"MIN_{i}",), (f"MAX_{i}",)] + ) + return desc + + +# --------------------------------------------------------------------------- +# _is_complex_snowflake_type — DEC-003 skip-set. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("type_str", "expected"), + [ + ("ARRAY", True), + ("OBJECT", True), + ("VARIANT", True), + ("GEOGRAPHY", True), + ("GEOMETRY", True), + # Case / whitespace insensitivity. + ("array", True), + (" Variant ", True), + # Parametric tail stripped then re-checked (defensive; Snowflake today + # returns bare names). + ("ARRAY", True), + # BINARY is orderable — MIN/MAX runs. + ("BINARY", False), + ("NUMBER", False), + ("TEXT", False), + ("TIMESTAMP_NTZ", False), + ("DATE", False), + ("BOOLEAN", False), + ], +) +def test_is_complex_snowflake_type(type_str: str, expected: bool) -> None: + """DEC-003 — the complex-type set drives the MIN/MAX skip; BINARY is + orderable and NOT in the set.""" + assert _is_complex_snowflake_type(type_str) is expected + + +# --------------------------------------------------------------------------- +# Happy path — populated ColumnStats. +# --------------------------------------------------------------------------- + + +def test_column_stats_returns_populated_columnstats() -> None: + """One catalog lookup + one aggregate shape into a fully-populated + :class:`ColumnStats`, mapped field-for-field.""" + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(1000, 900, 750, 100, 1, 9999)], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "amount") + + assert isinstance(stats, ColumnStats) + assert stats.count == 900 + assert stats.distinct == 750 + assert stats.nulls == 100 + assert stats.min == 1 + assert stats.max == 9999 + assert stats.data_type == "NUMBER" + conn.assert_all_expectations_met() + + +def test_column_stats_carries_through_string_and_none_minmax() -> None: + """STRING min/max bounds pass through; a NULL min/max (all-null column) + surfaces as ``None``.""" + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("REGION", "TEXT")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(500, 0, 0, 500, None, None)], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "region") + + assert stats.count == 0 + assert stats.distinct == 0 + assert stats.nulls == 500 + assert stats.min is None + assert stats.max is None + assert stats.data_type == "TEXT" + + +def test_column_stats_query_shape_and_folding() -> None: + """The catalog + aggregate SQL fold-then-quote every identifier, use the + ``COUNT(*) - COUNT(col)`` null count (DEC-005), and reuse ``_quote`` for the + three-part FROM.""" + conn = _RecordingSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(1, 1, 1, 0, 5, 5)], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + adapter.column_stats(_TABLE, "amount") + + catalog_sql = conn.executed[0] + assert catalog_sql.startswith("SELECT COLUMN_NAME, DATA_TYPE FROM") + assert '"MYDATABASE".INFORMATION_SCHEMA.COLUMNS' in catalog_sql + assert "UPPER(TABLE_SCHEMA) = UPPER('SCH')" in catalog_sql + assert "UPPER(TABLE_NAME) = UPPER('ORDERS')" in catalog_sql + + agg_sql = conn.executed[1] + # Fold-then-quote: "amount" folds to UPPER before the quote chars. + assert 'COUNT("AMOUNT") AS count_0' in agg_sql + assert 'COUNT(DISTINCT "AMOUNT") AS distinct_0' in agg_sql + assert '(COUNT(*) - COUNT("AMOUNT")) AS nulls_0' in agg_sql + assert 'MIN("AMOUNT") AS min_0' in agg_sql + assert 'MAX("AMOUNT") AS max_0' in agg_sql + assert agg_sql.endswith('FROM "MYDATABASE"."SCH"."ORDERS"') + assert "amount" not in agg_sql # never the raw lowercase identifier + + +def test_column_stats_resolves_aliases_case_insensitively() -> None: + """A connection that preserved the alias case (UPPER) still maps — the + adapter lowercases the result keys before reading them.""" + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("QTY", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(10, 7, 4, 3, 0, 10)], + # Explicit UPPER aliases (Snowflake's real folding). + description=[ + ("ROW_COUNT",), + ("COUNT_0",), + ("DISTINCT_0",), + ("NULLS_0",), + ("MIN_0",), + ("MAX_0",), + ], + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "qty") + + assert stats.count == 7 + assert stats.distinct == 4 + assert stats.nulls == 3 + assert stats.min == 0 + assert stats.max == 10 + assert stats.data_type == "NUMBER" + + +def test_column_stats_empty_table() -> None: + """An empty table still carries the column in the catalog, so the aggregate + yields count=0 / min=max=NULL with a real ``data_type``.""" + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(0, 0, 0, 0, None, None)], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "amount") + + assert stats.count == 0 + assert stats.distinct == 0 + assert stats.nulls == 0 + assert stats.min is None + assert stats.max is None + assert stats.data_type == "NUMBER" + + +# --------------------------------------------------------------------------- +# Identifier validation + guard. +# --------------------------------------------------------------------------- + + +def test_column_stats_validates_column_identifier() -> None: + """A malformed column name is rejected by ``validate_identifier`` BEFORE any + query is issued (DEC-013) — no expectation is consumed.""" + conn = FakeSnowflakeConnection() + + with _make_adapter(conn) as adapter, pytest.raises(InvalidIdentifierError): + adapter.column_stats(_TABLE, "amount; DROP TABLE x") + + assert not conn.cursors # nothing executed + + +def test_column_stats_raises_runtime_error_outside_with_block() -> None: + """DEC-025 guard — outside a ``with adapter:`` block the batching caches are + ``None`` and ``column_stats`` raises ``RuntimeError`` before any query.""" + conn = FakeSnowflakeConnection() + adapter = _make_adapter(conn) # NOT entered as a context manager + + with pytest.raises(RuntimeError, match="with adapter"): + adapter.column_stats(_TABLE, "amount") + + assert not conn.cursors + + +# --------------------------------------------------------------------------- +# Table quoting. +# --------------------------------------------------------------------------- + + +def test_column_stats_two_part_table_quoting() -> None: + """``project=None`` yields an unqualified ``INFORMATION_SCHEMA.COLUMNS`` + catalog query and a two-part ``"SCH"."ORDERS"`` aggregate FROM clause.""" + two_part = TableRef(project=None, dataset="SCH", name="ORDERS") + conn = _RecordingSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(1, 1, 1, 0, 5, 5)], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + adapter.column_stats(two_part, "amount") + + catalog_sql = conn.executed[0] + # Unqualified INFORMATION_SCHEMA (no leading "." prefix). + assert "FROM INFORMATION_SCHEMA.COLUMNS" in catalog_sql + assert '"."INFORMATION_SCHEMA' not in catalog_sql + assert conn.executed[1].endswith('FROM "SCH"."ORDERS"') + + +# --------------------------------------------------------------------------- +# Error mapping. +# --------------------------------------------------------------------------- + + +def test_column_stats_programming_error_maps_to_query_syntax_error() -> None: + """A connector ``ProgrammingError`` from the aggregate maps to + :class:`QuerySyntaxError` via ``map_snowflake_exception``.""" + pytest.importorskip("snowflake.connector") + from snowflake.connector import errors as sfe + + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=sfe.ProgrammingError("SQL compilation error: bad aggregate"), + ) + + with _make_adapter(conn) as adapter, pytest.raises(QuerySyntaxError): + adapter.column_stats(_TABLE, "amount") + + +def test_column_stats_column_not_found_maps_with_context() -> None: + """A column absent from the catalog map raises :class:`ColumnNotFoundError` + (carrying the table + column context) BEFORE the aggregate is issued.""" + conn = FakeSnowflakeConnection() + # Catalog returns a DIFFERENT column, so the requested one is absent. + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + + with _make_adapter(conn) as adapter, pytest.raises(ColumnNotFoundError) as exc_info: + adapter.column_stats(_TABLE, "ghost") + + assert exc_info.value.column == "ghost" + assert exc_info.value.table == _TABLE.qualified_name + # Only the catalog query ran; the aggregate was never issued. + assert len(conn.cursors) == 1 + + +# --------------------------------------------------------------------------- +# Complex-type MIN/MAX skip (DEC-003) + Decimal coercion (DEC-004). +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("data_type", ["ARRAY", "OBJECT", "VARIANT", "GEOGRAPHY", "GEOMETRY"]) +def test_column_stats_complex_type_nulls_min_max(data_type: str) -> None: + """A complex Snowflake type omits MIN/MAX from the emitted aggregate and + returns ``min=max=None`` (DEC-003).""" + conn = _RecordingSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("PAYLOAD", data_type)], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + # row_count, count_0, distinct_0, nulls_0 — no MIN/MAX for a complex col. + returns=[(42, 40, 2, 0)], + description=[("ROW_COUNT",), ("COUNT_0",), ("DISTINCT_0",), ("NULLS_0",)], + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "payload") + + assert stats.count == 40 + assert stats.distinct == 2 + assert stats.min is None + assert stats.max is None + assert stats.data_type == data_type + # MIN/MAX absent from the emitted SQL. + agg_sql = conn.executed[1] + assert "MIN(" not in agg_sql + assert "MAX(" not in agg_sql + + +def test_column_stats_scalar_type_preserves_min_max() -> None: + """A scalar (orderable) type keeps MIN/MAX in the aggregate and the result.""" + conn = _RecordingSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("TS", "TIMESTAMP_NTZ")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(5, 5, 4, 1, "2020-01-01", "2020-12-31")], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "ts") + + assert stats.min == "2020-01-01" + assert stats.max == "2020-12-31" + assert stats.data_type == "TIMESTAMP_NTZ" + assert 'MIN("TS") AS min_0' in conn.executed[1] + + +def test_column_stats_decimal_min_max_coerced_to_float() -> None: + """A NUMBER column whose aggregate returns ``Decimal`` min/max is coerced to + ``float`` (DEC-004 — ``Decimal`` is not in the ``ColumnStats`` union).""" + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("PRICE", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + # row_count, count_0, distinct_0, nulls_0, min_0, max_0. + returns=[(3, 3, 3, 0, Decimal("1.50"), Decimal("99.99"))], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "price") + + assert isinstance(stats.min, float) + assert isinstance(stats.max, float) + assert stats.min == 1.50 + assert stats.max == 99.99 + + +def test_column_stats_binary_min_max_coerced_to_none() -> None: + """BINARY is SQL-orderable (MIN/MAX runs, NOT in the skip set), but the + connector returns ``bytearray`` — outside the ``ColumnStats`` union. The + coercion net nulls it rather than raising a ValidationError that would fail + the whole batch (#258 QG). count/distinct/nulls still populate.""" + conn = _RecordingSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("BLOB", "BINARY")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + # min_0 / max_0 come back as bytearray (non-utf8 blob) from the connector. + returns=[(4, 4, 3, 1, bytearray(b"\x00\x01"), bytearray(b"\xff\xfe"))], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "blob") + + # MIN/MAX ARE emitted for BINARY (not a skip-set type) ... + assert 'MIN("BLOB") AS min_0' in conn.executed[1] + # ... but the bytearray result is nulled by _coerce_min_max. + assert stats.min is None + assert stats.max is None + assert stats.count == 4 + assert stats.distinct == 3 + assert stats.nulls == 1 + assert stats.data_type == "BINARY" + + +def test_column_stats_time_min_max_coerced_to_none() -> None: + """TIME is SQL-orderable but the connector returns ``datetime.time``, which + is not in the ``ColumnStats`` union — the coercion net nulls it (#258 QG).""" + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("T", "TIME")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(6, 6, 5, 1, time(8, 30), time(17, 45))], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "t") + + assert stats.min is None + assert stats.max is None + assert stats.count == 6 + assert stats.data_type == "TIME" + + +def test_column_stats_failed_column_does_not_poison_rest_of_table() -> None: + """A column that fails to resolve must not stay stuck in the pending queue: + a later ``column_stats`` for a DIFFERENT valid column of the same table must + still succeed (#258 QG — Critical). The pending batch is drained up front, so + the failed call's column is gone before the next call queues a fresh batch.""" + conn = FakeSnowflakeConnection() + # Call 1 ("bogus"): catalog lookup returns only AMOUNT — bogus is absent, so + # column_stats raises ColumnNotFoundError before any aggregate. + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + # Call 2 ("amount"): a FRESH catalog lookup + the aggregate. This only runs + # if the pending queue was drained after call 1's failure — otherwise the + # stuck "bogus" column would re-poison this flush and raise again. + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(3, 3, 3, 0, 1, 9)], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + with pytest.raises(ColumnNotFoundError): + adapter.column_stats(_TABLE, "bogus") + # Pre-fix, this would ALSO raise ColumnNotFoundError (bogus re-included). + stats = adapter.column_stats(_TABLE, "amount") + + assert stats.count == 3 + assert stats.data_type == "NUMBER" + + +# --------------------------------------------------------------------------- +# Batching — multiple columns in one aggregate flush. +# --------------------------------------------------------------------------- + + +def test_column_stats_batches_all_queued_columns_in_one_flush() -> None: + """Two columns queued before the first read flush in ONE aggregate query + (pre-seed ``_column_stats_pending``, mirroring the BigQuery batch test).""" + conn = _RecordingSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, + returns=[("AMOUNT", "NUMBER"), ("QTY", "NUMBER")], + description=_CATALOG_DESCRIPTION, + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(100, 90, 80, 10, 1, 9999, 95, 50, 5, 2, 500)], + description=_scalar_agg_description(2), + ) + + adapter = _make_adapter(conn) + with adapter: + assert adapter._column_stats_pending is not None + adapter._column_stats_pending[_TABLE] = ["amount", "qty"] + stats_a = adapter.column_stats(_TABLE, "amount") + # Second read hits the cache — no new query. + stats_b = adapter.column_stats(_TABLE, "qty") + + # amount is index 0, qty is index 1 (pending-list order). + assert stats_a.count == 90 + assert stats_a.min == 1 + assert stats_b.count == 95 + assert stats_b.max == 500 + + # Exactly ONE aggregate query (plus the one catalog lookup) covered both. + agg_queries = [s for s in conn.executed if "COUNT(DISTINCT" in s] + assert len(agg_queries) == 1 + assert '"AMOUNT"' in agg_queries[0] + assert '"QTY"' in agg_queries[0] + assert len(conn.executed) == 2 # catalog + single aggregate + conn.assert_all_expectations_met() + + +# --------------------------------------------------------------------------- +# Edge cases — large-batch WARNING, catalog-row skip, empty-batch no-op. +# --------------------------------------------------------------------------- + + +def test_column_stats_warns_on_large_batch( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Queuing more than ``_COLUMN_BATCH_WARN_AT`` columns emits ONE WARNING per + flush (DEC-006; threshold patched to 0 so a single column trips it).""" + monkeypatch.setattr("signalforge.warehouse.adapters.snowflake._COLUMN_BATCH_WARN_AT", 0) + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, returns=[("AMOUNT", "NUMBER")], description=_CATALOG_DESCRIPTION + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(1, 1, 1, 0, 5, 5)], + description=_scalar_agg_description(1), + ) + + with ( + caplog.at_level(logging.WARNING, logger="signalforge.warehouse"), + _make_adapter(conn) as adapter, + ): + adapter.column_stats(_TABLE, "amount") + + assert any("Large column_stats batch" in r.getMessage() for r in caplog.records) + + +def test_column_stats_skips_catalog_rows_with_null_name_or_type() -> None: + """Catalog rows with a NULL ``COLUMN_NAME`` or ``DATA_TYPE`` are skipped; a + valid row for the requested column still resolves.""" + conn = FakeSnowflakeConnection() + conn.expect_execute( + matching=_COLUMNS_QUERY, + returns=[(None, "NUMBER"), ("AMOUNT", None), ("AMOUNT", "NUMBER")], + description=_CATALOG_DESCRIPTION, + ) + conn.expect_execute( + matching=_AGG_QUERY, + returns=[(1, 1, 1, 0, 5, 5)], + description=_scalar_agg_description(1), + ) + + with _make_adapter(conn) as adapter: + stats = adapter.column_stats(_TABLE, "amount") + + assert stats.data_type == "NUMBER" + + +def test_flush_column_stats_batch_no_op_when_no_columns_queued() -> None: + """``_flush_column_stats_batch`` with nothing queued short-circuits before + any warehouse call (the empty-batch guard).""" + conn = FakeSnowflakeConnection() + adapter = _make_adapter(conn) + + with adapter: + adapter._flush_column_stats_batch(_TABLE) + + assert not conn.cursors + + +# --------------------------------------------------------------------------- +# Connection lifecycle across multiple `with adapter:` blocks (#258). +# `generate` with `safety: aggregate-only` enters two `with adapter:` blocks on +# one instance (safety-aggregate `column_stats`, then `prune_tests`); after the +# first `__exit__` closes the session, the second must not reuse a dead one. +# --------------------------------------------------------------------------- +def test_owned_connection_rebuilt_after_close_across_with_blocks( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A LAZILY-BUILT (owned) connection is nulled on ``__exit__`` so a second + ``with adapter:`` rebuilds a fresh one instead of reusing the closed + connection (250002: Connection is closed). Regression for the #258 + aggregate-only double-``with`` path.""" + built: list[FakeSnowflakeConnection] = [] + + def _fake_builder(**_kwargs: object) -> FakeSnowflakeConnection: + conn = FakeSnowflakeConnection() + built.append(conn) + return conn + + # Patch at the source module — ``_get_connection`` does a lazy + # ``from ..._snowflake_client import make_real_client`` at call time. + monkeypatch.setattr( + "signalforge.warehouse.adapters._snowflake_client.make_real_client", + _fake_builder, + ) + adapter = SnowflakeAdapter( + account="acct", user="usr", password="pw", warehouse="wh", database="db", schema="sc" + ) + + with adapter: + first = adapter._get_connection() + # Owned → nulled after close so the next block rebuilds. + assert adapter._connection is None + assert adapter._owns_connection is False + + with adapter: + second = adapter._get_connection() + + assert len(built) == 2, "expected a fresh connection to be rebuilt, not the closed one reused" + assert first is not second # the second block got a fresh connection, not the closed one + + +def test_injected_connection_not_nulled_after_close() -> None: + """An INJECTED connection is left intact (never nulled) across ``__exit__`` + so a fake is reused across blocks — the #122 concern the #258 owned-connection + rebuild deliberately preserves.""" + conn = FakeSnowflakeConnection() + adapter = SnowflakeAdapter(connection=conn) + + with adapter: + adapter._get_connection() + + assert adapter._connection is conn + assert adapter._owns_connection is False diff --git a/tests/warehouse/test_snowflake_adapter_fakesnow.py b/tests/warehouse/test_snowflake_adapter_fakesnow.py index f85f426a..fcb2c729 100644 --- a/tests/warehouse/test_snowflake_adapter_fakesnow.py +++ b/tests/warehouse/test_snowflake_adapter_fakesnow.py @@ -34,18 +34,39 @@ * The ``capture_failures`` wrap uses ``ARRAY_AGG(OBJECT_CONSTRUCT(*))``; fakesnow's DuckDB has no ``OBJECT_CONSTRUCT(*)`` analogue, so it is parse-only. +``column_stats`` (#258 US-003) is fully EXECUTABLE under fakesnow — both halves +of the batched flush run end-to-end against DuckDB: the +``INFORMATION_SCHEMA.COLUMNS`` catalog lookup (``_get_column_types`` — fakesnow +returns Snowflake-flavoured ``DATA_TYPE`` strings ``NUMBER`` / ``TEXT`` / +``VARIANT`` / ``TIMESTAMP_NTZ`` the adapter consumes verbatim) AND the aggregate +(``COUNT`` / ``COUNT(DISTINCT)`` / ``COUNT(*) - COUNT(col)`` null count / ``MIN`` +/ ``MAX``). So every ``column_stats`` sub-case below is an EXECUTION test, none +degrades to sqlglot-parse-only. The one thing fakesnow can NOT settle is whether +real Snowflake *rejects* ``MIN``/``MAX`` on an unorderable type at analysis time +— the adapter's DEC-003 catalog pre-filter SKIPS those aggregates, so the skip +DECISION is validated here (VARIANT → ``min=max=None``), but the warehouse's +actual rejection is the gated live cert's job (US-004; the #227 lesson). + +``ColumnStats`` needs NO drift detector — it is ``frozen=True``, produced +in-process, and never read back from disk (mirrors the ingest-layer rule; the +``extra="forbid"`` strict-mirror + fixture drift pattern is mandatory only for +JSONL/sidecar models re-read from disk, which ``ColumnStats`` is not). + Each parse-only sub-case carries an inline comment naming the fakesnow gap. Determinism is engineered by **rule semantics, not value-equality with real Snowflake** (``testing-signal.md`` § "Engineered determinism for LLM-driven assertions"): a ``not_null`` over a column with one NULL row returns ``failures >= 1``; over a column with no NULLs returns ``0`` — and so on for -``unique`` / ``accepted_values`` / ``relationships``. +``unique`` / ``accepted_values`` / ``relationships``; a ``column_stats`` over +engineered rows with a known NULL / distinct / min / max profile returns those +exact counts and bounds. Gated behind ``@pytest.mark.snowflake`` (excluded from the default ``addopts`` deselection); run with ``uv run pytest -m snowflake --no-cov``. -Traces to: plans/super/124-snowflake-test-harness-docs.md US-002. +Traces to: plans/super/124-snowflake-test-harness-docs.md US-002; +plans/super/258-snowflake-column-stats.md US-003 (DEC-008). """ from __future__ import annotations @@ -57,7 +78,7 @@ import pytest from signalforge.warehouse import SnowflakeAdapter -from signalforge.warehouse.models import TableRef +from signalforge.warehouse.models import ColumnStats, TableRef from tests.warehouse._fake_snowflake import FakeSnowflakeConnection pytestmark = pytest.mark.snowflake @@ -426,3 +447,157 @@ def test_get_num_rows_emitted_sql_parses_under_snowflake_dialect() -> None: adapter._get_num_rows(_TABLE) _parse_snowflake(conn.executed[0]) + + +# =========================================================================== +# EXECUTE: column_stats over engineered rows (#258 US-003, DEC-008). +# +# fakesnow's DuckDB backend executes BOTH halves of the column_stats flush: +# the ``INFORMATION_SCHEMA.COLUMNS`` catalog lookup (``_get_column_types`` — +# returning Snowflake-flavoured ``DATA_TYPE`` strings the adapter consumes +# verbatim) AND the aggregate (``COUNT`` / ``COUNT(DISTINCT)`` / +# ``COUNT(*) - COUNT(col)`` null count / ``MIN`` / ``MAX``). So the full adapter +# ``column_stats`` path runs end-to-end and every sub-case below is an EXECUTION +# test — NONE degrades to sqlglot-parse-only. Rule-semantic assertions on the +# engineered rows, never HASH()/value-equality with real Snowflake. +# +# ``ColumnStats`` needs NO drift detector — frozen, produced in-process, never +# read back from disk (see the module docstring; mirrors the ingest-layer rule). +# =========================================================================== + + +def _column_stats_in_block(conn: Any, table: TableRef, column: str) -> ColumnStats: + """Drive the real adapter's ``column_stats`` inside a ``with`` block. + + ``column_stats`` requires an active context (the ``None`` batching sentinel + is the DEC-025 guard), so it must run under ``with adapter:``. The block's + ``__exit__`` closes the fakesnow connection (reaping the session); the outer + ``_fakesnow_connection`` ``finally`` double-closes, which fakesnow tolerates. + The returned :class:`ColumnStats` is in-memory, so asserting on it after the + block is safe. + """ + adapter = SnowflakeAdapter(connection=conn) + with adapter: + return adapter.column_stats(table, column) + + +def test_column_stats_scalar_column_executes_end_to_end() -> None: + """A NUMBER column with a known NULL / distinct / min / max profile flows + through the real catalog lookup + aggregate to a populated + :class:`ColumnStats`. Exercises ``COUNT`` / ``COUNT(DISTINCT)`` / + ``COUNT(*) - COUNT(col)`` null count / ``MIN`` / ``MAX`` executing against + DuckDB, plus the DEC-004 ``Decimal`` → ``float`` coercion on the real + ``NUMBER`` bounds. Values ``(10.50, 20.50, 20.50, NULL)`` → + count=3, distinct=2, nulls=1, min=10.5, max=20.5.""" + with _fakesnow_connection() as conn: + _create_orders( + conn, + column_sql="AMOUNT NUMBER(10, 2)", + values_sql="(10.50), (20.50), (20.50), (NULL)", + ) + stats = _column_stats_in_block(conn, _TABLE, "amount") + + assert isinstance(stats, ColumnStats) + assert stats.count == 3 # non-null count + assert stats.distinct == 2 # 10.50 and 20.50 + assert stats.nulls == 1 # COUNT(*) - COUNT(col) = 4 - 3 + assert stats.min == 10.5 + assert stats.max == 20.5 + # DEC-004: NUMBER surfaces as Decimal from the connector; coerced to float. + assert isinstance(stats.min, float) + assert isinstance(stats.max, float) + # fakesnow returns Snowflake's bare ``DATA_TYPE`` name for NUMBER(10,2). + assert stats.data_type == "NUMBER" + + +def test_column_stats_catalog_lookup_returns_real_data_type() -> None: + """The ``INFORMATION_SCHEMA.COLUMNS`` lookup (``_get_column_types``) executes + against fakesnow's real catalog and returns consumable Snowflake ``DATA_TYPE`` + strings keyed by ``lower(COLUMN_NAME)``. fakesnow populates ``DATA_TYPE`` in + the same bare-name form real Snowflake does (``NUMBER`` / ``TEXT``), so this + is a full EXECUTION round-trip — NOT a parse-only degrade. + + ``_get_column_types`` runs a plain SELECT and needs no ``with`` block.""" + with _fakesnow_connection() as conn: + _create_orders( + conn, + column_sql="AMOUNT NUMBER(10, 2), REGION VARCHAR", + values_sql="(10.50, 'east'), (20.50, 'west')", + ) + adapter = SnowflakeAdapter(connection=conn) + types = adapter._get_column_types(_TABLE) + + # Keyed by lower(COLUMN_NAME); values are the real fakesnow-executed + # Snowflake DATA_TYPE strings (bare names, matching real Snowflake's + # INFORMATION_SCHEMA.COLUMNS.DATA_TYPE which carries precision separately). + assert types["amount"] == "NUMBER" + assert types["region"] == "TEXT" + + +def test_column_stats_string_min_max_executes() -> None: + """A VARCHAR column's ``MIN`` / ``MAX`` string bounds execute end-to-end and + pass through unchanged (no coercion). Values ``('east', 'west', 'east')`` → + count=3, distinct=2, nulls=0, min='east', max='west', data_type='TEXT'.""" + with _fakesnow_connection() as conn: + _create_orders( + conn, + column_sql="REGION VARCHAR", + values_sql="('east'), ('west'), ('east')", + ) + stats = _column_stats_in_block(conn, _TABLE, "region") + + assert stats.count == 3 + assert stats.distinct == 2 + assert stats.nulls == 0 + assert stats.min == "east" + assert stats.max == "west" + assert stats.data_type == "TEXT" + + +def test_column_stats_complex_type_skips_min_max_executes() -> None: + """A VARIANT column is DEC-003 complex, so the adapter OMITS ``MIN`` / ``MAX`` + from the aggregate (catalog pre-filter) — ``min`` / ``max`` return ``None`` + while ``count`` / ``distinct`` / ``nulls`` still execute. This validates the + SKIP DECISION end-to-end (fakesnow catalog reports ``VARIANT`` → adapter + skips), NOT the warehouse's analysis-time rejection of ``MIN(VARIANT)`` — + that (the #227 ``INVALID_ORDERING_TYPE`` failure a fake cannot reproduce) is + the gated live cert's job (US-004). Values ``(json, json, NULL)`` → + count=2, distinct=2, nulls=1, min=max=None.""" + with _fakesnow_connection() as conn: + _create_orders( + conn, + column_sql="PAYLOAD VARIANT", + # PARSE_JSON is the Snowflake way to seed a VARIANT literal; fakesnow + # supports it in a VALUES clause. + values_sql="(PARSE_JSON('{\"a\": 1}')), (PARSE_JSON('{\"b\": 2}')), (NULL)", + ) + stats = _column_stats_in_block(conn, _TABLE, "payload") + + assert stats.count == 2 + assert stats.distinct == 2 + assert stats.nulls == 1 + # MIN/MAX omitted from the aggregate for the complex type → None. + assert stats.min is None + assert stats.max is None + assert stats.data_type == "VARIANT" + + +def test_column_stats_all_null_column_executes() -> None: + """An all-NULL scalar column exercises the ``COUNT(*) - COUNT(col)`` null-count + arithmetic and the empty-aggregate ``MIN`` / ``MAX`` → ``None`` path against + real DuckDB. Values ``(NULL, NULL)`` → count=0, distinct=0, nulls=2, + min=max=None (data_type still resolved from the catalog).""" + with _fakesnow_connection() as conn: + _create_orders( + conn, + column_sql="AMOUNT NUMBER(10, 2)", + values_sql="(NULL), (NULL)", + ) + stats = _column_stats_in_block(conn, _TABLE, "amount") + + assert stats.count == 0 + assert stats.distinct == 0 + assert stats.nulls == 2 # COUNT(*) - COUNT(col) = 2 - 0 + assert stats.min is None + assert stats.max is None + assert stats.data_type == "NUMBER" diff --git a/tests/warehouse/test_snowflake_columnstats_live.py b/tests/warehouse/test_snowflake_columnstats_live.py new file mode 100644 index 00000000..868d5012 --- /dev/null +++ b/tests/warehouse/test_snowflake_columnstats_live.py @@ -0,0 +1,387 @@ +"""Gated live certification for Snowflake ``column_stats`` (#258 US-004). + +This is the **live certification for the ``column_stats`` implementation** — the +#258 backfill that gives Snowflake parity with Databricks and enables +``safety: aggregate-only`` on Snowflake (DEC-001). The offline hand-fake / +``fakesnow`` suite (``tests/warehouse/test_snowflake_adapter.py``, +``tests/warehouse/test_snowflake_adapter_fakesnow.py``) pins the compiled SQL's +*shape* + ``fakesnow`` execution of the scalar aggregate, but neither certifies +that a **real** Snowflake ACCEPTS the shape — chiefly the **DEC-003 complex-type +MIN/MAX skip-set**. ``fakesnow``'s DuckDB backend does not model Snowflake's +``ARRAY`` / ``OBJECT`` / ``VARIANT`` ``MIN`` / ``MAX`` rejection semantics, so +only a live run settles whether ``_COMPLEX_SNOWFLAKE_TYPES`` (``{ARRAY, OBJECT, +VARIANT, GEOGRAPHY, GEOMETRY}``) is complete — the #227 lesson restated for +Snowflake: **fakes/parse certify SHAPE, live certifies ACCEPTANCE.** An +UNDER-skipped unorderable column would raise and fail the whole batch (DEC-003), +so this test's complex-column assertions are the load-bearing skip-set +validation the plan calls out. + +NO LLM, NO ``generate`` CLI: this test builds a :class:`TableRef` in-process and +calls :meth:`SnowflakeAdapter.column_stats` directly inside a ``with adapter:`` +block (the ABC batching contract — DEC-025 requires the block; DEC-006 of #258 +opens the per-table batch caches on ``__enter__``). A **separate** short-lived +adapter does the engineered-table setup and the ``DROP TABLE`` teardown. + +The engineered table MUST live in a WRITABLE schema: the read-only +``SNOWFLAKE_SAMPLE_DATA`` share carries no ``ARRAY`` / ``OBJECT`` / ``VARIANT`` +column and cannot accept a ``CREATE TABLE`` — so, exactly like +``test_snowflake_prune_live.py``, the table is created in the maintainer's +``SNOWFLAKE_DATABASE.SNOWFLAKE_SCHEMA`` and dropped in ``finally``. + +**No drift detector** for :class:`ColumnStats`: it is frozen, produced +in-process, and never read back from disk (mirrors the ingest-layer rule + +DEC-008 of #258). + +Belt-and-suspenders gating (``.claude/rules/testing-signal.md`` § "End-to-end +gated tests") — identical to ``test_snowflake_prune_live.py``: + +1. ``@pytest.mark.snowflake`` — registered in ``pyproject.toml`` + ``[tool.pytest.ini_options].markers`` and deselected by the default + ``addopts`` (``-m '... and not snowflake'``), so the default ``pytest`` run + never collects this test. +2. A runtime :func:`_skip_reason` — when a maintainer runs ``pytest -m + snowflake`` but lacks credentials, each missing prerequisite surfaces as a + distinct skip-with-reason rather than a confusing connection error. + +Required env vars (each missing one yields its own distinct skip reason): + +* ``SF_RUN_SNOWFLAKE=1`` — the project-wide opt-in for "this test talks to a + real warehouse" (accepts ``1``/``true``/``yes``/``on``). +* ``SNOWFLAKE_ACCOUNT`` / ``SNOWFLAKE_USER`` / ``SNOWFLAKE_PASSWORD`` — the + minimal password-auth connection triple. +* ``SNOWFLAKE_WAREHOUSE`` — compute context for the engineered ``CREATE TABLE``, + the ``INFORMATION_SCHEMA.COLUMNS`` catalog lookup, and the per-column + aggregate. +* ``SNOWFLAKE_DATABASE`` + ``SNOWFLAKE_SCHEMA`` — the **WRITABLE** target where + the engineered table is created (and dropped in teardown). + +**Cost guidance — set a Snowflake resource monitor FIRST.** Before running, +create a resource monitor with a hard credit cap. Use an **XS (extra-small) +warehouse** with **aggressive auto-suspend** (e.g. 60 seconds). The engineered +table is a handful of rows, so the catalog lookup + per-column aggregate are +tiny; the dominant cost is warehouse spin-up. + +Run via the maintainer-only invocation (``--no-cov`` because ``--cov-fail-under`` +in ``addopts`` would fail a marker-specific run that exercises only a fraction of +the codebase):: + + export SF_RUN_SNOWFLAKE=1 + export SNOWFLAKE_ACCOUNT= + export SNOWFLAKE_USER= + export SNOWFLAKE_PASSWORD= + export SNOWFLAKE_WAREHOUSE= + export SNOWFLAKE_DATABASE= + export SNOWFLAKE_SCHEMA= + uv run pytest -m snowflake --no-cov + +Engineered determinism (``.claude/rules/testing-signal.md`` § "Engineered +determinism"): the assertions do NOT depend on any LLM output — the engineered +rows are hand-crafted, so every count / null / min / max assertion is +mathematically guaranteed. The scalar ``id`` / ``name`` columns are populated on +both rows (count=2, nulls=0); the complex ``tags`` / ``meta`` / ``payload`` +columns hold a value on row 1 and NULL on row 2 (count=1, nulls=1) so the +null-count and skip assertions are meaningful. + +Traces to: #258 US-004 (gated live cert of Snowflake ``column_stats`` — +scalar populate + complex-type MIN/MAX skip-set validation, DEC-003/DEC-008). +""" + +from __future__ import annotations + +import os +import uuid + +import pytest + +from signalforge.warehouse import ColumnStats, SnowflakeAdapter, TableRef + +_TRUTHY = frozenset({"1", "true", "yes", "on"}) + +# Connection env vars the setup / stats / teardown adapters always need, plus +# the writable namespace the engineered table is created in. Auth is a separate +# axis: EITHER ``SNOWFLAKE_PASSWORD`` OR ``SNOWFLAKE_PRIVATE_KEY_PATH`` (key-pair +# / JWT — the non-interactive path required by MFA-enforced accounts, since a +# bare password login is rejected there). +_REQUIRED_CONN_VARS = ( + "SNOWFLAKE_ACCOUNT", + "SNOWFLAKE_USER", + "SNOWFLAKE_WAREHOUSE", + "SNOWFLAKE_DATABASE", + "SNOWFLAKE_SCHEMA", +) + +# Engineered-table name PREFIX. The full name gets a per-run random suffix (see +# ``_unique_table_name``) so two concurrent maintainer runs against the same +# writable schema cannot race on the same ``DROP TABLE`` / clobber an unrelated +# leftover object. The prefix + suffix are a valid bare identifier (the strict +# DEC-013 regex used by ``TableRef``). +_ENGINEERED_TABLE_PREFIX = "sf_colstats_live_engineered" + + +def _unique_table_name() -> str: + """A per-run engineered-table name: prefix + 12 random hex chars.""" + return f"{_ENGINEERED_TABLE_PREFIX}_{uuid.uuid4().hex[:12]}" + + +def _snowflake_runs_enabled() -> bool: + """``SF_RUN_SNOWFLAKE`` is set to a truthy value (the Snowflake analogue of + the ``SF_RUN_BQ`` opt-in; accepts ``1``/``true``/``yes``/``on``).""" + return os.environ.get("SF_RUN_SNOWFLAKE", "").lower() in _TRUTHY + + +def _skip_reason() -> str | None: + """Return a skip-reason string if any required prerequisite is missing. + + Returns ``None`` only when the opt-in flag AND every connection env var is + present — the test then proceeds to make real Snowflake calls (CREATE TABLE, + an ``INFORMATION_SCHEMA.COLUMNS`` lookup, per-column aggregates, DROP TABLE). + Each missing prerequisite yields its own distinct reason so a maintainer + running ``pytest -m snowflake`` sees exactly what to set. + """ + if not _snowflake_runs_enabled(): + return "SF_RUN_SNOWFLAKE=1 required (live test talks to a real Snowflake warehouse)" + for var in _REQUIRED_CONN_VARS: + if not os.environ.get(var): + return ( + f"{var} required (Snowflake connection / writable-target parameter " + f"for the live column_stats e2e)" + ) + if not os.environ.get("SNOWFLAKE_PASSWORD") and not os.environ.get( + "SNOWFLAKE_PRIVATE_KEY_PATH" + ): + return ( + "SNOWFLAKE_PASSWORD or SNOWFLAKE_PRIVATE_KEY_PATH required " + "(key-pair / JWT auth is the non-interactive path for MFA-enforced accounts)" + ) + return None + + +def _make_adapter() -> SnowflakeAdapter: + """Construct a real :class:`SnowflakeAdapter` from the env vars. + + A fresh adapter is built per use (setup / stats / teardown) so a ``with`` + block's session close does not strand another phase's cursors — each adapter + owns its own connection. Auth is key-pair (JWT) when + ``SNOWFLAKE_PRIVATE_KEY_PATH`` is set (with optional + ``SNOWFLAKE_PRIVATE_KEY_PASSPHRASE``), else password. + """ + return SnowflakeAdapter( + account=os.environ["SNOWFLAKE_ACCOUNT"], + user=os.environ["SNOWFLAKE_USER"], + password=os.environ.get("SNOWFLAKE_PASSWORD"), + warehouse=os.environ["SNOWFLAKE_WAREHOUSE"], + database=os.environ["SNOWFLAKE_DATABASE"], + schema=os.environ["SNOWFLAKE_SCHEMA"], + role=os.environ.get("SNOWFLAKE_ROLE"), + private_key_path=os.environ.get("SNOWFLAKE_PRIVATE_KEY_PATH"), + private_key_passphrase=os.environ.get("SNOWFLAKE_PRIVATE_KEY_PASSPHRASE"), + authenticator=os.environ.get("SNOWFLAKE_AUTHENTICATOR"), + ) + + +def _quoted_table(database: str, schema: str, name: str) -> str: + """Per-component quoted, UPPER-folded Snowflake identifier (#124). + + Must fold to UPPER then quote — byte-identical to the adapter's ``_quote`` — + so the table this test CREATEs / DROPs directly is the same case-sensitive + object the adapter's ``column_stats`` catalog lookup + aggregate REFERENCE + (the adapter folds the :class:`TableRef` to UPPER before quoting). A + case-preserved helper would create ``"…"`` while the adapter + references the upper-folded ``"…"`` → "Table not found". + """ + return f'"{database.upper()}"."{schema.upper()}"."{name.upper()}"' + + +@pytest.mark.snowflake +def test_column_stats_live_scalar_populated_and_complex_skipped() -> None: + """Certify ``column_stats`` against a live engineered table: scalar columns + populate min/max, complex (ARRAY/OBJECT/VARIANT) columns return min=max=None + without raising (the DEC-003 skip-set validation). + + Skips cleanly under ``pytest -m snowflake`` when any prerequisite is + missing. With credentials present: + + 1. Creates a tiny engineered table in the writable + ``SNOWFLAKE_DATABASE.SNOWFLAKE_SCHEMA`` — scalar columns ``id`` (NUMBER) + and ``name`` (VARCHAR), populated on both rows; complex columns ``tags`` + (ARRAY), ``meta`` (OBJECT), ``payload`` (VARIANT), populated on row 1 and + NULL on row 2. Complex constructors (``ARRAY_CONSTRUCT`` etc.) are not + constant expressions, so the rows are inserted via ``INSERT … SELECT … + UNION ALL SELECT …`` rather than ``INSERT … VALUES``. + 2. Inside ``with adapter:`` (the ABC batching contract — DEC-025 / DEC-006 of + #258), calls :meth:`SnowflakeAdapter.column_stats` per column against a + :class:`TableRef` for the engineered table. + 3. Asserts the SCALAR columns return populated + ``count``/``distinct``/``nulls`` AND non-``None`` ``min``/``max`` AND a + non-empty ``data_type``. + 4. Asserts the COMPLEX columns return ``min is None`` and ``max is None`` + WITHOUT raising — the load-bearing #227-style skip-set validation + (DEC-003) — with populated ``count`` (1, one non-null row) / ``nulls`` + (1) and a non-empty ``data_type``. + 5. Tears the engineered table down with ``DROP TABLE IF EXISTS`` in a + ``finally`` (idempotent; tolerates a partial-setup failure). + """ + if reason := _skip_reason(): + pytest.skip(reason) + + database = os.environ["SNOWFLAKE_DATABASE"] + schema = os.environ["SNOWFLAKE_SCHEMA"] + # Per-run unique name so concurrent runs don't race on DROP / clobber. + table_name = _unique_table_name() + quoted = _quoted_table(database, schema, table_name) + + # The setup (CREATE/INSERT) lives INSIDE the outer ``try`` whose ``finally`` + # drops the table — so a mid-setup failure (e.g. the hand-crafted multi-branch + # INSERT) still hits teardown and never orphans the per-run table (#258 QG). + try: + # --- Setup: create + populate the engineered table (own short-lived adapter). + setup_adapter = _make_adapter() + with setup_adapter: + cursor = setup_adapter._get_connection().cursor() + try: + cursor.execute(f"DROP TABLE IF EXISTS {quoted}") + cursor.execute( + f"CREATE TABLE {quoted} " + f"(id NUMBER, name VARCHAR, tags ARRAY, meta OBJECT, payload VARIANT, " + f"blob BINARY, t TIME)" + ) + # Complex constructors are not constant expressions, so INSERT … + # VALUES is rejected — use INSERT … SELECT … UNION ALL SELECT …. + # Row 1 populates every column; row 2 leaves the ARRAY/OBJECT/VARIANT + # columns NULL (typed casts keep the UNION branch types unified) so + # they get count=1, nulls=1. ``blob`` (BINARY) and ``t`` (TIME) are + # populated on BOTH rows so their MIN/MAX return real ``bytearray`` / + # ``datetime.time`` values — exercising the ``_coerce_min_max`` net + # that nulls out-of-union return types (#258 QG). + cursor.execute( + f"INSERT INTO {quoted} (id, name, tags, meta, payload, blob, t) " + f"SELECT 1, 'alpha', ARRAY_CONSTRUCT(1, 2, 3), " + f"OBJECT_CONSTRUCT('k', 'v1'), TO_VARIANT(100), " + f"TO_BINARY('DEADBEEF', 'HEX'), '08:30:00'::TIME " + f"UNION ALL " + f"SELECT 2, 'bravo', NULL::ARRAY, NULL::OBJECT, NULL::VARIANT, " + f"TO_BINARY('CAFE', 'HEX'), '17:45:00'::TIME" + ) + finally: + cursor.close() + + # ``project`` = the writable database; ``dataset`` = the schema; ``name`` + # = the engineered table. The adapter folds each to UPPER before quoting, + # so the raw (lowercased) values here resolve to the same object created + # above (mirrors ``test_snowflake_prune_live.py``'s ``TableRef`` shape). + table_ref = TableRef(project=database, dataset=schema, name=table_name) + + # ``column_stats`` MUST run inside a ``with adapter:`` block — the + # ``__enter__`` opens the per-table batch caches; outside the block the + # DEC-025 guard raises ``RuntimeError``. + stats_adapter = _make_adapter() + with stats_adapter: + id_stats: ColumnStats = stats_adapter.column_stats(table_ref, "id") + name_stats: ColumnStats = stats_adapter.column_stats(table_ref, "name") + tags_stats: ColumnStats = stats_adapter.column_stats(table_ref, "tags") + meta_stats: ColumnStats = stats_adapter.column_stats(table_ref, "meta") + payload_stats: ColumnStats = stats_adapter.column_stats(table_ref, "payload") + blob_stats: ColumnStats = stats_adapter.column_stats(table_ref, "blob") + time_stats: ColumnStats = stats_adapter.column_stats(table_ref, "t") + + # --- Scalar columns: fully populated, min/max present. ---------------- + for label, stats in (("id", id_stats), ("name", name_stats)): + assert stats.count == 2, ( + f"scalar column {label!r}: both rows are populated, so " + f"count should be 2; got {stats.count} ({stats!r})" + ) + assert stats.distinct == 2, ( + f"scalar column {label!r}: two distinct values, so distinct " + f"should be 2; got {stats.distinct} ({stats!r})" + ) + assert stats.nulls == 0, ( + f"scalar column {label!r}: no NULLs, so nulls should be 0; " + f"got {stats.nulls} ({stats!r})" + ) + assert stats.min is not None, ( + f"scalar column {label!r}: MIN should be populated (orderable " + f"type, not skipped); got min=None ({stats!r})" + ) + assert stats.max is not None, ( + f"scalar column {label!r}: MAX should be populated (orderable " + f"type, not skipped); got max=None ({stats!r})" + ) + assert stats.data_type, ( + f"scalar column {label!r}: data_type should be a non-empty " + f"warehouse type string; got {stats.data_type!r} ({stats!r})" + ) + + # --- Complex columns: MIN/MAX skipped (DEC-003), no raise. ------------ + # THIS is the load-bearing #227-style skip-set validation: if any of + # ARRAY / OBJECT / VARIANT were NOT in ``_COMPLEX_SNOWFLAKE_TYPES``, the + # aggregate would have emitted MIN/MAX for it and (per the plan's DEC-003 + # analysis) Snowflake would raise, failing the whole batch — so reaching + # these assertions at all proves the skip-set covers these types, and + # the ``min is None`` / ``max is None`` checks confirm they were skipped. + for label, stats in ( + ("tags", tags_stats), + ("meta", meta_stats), + ("payload", payload_stats), + ): + assert stats.min is None, ( + f"complex column {label!r}: MIN must be skipped (DEC-003 skip-set) " + f"so min should be None; got {stats.min!r} ({stats!r})" + ) + assert stats.max is None, ( + f"complex column {label!r}: MAX must be skipped (DEC-003 skip-set) " + f"so max should be None; got {stats.max!r} ({stats!r})" + ) + assert stats.count == 1, ( + f"complex column {label!r}: row 1 populated, row 2 NULL, so the " + f"non-null count should be 1; got {stats.count} ({stats!r})" + ) + assert stats.nulls == 1, ( + f"complex column {label!r}: exactly one NULL row, so nulls " + f"should be 1; got {stats.nulls} ({stats!r})" + ) + assert stats.data_type, ( + f"complex column {label!r}: data_type should be a non-empty " + f"warehouse type string (e.g. ARRAY/OBJECT/VARIANT); got " + f"{stats.data_type!r} ({stats!r})" + ) + + # --- Out-of-union return types: BINARY / TIME (#258 QG). -------------- + # BINARY and TIME are SQL-orderable, so they are NOT in the skip-set and + # MIN/MAX IS emitted for them — but the connector returns ``bytearray`` / + # ``datetime.time``, which are outside ``ColumnStats.min``/``max``'s union. + # ``_coerce_min_max`` nulls them; without that net the ``ColumnStats(...)`` + # construction would raise a Pydantic ValidationError and fail the whole + # batch (a non-WarehouseError panic-tier crash). Both rows are populated, + # so count/distinct are 2 and nulls is 0. + for label, stats in (("blob", blob_stats), ("t", time_stats)): + assert stats.min is None, ( + f"orderable-but-out-of-union column {label!r}: min must be nulled " + f"by _coerce_min_max; got {stats.min!r} ({stats!r})" + ) + assert stats.max is None, ( + f"orderable-but-out-of-union column {label!r}: max must be nulled " + f"by _coerce_min_max; got {stats.max!r} ({stats!r})" + ) + assert stats.count == 2, ( + f"column {label!r}: both rows populated, so count should be 2; " + f"got {stats.count} ({stats!r})" + ) + assert stats.nulls == 0, ( + f"column {label!r}: no NULLs, so nulls should be 0; got {stats.nulls} ({stats!r})" + ) + assert stats.data_type, ( + f"column {label!r}: data_type should be a non-empty warehouse " + f"type string (BINARY/TIME); got {stats.data_type!r} ({stats!r})" + ) + finally: + # --- Teardown: drop the engineered table (idempotent). ---------------- + # A fresh adapter — the stats adapter's session has been closed by its + # own ``__exit__``. ``IF EXISTS`` tolerates a partial setup where the + # table was never created. + teardown_adapter = _make_adapter() + with teardown_adapter: + teardown_cursor = teardown_adapter._get_connection().cursor() + try: + teardown_cursor.execute(f"DROP TABLE IF EXISTS {quoted}") + finally: + teardown_cursor.close() diff --git a/tests/warehouse/test_snowflake_stub.py b/tests/warehouse/test_snowflake_stub.py index f6009235..7e0c217a 100644 --- a/tests/warehouse/test_snowflake_stub.py +++ b/tests/warehouse/test_snowflake_stub.py @@ -13,9 +13,10 @@ 4. SDK type-ignores are confined to ``_snowflake_client.py`` — pinned by ``tests/warehouse/test_snowflake_client_confinement.py``, not here. -``column_stats`` still raises :class:`NotImplementedError` naming the epic -(#118) — ``sample_rows`` (#122 US-003), ``materialise_sample`` / ``run_test_sql`` -(#122 US-004) are now implemented and exercised in the sampling / materialise +``column_stats`` is implemented (#258, BigQuery-style batching + catalog +pre-filter) and exercised in ``tests/warehouse/test_snowflake_adapter.py``; +``sample_rows`` (#122 US-003), ``materialise_sample`` / ``run_test_sql`` +(#122 US-004) are implemented and exercised in the sampling / materialise suites. ``estimate_query_bytes`` is overridden by a real EXPLAIN-based implementation (#130 US-003): it runs ``EXPLAIN USING JSON `` and parses ``GlobalStats.bytesAssigned``, returning a real ``int`` on the happy path and @@ -33,7 +34,7 @@ from signalforge.warehouse import EstimateUnavailableError, SnowflakeAdapter from signalforge.warehouse.base import WarehouseAdapter -from signalforge.warehouse.models import SNOWFLAKE_DIALECT, Dialect, TableRef +from signalforge.warehouse.models import SNOWFLAKE_DIALECT, Dialect from signalforge.warehouse.profiles import DbtProfileTarget from tests.warehouse._fake_snowflake import FakeSnowflakeConnection @@ -127,22 +128,6 @@ def test_init_stores_key_pair_and_sso_auth_fields() -> None: assert adapter._authenticator == "externalbrowser" -# --------------------------------------------------------------------------- -# Stub methods raise NotImplementedError naming the epic (#118) -# --------------------------------------------------------------------------- - - -def test_column_stats_raises_not_implemented() -> None: - """:meth:`column_stats` is part of the v0.2 skeleton surface.""" - adapter = SnowflakeAdapter() - table = TableRef(project=None, dataset="public", name="t") - - with pytest.raises(NotImplementedError) as exc_info: - adapter.column_stats(table, "id") - - assert "issue #118" in str(exc_info.value) - - # --------------------------------------------------------------------------- # estimate_query_bytes — real EXPLAIN-based implementation (#130 US-003) # ---------------------------------------------------------------------------