fix(#171): row_count_anomaly_by_period emits valid BigQuery SQL - #248
Conversation
The gated live BigQuery e2e (never run since #171 merged) revealed the anomaly compiler had never produced executable BigQuery SQL — anomaly detection silently degraded to kept-without-evidence on every BQ run. Two root causes: 1. mad/percentile methods emitted PERCENTILE_CONT(p) WITHIN GROUP — a standard-SQL ordered-set aggregate BigQuery does not support (its PERCENTILE_CONT is window-only). BigQuery now uses the GROUP-BY APPROX_QUANTILES(expr,100)[OFFSET(round(p*100))] idiom; Snowflake/ Postgres keep WITHIN GROUP (default flipped, so POSTGRES_DIALECT now declares its percentile template explicitly). 2. The DEC-012 partition-pruning predicate compared a TIMESTAMP date column against a DATE('...') bound (rejected: TIMESTAMP vs DATE). The bound literal is now type-matched to the column's data_type (new Dialect.datetime_literal_template + _date_value_literal, threaded from model.columns into the engine-side and the operator- shipped singular-test compilers). CASTing the column would disable partition pruning, so the literal is matched instead. Also fixes read_prune_decisions dropping as_of/stats on read-back. Certified end-to-end against real BigQuery (live anomaly e2e green). Regenerated BQ mad/percentile snapshots; added TIMESTAMP-column regression tests for both the engine and emitter paths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFixes two BigQuery bugs in ChangesBigQuery anomaly SQL generation fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 |
There was a problem hiding this comment.
Pull request overview
Fixes row_count_anomaly_by_period so it generates executable BigQuery SQL (restoring anomaly detection from silent kept-without-evidence), by moving BigQuery percentile rendering to APPROX_QUANTILES and type-matching DEC-012 partition-pruning bound literals to the date column’s warehouse data_type across both the engine compiler and emitted dbt singular tests.
Changes:
- Switch BigQuery’s GROUP-BY percentile implementation to
APPROX_QUANTILES(...)[OFFSET(...)]while keeping Snowflake/Postgres on ordered-setPERCENTILE_CONT ... WITHIN GROUP. - Type-match anomaly partition bound literals (
DATE/DATETIME/TIMESTAMP) using manifest columndata_type, threaded through engine compilation and the diff emitter. - Update fixtures/tests and fix
read_prune_decisionshelper to preserveas_of/stats.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/warehouse/test_models.py | Updates dialect field expectations (new datetime_literal_template, BigQuery percentile default). |
| tests/prune/test_compiler.py | Adds regression tests for type-matched bound literals; updates percentile SQL expectations for BigQuery. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_none_stats.sql | Snapshot update: BigQuery percentile stats use APPROX_QUANTILES. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_dow_stats.sql | Snapshot update: BigQuery DOW percentile stats use APPROX_QUANTILES. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_none_stats.sql | Snapshot update: BigQuery MAD stats use APPROX_QUANTILES median/MAD. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_dow_stats.sql | Snapshot update: BigQuery seasonal MAD stats use APPROX_QUANTILES. |
| tests/fixtures/diff/proposed_test_files/anomaly/orders__row_count_anomaly_by_period_8e4d6245.sql | Snapshot update: emitted singular test SQL uses BigQuery-legal percentile form. |
| tests/cli/_e2e_helpers.py | Preserves as_of and stats when reading prune decisions back from audit events. |
| src/signalforge/warehouse/models.py | Extends Dialect with datetime_literal_template; flips BigQuery percentile default; makes Postgres declare ordered-set percentile explicitly. |
| src/signalforge/prune/compiler.py | Adds _date_value_literal and threads date_column_type; updates percentile rendering to include offset. |
| src/signalforge/diff/_emitter.py | Threads model column data_type into singular-test SQL compilation for type-matched bounds. |
| CHANGELOG.md | Documents the BigQuery fix and the read-back helper fix. |
| .claude/rules/warehouse-adapters.md | Documents the dialect fragment corrections and new datetime literal fragment. |
| .claude/rules/business-rule-tests.md | Records the durable “live cert vs snapshot” lesson and the concrete fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ounding Addresses Copilot review on PR #248: Python's built-in round() is banker's rounding (ties-to-even), so a half-integer percentile bucket (p=0.125 -> 12.5) resolved to OFFSET(12) instead of the conventional OFFSET(13). Switch to int(p*100 + 0.5) (round-half-up, away from zero). All realistic inputs (p=0.5 median; integer thresholds) land on exact integers, so no snapshot moves. Adds a regression test for the half-integer case.
|
Addressed the Copilot review note on |
Summary
While pre-release-testing the Airflow epic (#228), the gated live BigQuery e2e (which had not been run since #171 merged) revealed that
row_count_anomaly_by_periodnever produced executable BigQuery SQL — anomaly detection silently degraded tokept-without-evidenceon every BigQuery run. Snapshot fixtures pinned the invalid SQL byte-for-byte, so the offline suite was green.Root causes
PERCENTILE_CONT(p) WITHIN GROUP (ORDER BY …)(themad/percentilemethods) is a standard-SQL ordered-set aggregate BigQuery does not support — itsPERCENTILE_CONTis window-only and cannot reduce rows in aGROUP BY. BigQuery now uses the GROUP-BY-compatibleAPPROX_QUANTILES(expr, 100)[OFFSET(round(p*100))]idiom. Snowflake / Postgres keep theWITHIN GROUPform — theDialectdefault flipped, soPOSTGRES_DIALECTnow declares its percentile template explicitly (a BigQuery-only idiom can't be a shared default).DATE('…')bound →No matching signature for >= : TIMESTAMP, DATE. The bound literal is now type-matched to the date column's warehousedata_type(newDialect.datetime_literal_template+_date_value_literal, threading the column type frommodel.columns[date_column]into both the engine-side compiler and the operator-shipped singular-test compiler). CASTing the column would have disabled partition pruning, so the literal is matched, not the column.Also fixes the
read_prune_decisionstest helper droppingas_of/statson read-back.Certification
tests/cli/test_e2e_row_count_anomaly.py, real Anthropic + BigQuery) — valid SQL, engineered volume drop detected →kept,as_of/statspopulated. Each compiled stats + violation query independently validated viabq query --dry_run.mad/percentilesnapshots regenerated;zscore/min_maxunchanged.Durable lesson
A
Dialectdate/percentile SQL fragment is not validated by snapshot equality — only a live (orsqlglot/executor) run catches invalid-but-byte-stable SQL. Captured in.claude/rules/business-rule-tests.md§ "#171 follow-up correction" andwarehouse-adapters.md.Closes the row_count_anomaly bug found pre-release; the Airflow feature (#228) itself is unaffected and green.
Summary by CodeRabbit
Release Notes
Bug Fixes
row_count_anomaly_by_periodto use executable percentile syntax.as_ofandstatswhen reading back results.Tests
Documentation
Chores
dag_id.