#139: Snowflake projection-subquery sample shape - #142
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
wjduenow
marked this pull request as ready for review
May 27, 2026 16:55
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…der_sample_select helper Add sample_hash_in_projection / sample_hash_alias to the Dialect frozen dataclass (BigQuery/Postgres keep inline defaults; SNOWFLAKE_DIALECT sets projection=True). New stateless warehouse-layer helper render_sample_select switches inline-vs-projection on the boolean flag only (never dialect.name): inline reproduces the prune compiler's current sample-CTE body byte-for-byte; projection-subquery computes HASH(*) in an inner projection and references the alias in WHERE/ORDER BY with SELECT * EXCLUDE. Traces to DEC-001/002/003/004. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r_sample_select + regen Snowflake snapshots Replace _render_sample_cte's inline hash-mod string-building with a call to the shared signalforge.warehouse._sample_sql.render_sample_select helper (order_by_hash=False; the compiler CTE has no ORDER BY). Partition predicate stays rendered by the compiler's own _render_partition_filter and is passed as extra_where. Switches on the boolean Dialect.sample_hash_in_projection — no dialect.name branch, no warehouse-SDK import (import-guard green). BigQuery (inline) output is byte-identical — top-level compiled_sql/*.sql fixtures unchanged (the regression gate). The five Snowflake *_sample.sql fixtures regenerated to the projection-subquery form (SELECT * EXCLUDE (_sf_sample_hash) FROM (SELECT t.*, ABS(HASH(*)) AS _sf_sample_hash ...) WHERE MOD(_sf_sample_hash, n) < 1) so HASH(*) is computed in the projection, never a predicate. sqlglot snowflake-dialect parse guard passes on the new form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…aterialise_sample use projection-subquery shape Replace the inline MOD(ABS(HASH(*)), n) < 1 + ORDER BY ABS(HASH(*)) building in SnowflakeAdapter.sample_rows and materialise_sample with the shared render_sample_select(..., order_by_hash=True) helper (US-001). Snowflake's HASH(*) is invalid in a WHERE/ORDER BY predicate (002079); the helper's projection-subquery branch computes the hash once in an inner SELECT t.*, ABS(HASH(*)) AS _sf_sample_hash and the outer clauses reference the alias, with SELECT * EXCLUDE (_sf_sample_hash) stripping the helper column so returned rows / the materialised temp table carry only source columns. Partition filters stay rendered by the adapter's own _render_partition_filter and pass to the helper as extra_where (no name branch, no hard-coded HASH(*)). Update the fakesnow/sqlglot adapter guards plus the test_snowflake_sampling and test_snowflake_materialise unit assertions to the new shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… e2e at scope=sample + 5-surface docs Flip test_snowflake_prune_live.py to scope=sample + sample_strategy=materialised (exercising the #139 projection-subquery CTAS); rename to test_prune_drops_always_passes_not_null_live_materialised_sample. Two-gate discipline (marker + runtime _skip_reason) preserved; self-skips cleanly offline. DEC-004 primary/fallback note added to the module docstring. 5-surface graduation: warehouse-adapter-ops.md (remove HASH(*)-in-predicate limitation; materialised sample-mode now works), .claude/rules/warehouse-adapters.md (new Dialect fields + mark bd_1-scaffolding-cdp FIXED), .claude/rules/prune-engine.md (compiler dialect field list + render_sample_select delegation). Live certification still pending (maintainer-run with SF_RUN_SNOWFLAKE=1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- harden the name-agnostic helper test with a symmetric inline-direction assertion (pins both branches against a dialect.name-based dispatch) - correct stale docstring/comment in test_e2e_snowflake_smoke.py: the HASH(*)-in-WHERE/ORDER-BY shape bug is FIXED by #139 for both sample strategies; the remaining scope=full requirement here is the read-only SNOWFLAKE_SAMPLE_DATA share (materialised) + oneshot's open row-count seam (bd_1-scaffolding-tft), not the shape bug Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ure dialect-shape lesson - prune-engine.md § "Adding a new vendor dialect": capture the generalised #139 lesson (a single inline SQL-fragment string can't express a clause- POSITION constraint; when the SQL *shape* differs, add a structural Dialect field + shared renderer; sqlglot parses but cannot certify warehouse acceptance, so the live-gated test is the real merge gate) - plans/super/139: add Outcome note (6 stories landed; QG 4 passes fixed E501 + stale e2e docstring; offline green 2394 passed / pyright 0 / -m snowflake 33 passed,4 live-skipped; DEC-006 live cert PENDING/maintainer) - verified US-004's rule/doc graduation already complete + coherent (no re-edit needed there) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… form confirmed) test_prune_drops_always_passes_not_null_live_materialised_sample passed against a real Snowflake warehouse (1 passed in 14.78s) — the materialised scope=sample path executes the SELECT * EXCLUDE projection-subquery CTAS and drops the always-passes test. Snowflake accepts ORDER BY of an EXCLUDE-d column, so the primary form stands (no DEC-004 fallback needed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements the Snowflake deterministic sampling fix for issue #139 by moving HASH(*) into a projection subquery and sharing that sample SQL shape between prune compilation and Snowflake adapter sampling paths.
Changes:
- Adds dialect-level sample shape fields and a shared
render_sample_selecthelper. - Wires Snowflake
sample_rows,materialise_sample, and prune sample CTE generation to the new projection-subquery form. - Updates Snowflake SQL fixtures, tests, live-test docs, and operational guidance for the fixed materialised sample path.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/signalforge/warehouse/_sample_sql.py |
Adds shared deterministic sample SELECT renderer. |
src/signalforge/warehouse/models.py |
Adds dialect fields for projection-based sample hashing. |
src/signalforge/warehouse/adapters/snowflake.py |
Uses shared renderer for Snowflake sample queries and CTAS. |
src/signalforge/prune/compiler.py |
Uses shared renderer for sample CTE bodies. |
tests/warehouse/test_sample_sql.py |
Adds unit tests for sample SQL rendering shapes. |
tests/warehouse/test_models.py |
Pins new dialect field defaults and Snowflake values. |
tests/warehouse/test_snowflake_sampling.py |
Updates Snowflake sample SQL expectations. |
tests/warehouse/test_snowflake_materialise.py |
Updates materialised CTAS SQL expectations. |
tests/warehouse/test_snowflake_adapter_fakesnow.py |
Updates fakesnow/sqlglot parse guards. |
tests/warehouse/test_snowflake_prune_live.py |
Switches live prune e2e to materialised sample mode. |
tests/prune/test_compiler.py |
Updates Snowflake compiler guards for projection-subquery fixtures. |
tests/fixtures/prune/compiled_sql/snowflake/accepted_values_sample.sql |
Regenerates Snowflake sample fixture. |
tests/fixtures/prune/compiled_sql/snowflake/custom_sql_sample.sql |
Regenerates Snowflake sample fixture. |
tests/fixtures/prune/compiled_sql/snowflake/not_null_sample.sql |
Regenerates Snowflake sample fixture. |
tests/fixtures/prune/compiled_sql/snowflake/relationships_sample.sql |
Regenerates Snowflake sample fixture. |
tests/fixtures/prune/compiled_sql/snowflake/unique_sample.sql |
Regenerates Snowflake sample fixture. |
tests/cli/test_e2e_snowflake_smoke.py |
Updates Snowflake smoke-test rationale. |
docs/warehouse-adapter-ops.md |
Updates Snowflake sampling operations guidance. |
.claude/rules/warehouse-adapters.md |
Records adapter convention changes. |
.claude/rules/prune-engine.md |
Records compiler/dialect convention changes. |
plans/super/139-snowflake-sample-shape.md |
Adds implementation plan and outcome record. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/signalforge/warehouse/_sample_sql.py (1)
71-89: ⚡ Quick winFail fast on invalid
sample_bucket/sample_sizeinputs.
sample_bucket <= 0can produce invalidMOD(..., 0)SQL, and non-positivesample_sizeyields invalid/uselessLIMITvalues. Guard early with a clear exception.Proposed patch
def render_sample_select( @@ ) -> str: @@ - expr = dialect.sample_row_hash_expr + if sample_bucket <= 0: + raise ValueError("sample_bucket must be > 0") + if sample_size <= 0: + raise ValueError("sample_size must be > 0") + + expr = dialect.sample_row_hash_expr🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/signalforge/warehouse/_sample_sql.py` around lines 71 - 89, Add early validation for the sampling inputs: before computing expr = dialect.sample_row_hash_expr (i.e., at the start of the function that builds this SQL), check that sample_bucket > 0 and sample_size > 0 and raise a ValueError with a clear message if not; this prevents generating invalid SQL like MOD(..., 0) or nonsensical LIMIT values when sample_bucket or sample_size are non-positive and keeps the rest of the logic (the branches using dialect.sample_hash_in_projection, alias, where_sql, order_sql, and the final RETURNs) unchanged.src/signalforge/warehouse/adapters/snowflake.py (1)
76-76: ⚡ Quick winUse the warehouse public import surface for the new helper.
Line 76 imports from a private module path. Please import
render_sample_selectvia thesignalforge.warehousepublic API and re-export it there if needed.Suggested change
-from signalforge.warehouse._sample_sql import render_sample_select +from signalforge.warehouse import render_sample_selectAs per coding guidelines: "Package imports: import from the public API surface (re-exported names from subpackage
__init__.pyfiles) rather than private submodule paths."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/signalforge/warehouse/adapters/snowflake.py` at line 76, The import in signalforge.warehouse.adapters.snowflake.py uses a private module path for render_sample_select; change it to import render_sample_select from the package public API (from signalforge.warehouse import render_sample_select) and if render_sample_select is not already re-exported, add it to signalforge.warehouse.__init__.py's exports so the symbol is available from the public surface; update any references in snowflake.py to use the imported render_sample_select.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/warehouse-adapter-ops.md`:
- Around line 614-616: The paragraph contains a false heading token "`#139`"
that triggers markdownlint MD018; change the literal "`#139 projection-subquery
shape`" to plain prose such as "issue `#139` projection-subquery shape" (or "issue
`#139`" + the rest) so the text no longer begins with an ATX heading marker;
update the occurrence near the `_sf_sample_hash` example in the
materialised-sample CTAS description.
In `@plans/super/139-snowflake-sample-shape.md`:
- Around line 121-127: Markdown fenced code blocks for the two SQL examples are
missing language identifiers (triggering markdownlint MD040); update the opening
backtick fences for the block starting with "SELECT * FROM <table_sql> AS t
WHERE MOD(<hash_expr>..." and the block starting with "SELECT * EXCLUDE
(<alias>) FROM (SELECT t.*, <hash_expr> AS <alias>..." to include "sql" (i.e.,
change ``` to ```sql) so both blocks are properly tagged as SQL.
In `@src/signalforge/prune/compiler.py`:
- Line 74: The import is reaching into a private module; change the import of
render_sample_select to come from the package public API (e.g., import
render_sample_select from signalforge.warehouse) and, if that symbol is not
currently re-exported, add render_sample_select to the warehouse package
__init__.py exports so the name is available from the public surface; update the
import in src/signalforge/prune/compiler.py to use that public import path
referencing render_sample_select.
In `@src/signalforge/warehouse/_sample_sql.py`:
- Line 41: The import currently uses a private submodule path; change the import
to use the package public API by importing Dialect from the warehouse package
(i.e., replace the existing "from signalforge.warehouse.models import Dialect"
with an import from "signalforge.warehouse" so the symbol Dialect is imported
from the package's public surface), ensuring other references to Dialect in this
module remain unchanged.
In `@tests/warehouse/test_sample_sql.py`:
- Around line 19-24: The test imports are using private module paths; change
them to import the public re-exports from the subpackage root by replacing
imports of render_sample_select, BIGQUERY_DIALECT, SNOWFLAKE_DIALECT and Dialect
from signalforge.warehouse._sample_sql and signalforge.warehouse.models to
import those same symbols directly from signalforge.warehouse so the test uses
the package public API surface (reference symbols: render_sample_select,
BIGQUERY_DIALECT, SNOWFLAKE_DIALECT, Dialect).
---
Nitpick comments:
In `@src/signalforge/warehouse/_sample_sql.py`:
- Around line 71-89: Add early validation for the sampling inputs: before
computing expr = dialect.sample_row_hash_expr (i.e., at the start of the
function that builds this SQL), check that sample_bucket > 0 and sample_size > 0
and raise a ValueError with a clear message if not; this prevents generating
invalid SQL like MOD(..., 0) or nonsensical LIMIT values when sample_bucket or
sample_size are non-positive and keeps the rest of the logic (the branches using
dialect.sample_hash_in_projection, alias, where_sql, order_sql, and the final
RETURNs) unchanged.
In `@src/signalforge/warehouse/adapters/snowflake.py`:
- Line 76: The import in signalforge.warehouse.adapters.snowflake.py uses a
private module path for render_sample_select; change it to import
render_sample_select from the package public API (from signalforge.warehouse
import render_sample_select) and if render_sample_select is not already
re-exported, add it to signalforge.warehouse.__init__.py's exports so the symbol
is available from the public surface; update any references in snowflake.py to
use the imported render_sample_select.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0799e9c7-9667-4d24-a7ab-7f0288c4ba5d
📒 Files selected for processing (21)
.claude/rules/prune-engine.md.claude/rules/warehouse-adapters.mddocs/warehouse-adapter-ops.mdplans/super/139-snowflake-sample-shape.mdsrc/signalforge/prune/compiler.pysrc/signalforge/warehouse/_sample_sql.pysrc/signalforge/warehouse/adapters/snowflake.pysrc/signalforge/warehouse/models.pytests/cli/test_e2e_snowflake_smoke.pytests/fixtures/prune/compiled_sql/snowflake/accepted_values_sample.sqltests/fixtures/prune/compiled_sql/snowflake/custom_sql_sample.sqltests/fixtures/prune/compiled_sql/snowflake/not_null_sample.sqltests/fixtures/prune/compiled_sql/snowflake/relationships_sample.sqltests/fixtures/prune/compiled_sql/snowflake/unique_sample.sqltests/prune/test_compiler.pytests/warehouse/test_models.pytests/warehouse/test_sample_sql.pytests/warehouse/test_snowflake_adapter_fakesnow.pytests/warehouse/test_snowflake_materialise.pytests/warehouse/test_snowflake_prune_live.pytests/warehouse/test_snowflake_sampling.py
- test_snowflake_prune_live.py: DEC-004 note now records the live cert RESOLVED the ORDER-BY-of-EXCLUDE-d-column question in favour of the primary form (Copilot: stale "unresolved decision point" docstring) - docs/warehouse-adapter-ops.md: reword so the line no longer starts with "#139" (CodeRabbit/markdownlint MD heading false-trigger) - plans/super/139: add `sql` language tags to two fenced blocks (CodeRabbit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review SummaryLive Snowflake certification passed ( Fixed (3 items)
False Positives (3 items)
All 6 review threads resolved. |
wjduenow
changed the title
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #139 — Snowflake sample-mode prune emitted
MOD(ABS(HASH(*)), n)/ORDER BY ABS(HASH(*)), which Snowflake rejects (002079:HASH(*)is valid only in the SELECT projection). Until now Snowflake prune worked only withprune.scope: full.Changes
Dialectgains two structural fields —sample_hash_in_projection: bool(BigQueryFalse, SnowflakeTrue) andsample_hash_alias: str— so the sample-SQL shape is dialect-driven, never name-branched.warehouse/_sample_sql.render_sample_select(new shared helper) renders the inline form (BigQuery, byte-identical to before) or the projection-subquery form for Snowflake:SELECT * EXCLUDE (_sf_sample_hash) FROM (SELECT t.*, ABS(HASH(*)) AS _sf_sample_hash FROM <src> AS t) WHERE MOD(_sf_sample_hash, b) < 1 [AND <pf>] [ORDER BY _sf_sample_hash] LIMIT nSnowflakeAdapter.sample_rows/materialise_sample.SELECT * EXCLUDEkeeps the hash column out of returned rows and the materialised temp table.*_sample.sqlsnapshots were regenerated.Testing
2394 passed,pyright0 errors,ruff/ruff formatclean; offline-m snowflake33 passed / 4 live-skipped(snapshots + sqlglot parse guards on the newEXCLUDEform).test_snowflake_prune_live.py::test_prune_drops_always_passes_not_null_live_materialised_sampleran green against a real warehouse (scope=sample+materialised), dropping the always-passes test. This certifies DEC-006 and resolves DEC-004 in favour of the primary form (Snowflake acceptsORDER BYof aSELECT * EXCLUDE-d column — no fallback needed).Compounding Update
.claude/rules/warehouse-adapters.md+prune-engine.md: documented the two newDialectfields + the projection-subquery shape; marked the live-harness findingbd_1-scaffolding-cdpFIXED; captured the generalised lesson (a single inline SQL-fragment string can't express a dialect's clause-POSITION constraint — add a structuralDialectfield + shared renderer; only a live-gated test certifies vendor acceptance).docs/warehouse-adapter-ops.md: "Known limitations" updated — HASH(*) shape bug fixed, materialised sample-mode works;oneshotrow-count seam (bd_1-scaffolding-tft) noted as a separate open bug.Plan:
plans/super/139-snowflake-sample-shape.md. Beads epicbd_1-scaffolding-kay(closed).🤖 Generated with Claude Code