test(data): static gate — no dbt model may dedup on a nullable key (#1330) - #1348
test(data): static gate — no dbt model may dedup on a nullable key (#1330)#1348SharedQA wants to merge 6 commits into
Conversation
…onstructorfabric#1330) A ReplacingMergeTree that dedups on a NULLABLE order_by key never collapses its duplicates (NULL != NULL), so rows accumulate and the metric drifts as merges run — the 'same period, different number' class. allow_nullable_key:1 is set in ~99 models (needed just to create such a table), so ClickHouse won't stop you. scripts/ci/nullable_key_audit.py statically flags any model whose dedup-key projection is Nullable(...) / CAST(NULL ...), using a paren-depth-aware scan so MD5(concat(a,b,c)) inner commas and sibling CAST(NULL) columns don't taint the key. Wired blocking in data-integrity.yml (no warehouse needed). Parser self-test included. Current repo: 80 ordered/RMT models scanned, 0 nullable dedup keys — green; the keys are non-null by construction (MD5 / CAST AS String / coalesce) today, this locks that invariant going forward. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
|
Warning Review limit reached
More reviews will be available in 59 minutes and 47 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a static analysis CI guard for dbt models: a new Python script scans ChangesNullable Key Audit Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/data-integrity.yml:
- Around line 8-11: The `paths` filter in the data-integrity.yml workflow
trigger (lines 8-11) is incomplete and excludes important files that should
cause this workflow to run. Add `scripts/ci/test_nullable_key_audit.py` and
`.github/workflows/data-integrity.yml` to the paths list so that changes to the
guard validation test logic or the workflow configuration itself will properly
trigger this data integrity check.
- Around line 17-20: The workflow uses tag-based pinning (v4 and v5) for both
the checkout and setup-python actions instead of commit SHA pinning, which
weakens supply-chain security. Additionally, the checkout action persists
credentials by default, creating a token hygiene risk. Replace the tag-pinned
versions of actions/checkout@v4 and actions/setup-python@v5 with their full
commit SHA equivalents, and add persist-credentials: false as an option to the
checkout action to disable credential persistence and improve CI supply-chain
hardening.
In `@scripts/ci/nullable_key_audit.py`:
- Around line 81-87: When projections_for(text, col) returns an empty result (no
projections found), the current code silently passes without flagging an error,
creating a false-negative. Add a check to detect when projections_for(text, col)
yields no results and append an error entry to out with an appropriate message
indicating that the dedup key projection could not be resolved. This ensures the
script fails closed by flagging ambiguous or unresolvable dedup key projections
rather than silently ignoring them, consistent with the err-toward-flagging
behavior.
- Line 49: The regex pattern in nullable_key_audit.py at line 49 uses word
boundaries (\b) which cannot match quoted column aliases like AS "unique_key" or
AS `unique_key`. To fix this, update the regex pattern to handle all three forms
of identifiers: unquoted, double-quoted, and backtick-quoted. Replace the
current pattern with one that uses alternation to match any of these identifier
forms and uses appropriate terminators (space, end of string, or comma) instead
of word boundaries, as word boundaries don't work correctly after closing
quotes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d442bc01-4c0d-44d6-84c8-97ec3d813212
📒 Files selected for processing (3)
.github/workflows/data-integrity.ymlscripts/ci/nullable_key_audit.pyscripts/ci/test_nullable_key_audit.py
- workflow paths: also trigger on the guard's own test + the workflow file. - harden actions: SHA-pin checkout (34e1148, v4) + setup-python (a26af69, v5), persist-credentials: false (zizmor blanket-policy + token hygiene). - fail closed on a genuinely unresolvable dedup key, while treating SELECT*/union_by_tag/bare+qualified passthrough/macro-generated (snapshot) keys as inherited-from-upstream (a blanket fail-closed false-positives 49 legitimate models — verified). Added self-tests for each path. - skipped quoted-alias regex: CodeRabbit's own scan found zero quoted aliases in src/ingestion; revisit if the convention changes. gate: 80 models, 0 nullable keys (PASSED); parser self-test 8 cases. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
Review feedback on constructorfabric#1348 (cyberantonz): - Remove the `--check` flag. A CI guard has no use for a report-only mode, so the script now always exits 1 on a violation (main returns an int; the workflow invokes it with no flag). - Run ruff + mypy on the guard, as the PR template promises. Applied `ruff format`, fixed lint, and made it mypy-clean; added a "ruff check + ruff format --check + mypy" step to data-integrity.yml so it stays enforced. - Clarify the `parents[2]` root resolution with a comment (scripts/ci/<file> → repo root). Gate unchanged: 80 models, 0 nullable keys; 8 self-tests pass. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
|
Two separate things are bundled in the question — the static guard in this PR, and the data-quality tests that genuinely need a warehouse. They need different answers. Why this one is a script, not a data test
And it is tested: What the data-quality checks need (the ones you mean — non-negative, spike, caps, bounds, freshness)These do need a harness — a real ClickHouse, dbt, and seeded data, exactly as you said. Today they run only post-deploy, on the Argo
This maps to the warehouse-in-CI items already on the QA plan (ClickHouse + dbt in CI; seeded test tenant + fixtures). Until it lands, the catalog's coverage is the nightly cron, and the static guards like this one are the part we can already gate on a PR with no env. Short version: this PR's guard is already fully testable and tested with no harness — that's why it's a script. The harness work is for the data tests, and the fastest route is extending the e2e rig with violating fixtures, not standing up something new. |
…ts (constructorfabric#1348) Review of the data-quality PoC: - Relative dates: `_row()` now uses `today() - days_ago` instead of a hardcoded 2026 date. The non-negative check has no date filter, but a sibling check (no_spike) filters `date >= today() - {120,3}`; a hardcoded date would be filtered out and a seeded violation would falsely pass. A date factory must survive time-bound dbt logic. - Crash-safe results: `DbtRunner.run_test` now unlinks run_results.json BEFORE running and treats an absent file afterward as a dbt failure (compile/conn error), surfacing stdout/stderr — instead of silently reading a stale file from the previous run. - Documented two review points in the README: the bronze→silver blindspot is the priority follow-up (and why it's a real task — the build pulls m365__bronze_ promoted + both staging models, so the m365 bronze placeholders must be completed first), and the serial/TRUNCATE limitation is the rig-wide model today (worker_id already plumbed for the eventual per-worker-schema fix), not a flaw unique to this test. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
Adds an e2e test that seeds bronze M365 sharepoint/onedrive rows, runs `dbt build --select +class_collab_document_activity`, and asserts the silver output — covering the bronze→silver transform that the data-quality silver-seed PoC (constructorfabric#1348) bypasses. Key assertion: a negative viewedOrEditedFileCount is preserved through the transform, not silently dropped. Required fixes surfaced by actually executing the build: - create-bronze-placeholders.sh: add the m365 document-activity source columns (tenant_id/source_id are connector-injected physical fields; plus reportRefreshDate, reportPeriod, viewed/synced/visited counts). visitedPageCount typed Int64 to match the OneDrive staging cast (else silver UNION NO_COMMON_TYPE). - dbt_runner.py: use cfg.ch_host instead of hardcoded 127.0.0.1 so `dbt build` is reachable from the dockerized runner (no existing fixture exercised it). - pytest.ini: collect the new transform/ dir. Verified green twice from a clean stack. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kenan Salim <kenan.salim@gmail.com>
The "Self-test the guard's parser" step ran `python -m pytest ...` but nothing installed pytest (the lint step only installs ruff+mypy), so the job failed with "No module named pytest". Install pytest in the step, mirroring how the lint step installs its own tools inline. Verified locally: `pytest scripts/ci/test_nullable_key_audit.py -q` → 8 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kenan Salim <kenan.salim@gmail.com>
…lity.yml data-checks / data-integrity / data-contracts all triggered on the same src/ingestion/** PRs — three workflows, three checkouts, three Python setups. Fold them into one data-quality.yml with parallel jobs: - connector-silver-coverage (blocking) - nullable-key-guard (blocking; absorbs constructorfabric#1348's nullable_key_audit + self-test) - dbt-coverage (report-only; absorbs constructorfabric#1320's dbt_coverage) Scripts moved into this PR; constructorfabric#1348 is closed as absorbed and constructorfabric#1320 slims to its m365 type-mismatch fix. SHA-pinned actions, persist-credentials:false throughout. Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com>
|
Closing as absorbed — the nullable-key dedup guard ( |
3 participants
What
scripts/ci/nullable_key_audit.py— a static (warehouse-free) gate that fails CI if any dbt model's ReplacingMergeTree dedup key (order_by) is projected asNullable(...)/CAST(NULL …).Why
A NULL in the sorting key never compares equal to itself, so RMT can't collapse those rows — they accumulate as silent duplicates and the metric drifts as background merges run (the "same period, different number" class, #1330).
allow_nullable_key: 1is set in ~99 models (it's needed just to create a table whose ORDER BY has a nullable column), so ClickHouse permits it — this gate is the backstop.How
Paren-depth-aware backward scan isolates the key's own projection, so
MD5(concat(a, b, c))inner commas and a sibling column'sCAST(NULL …)don't cause false positives (both were real cases — see the parser self-test). Blocking from day one indata-integrity.yml; no service container needed.Result today
80 ordered/RMT models scanned, 0 nullable dedup keys — green. Keys are non-null by construction (MD5 / CAST AS String / coalesce); this locks that invariant so a future model can't quietly reintroduce the duplicate-metric bug. Parser self-test: 4 cases incl. the MD5-inner-comma and sibling-CAST(NULL) false-positive guards.
This is the write/schema complement to the
not_nullread tests and the enforced-contract work on #1346.Summary by CodeRabbit
Chores
Tests