data: fix #1318 type mismatch + enforce contract on collab silver + dbt-coverage gate - #1320
data: fix #1318 type mismatch + enforce contract on collab silver + dbt-coverage gate#1320SharedQA wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an enforced, fully-typed silver schema for collaboration activity, aligns staging halves to use Nullable(Float64) for visited_page_count, and introduces a manifest-driven dbt_coverage CLI plus a GitHub Actions workflow to report (and optionally enforce) contract and key-test completeness. ChangesData Contract Enforcement and Type Alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/ci/dbt_coverage.py`:
- Around line 66-67: The code reads contract enforcement from n.get("contract")
but manifest stores it under n["config"]["contract"]["enforced"]; update the
enforcement check used to set the enforced variable (the expression that assigns
enforced in scripts/ci/dbt_coverage.py) to traverse config → contract → enforced
(e.g., use n.get("config", {}).get("contract", {}).get("enforced")) so enforced
is true for contract-enforced nodes; leave the tests_by_model lookup (kinds =
tests_by_model.get(uid, set())) unchanged.
🪄 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: f8b1c528-aaa2-4490-b09e-c1a0645776f0
📥 Commits
Reviewing files that changed from the base of the PR and between 43bfb9f and f96f59415c2d603d4593b5335650b9ac387bc329.
📒 Files selected for processing (4)
scripts/ci/dbt_coverage.pysrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_onedrive.sqlsrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_sharepoint.sqlsrc/ingestion/silver/collaboration/schema.yml
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-contracts.yml:
- Line 63: The grep invocation "grep -E 'metric-path models|enforced
contract|key tests|bronze sources|freshness declared' /tmp/dbt-coverage.txt" can
return non-zero and fail the step on harmless output drift; change that command
to tolerate no matches by adding a fallback (for example append "|| true" or
otherwise ensure a non-zero grep exit does not make the job fail) so the step
remains report-only and won't break CI if grep finds nothing.
- Around line 53-54: Replace the failing-suppressed command "dbt deps || true"
with a plain "dbt deps" so dependency resolution failures surface and fail the
CI job; keep the subsequent "dbt parse --no-version-check" as-is so parsing
still runs only after deps succeed.
In `@scripts/ci/dbt_coverage.py`:
- Around line 59-67: The code computes freshness over all manifest "sources" but
prints "bronze sources"; narrow the computation to the intended bronze/Airbyte
subset by creating a filtered dict (e.g., bronze_sources = {uid: s for uid, s in
sources.items() if <bronze-predicate>(s)}) and then replace uses of sources in
the fresh calculation, the count/percentage print, and the for-loop with
bronze_sources (update the print message to still say "bronze sources" and keep
the per-source check using s.get("freshness") and s.get("loaded_at_field"));
alternatively, if you prefer not to filter, change the printed label and comment
from "bronze sources" to "all sources" and leave the existing computations
as-is.
- Around line 39-43: The is_metric_path predicate is too broad because it
currently includes every model in the bronze schema; update the logic in
is_metric_path to stop treating all bronze-schema models as in-scope — only keep
silver/gold schemas plus explicit bronze promotion names and silver-tagged
models. Concretely, change the return expression to remove schema in ("bronze")
(i.e. use schema in ("silver", "gold") or name.endswith("__bronze_promoted") or
any(t == "silver" or t.startswith("silver:") for t in tags)) so only promoted
bronze views, silver feeders, and silver/gold models are considered.
🪄 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: b58adc3c-867c-41d2-bbbe-ee80442864f4
📥 Commits
Reviewing files that changed from the base of the PR and between f96f59415c2d603d4593b5335650b9ac387bc329 and 507664312fdc944ca0166fa09835ea9cfc4b5041.
📒 Files selected for processing (3)
.github/workflows/data-contracts.ymlscripts/ci/dbt_coverage.pysrc/ingestion/silver/collaboration/schema.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ingestion/silver/collaboration/schema.yml
02343af to
612b210
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
… contract, add dbt-coverage gate + CI Fixes the NO_COMMON_TYPE (Float64 vs Int64) build failure in class_collab_document_activity: visited_page_count is pinned to Nullable(Float64) on both M365 halves (OneDrive cast NULL; SharePoint cast through). Enables an ENFORCED dbt contract on class_collab_document_activity (every column declared with its data_type from the live ClickHouse catalog; on_schema_change: fail) so column/type drift fails the build, not production. First model on the path to full contract coverage. scripts/ci/dbt_coverage.py: walks the compiled manifest and flags metric-path models (bronze-promoted -> silver) lacking an enforced contract or not_null+unique on their key column, plus bronze source-freshness coverage. Validated locally with dbt-core 1.10 + dbt-clickhouse 1.9 (dbt parse). .github/workflows/data-contracts.yml: runs the gate in PR CI via dbt parse only (no warehouse/credentials), report-only, uploading the per-model gap list. Wording is neutral throughout (user-facing terms only). Closes constructorfabric#1318. Related: constructorfabric#1319, constructorfabric#1321, constructorfabric#1326. Signed-off-by: Kenan Salim <kenan.salim@rolos.com>
612b210 to
cb5b334
Compare
- is_metric_path: drop blanket bronze-schema scope; keep bronze-promotion views (__bronze_promoted) + silver/gold + silver:* feeders so the denominator matches the gate's stated scope. - sources freshness: relabel 'bronze sources' -> 'sources' (it counts all declared sources) and fix unused loop var (ruff B007). - CI: fail fast on 'dbt deps' instead of '|| true'; guard summary grep with '|| true' so harmless output drift can't red the step. Signed-off-by: Kenan Salim <ks@constructor.tech>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/ci/dbt_coverage.py`:
- Around line 75-89: The code only extracts single column_name values from tests
but unique_combination_of_columns tests store their columns in
kwargs.combination_of_columns as a list instead. After the existing column
extraction logic that looks for column_name, add fallback logic to extract the
list of columns from kwargs.combination_of_columns when column_name is not
present. Then modify the loop that processes dependencies and adds tests to
col_tests to handle both single columns and multiple columns from the
combination, ensuring the test kind is recorded for each column in the
combination so that composite unique tests are properly validated.
🪄 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: cc1e1628-2051-414d-97e2-91cb8a51feb5
📥 Commits
Reviewing files that changed from the base of the PR and between 507664312fdc944ca0166fa09835ea9cfc4b5041 and 8e73749.
📒 Files selected for processing (4)
.github/workflows/data-contracts.ymlscripts/ci/dbt_coverage.pysrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_onedrive.sqlsrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_sharepoint.sql
💤 Files with no reviewable changes (2)
- src/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_sharepoint.sql
- src/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_onedrive.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/data-contracts.yml
…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>
…constructorfabric#1335) Drop data-contracts.yml + dbt_coverage.py from this PR — they now live in the single data-quality.yml on constructorfabric#1335 (one data-gate workflow instead of three). This PR keeps only its substance: the m365 collab_document type-mismatch fix (constructorfabric#1318) + the silver collaboration contract. Signed-off-by: SharedQA <122366558+SharedQA@users.noreply.github.com>
|
reworked here #1695 |
First slice of the data-correctness work, three coherent parts.
1. Fix #1318 — the real type-mismatch bug
class_collab_document_activityfailed ClickHouseNO_COMMON_TYPE: Float64 vs Int64:visited_page_countwasInt64from the OneDrive half andFloat64from SharePoint, so the silver tag-union couldn't pick a supertype. Pinned toNullable(Float64)on both halves (OneDriveCAST(NULL …), SharePointCAST(visitedPageCount …)).2. Enforce a dbt contract on the collab silver model (first step to contract = 100%)
class_collab_document_activitynow hascontract: { enforced: true }with every column declared (name +data_type, taken from the live ClickHouse catalog). dbt verifies the built relation against the declared schema at build time, so a future Float64↔Int64 (or any) drift becomes a build failure, not a runtime ClickHouse error — the systematic prevention for the #1318 class.3.
scripts/ci/dbt_coverage.py— the data-contract gateWalks the compiled dbt manifest and fails if any metric-path model (schema silver/gold, plus
silver:*tag-union feeders) lacks an enforced contract or not_null+unique on its key. This is the mechanical enforcer for the "dbt/contract → 100%" targets. Wire-up:make contracts+ a CI step afterdbt parse(lands with the gate workflows once the enforcement PR merges).Validation note
dbtcan't be installed on the dev host, so I took the contractdata_types from the deployed ClickHouse catalog (DESCRIBE TABLE) rather than guessing. CI'sdbt buildis the authority — if any declared type is off, the contract (correctly) fails the build.Closes #1318 · Related #1319 (wired-empty connectors: seed-or-deregister decision).
Summary by CodeRabbit
not_null/uniquetests for configured key columns (currently report-focused by default).visited_page_countto useFloat64for M365 document-activity models to avoid type-union failures.