fix(hr): make class_people a true one-row-per-person snapshot - #2125
Merged
Conversation
`silver.class_people` overstated headcount by ~5% on virtuozzo (1440 rows for
1420 employees; 414 "active" rows for 394 active people). Two defects
compounded — neither is sufficient alone:
1. The `__to_class_people` staging views read append-only bronze WITHOUT
`FINAL`. Airbyte appends a full snapshot per sync and bronze is
RMT(_airbyte_extracted_at), which only collapses on background merge, so a
bare read emits every unmerged snapshot row.
2. Those views appended a version axis (`lastChanged` / `createdDateTime` /
`whenCreated` / `Last_Functionally_Updated`) to `unique_key`, per ADR-0004.
That turned each transient bronze duplicate into a permanently distinct
silver key, so the versionless RMT could never collapse it — and `FINAL`
could not either, because the keys genuinely differ. `valid_to` was
hardcoded NULL, so nothing marked which row was current.
The surplus was regenerated on every run and drifted with bronze merge timing,
which is why it surfaced as two separate audit findings ("duplicate active
emails" and "active-count drift").
class_people is a current-state snapshot, not an SCD2 history table: it is
`materialized='table'` and rebuilt in full each run, so it cannot accumulate
history by construction. HR attribute history already lives in the per-source
`*_snapshot` / `*_fields_history` chain, which is incremental+append and tracks
strictly more fields.
Changes:
* All four `__to_class_people` views (active-directory, bamboohr, ms-entra,
workday): entity-level `unique_key`, `valid_to` dropped, bronze read with
`FINAL`. Projections stay aligned 1:1 for the positional UNION.
* `ELSE 'active'` catch-alls replaced with `'unknown'`. A BambooHR record with
`status=''` / `employmentHistoryStatus='Third party'` was being counted as
active — this is the residual +1 between bronze (393) and silver (394).
`accepted_values` updated accordingly.
* Same missing `FINAL` fixed in `bamboohr__working_hours` /
`workday__working_hours`: there the `status='Active'` filter ran before
dedup, so a leaver could still qualify via a stale snapshot row.
* New `assert_class_people_one_row_per_person` data-quality check, plus
`unique`/`not_null` on `class_people.unique_key`.
* Reconcile the contract, which contradicted itself: ADR-0004 mandated the
version axis while DESIGN.md and ADR-0001 called class_people a pure
snapshot, and ADR-0001 exempted it from the read-dedup cleanup on the
now-false premise that it "already collapses to one row per key".
Verified against the live virtuozzo cluster: the fixed transform yields
1420 rows / 1420 distinct keys, active 394 -> 393, unknown 1; the new check
reports exactly the 20 current violations and goes green after a rebuild.
No migration needed — `materialized='table'` fully replaces the table.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
📝 WalkthroughWalkthroughThe PR changes HR ChangesHR current-state ingestion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Bronze
participant HRConnectorModels
participant union_by_tag
participant SilverClassPeople
Bronze->>HRConnectorModels: Read source rows with FINAL
HRConnectorModels->>union_by_tag: Provide entity-level unique_key rows
union_by_tag->>SilverClassPeople: Select versionless current-state rows
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
|
ktursunov
approved these changes
Aug 3, 2026
mitasovr
enabled auto-merge
August 3, 2026 03:47
The committed DDL snapshot still declared `valid_to Nullable(DateTime)` on `silver.class_people`. The `connectors-ddl snapshot + field parity` job rebuilds the database from the dbt models and re-dumps the DDL, so removing the column from the four `__to_class_people` producers made the snapshot drift and fail the gate. This is exactly the one-line delta the CI drift check computed; no other snapshot references `valid_to`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/components/connectors/hr-directory/README.md (1)
431-461: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the schema table with the shipped DDL.
Line 430 still calls
workspace_ida partition-key component, but Line 454 states that the table has no partition key. The DDL also definesvalid_from,source_person_id, and several person attributes as nullable, and definesorg_unit_idasNullable(UUID), notString. Update the table so it remains an accurate ClickHouse schema reference.🤖 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 `@docs/components/connectors/hr-directory/README.md` around lines 431 - 461, The schema table in the class people documentation is inconsistent with the shipped DDL. Remove the claim that workspace_id is part of the partition key, update valid_from, source_person_id, and the nullable person attributes to reflect their Nullable definitions, and change org_unit_id from String to Nullable(UUID). Keep the Engine/ORDER BY and no-partition-key description aligned with the DDL.
🤖 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/components/connectors/hr-directory/README.md`:
- Around line 67-79: Remove the shipped-flow claim that HR Silver produces
class_org_units in the paragraph describing canonical tables, keeping
class_people as the only currently produced table. Preserve the existing
statement that class_org_units does not exist, and describe it as planned only
if that terminology is already supported by the surrounding documentation.
In `@docs/shared/glossary/README.md`:
- Around line 521-522: Remove the `valid_from` to `effective_from` entry for
`bamboohr__to_class_people.sql` from the glossary convention-violations table;
treat `class_people` as a current-state snapshot whose `valid_from` is the
source change timestamp, without adding an exception unless required by the
surrounding documentation.
In
`@src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql`:
- Line 24: Preserve missing unique_key values so not_null validation can detect
them, or quarantine rows lacking an entity key, instead of coalescing them to an
empty string. Apply this change to the unique_key projection in
active_directory__to_class_people.sql (line 24), bamboohr__to_class_people.sql
(line 24), and ms_entra__to_class_people.sql (line 24), while retaining the
existing valid-key behavior.
In `@src/ingestion/silver/_shared/schema.yml`:
- Around line 5-13: Update the shared class_people contract description to
include Active Directory among the HR sources and accurately document
provider-specific valid_from semantics, including that Active Directory and MS
Entra use creation timestamps (whenCreated and createdDateTime) rather than
last-change timestamps; keep the contract aligned with the existing connector
projections.
---
Outside diff comments:
In `@docs/components/connectors/hr-directory/README.md`:
- Around line 431-461: The schema table in the class people documentation is
inconsistent with the shipped DDL. Remove the claim that workspace_id is part of
the partition key, update valid_from, source_person_id, and the nullable person
attributes to reflect their Nullable definitions, and change org_unit_id from
String to Nullable(UUID). Keep the Engine/ORDER BY and no-partition-key
description aligned with the DDL.
🪄 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 Plus
Run ID: 05201ca6-628c-476f-adb8-55fd4d10c88d
📒 Files selected for processing (18)
docs/components/connectors/hr-directory/README.mddocs/domain/ingestion-data-flow/specs/ADR/0001-rmt-with-version-and-unique-key.mddocs/domain/ingestion-data-flow/specs/ADR/0004-unique-key-formula.mddocs/shared/glossary/README.mdsrc/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sqlsrc/ingestion/connectors/hr-directory/active-directory/dbt/schema.ymlsrc/ingestion/connectors/hr-directory/bamboohr/dbt/bamboohr__to_class_people.sqlsrc/ingestion/connectors/hr-directory/bamboohr/dbt/bamboohr__working_hours.sqlsrc/ingestion/connectors/hr-directory/bamboohr/dbt/schema.ymlsrc/ingestion/connectors/hr-directory/ms-entra/dbt/ms_entra__to_class_people.sqlsrc/ingestion/connectors/hr-directory/ms-entra/dbt/schema.ymlsrc/ingestion/connectors/hr-directory/workday/dbt/schema.ymlsrc/ingestion/connectors/hr-directory/workday/dbt/workday__to_class_people.sqlsrc/ingestion/connectors/hr-directory/workday/dbt/workday__working_hours.sqlsrc/ingestion/dbt/macros/union_by_tag.sqlsrc/ingestion/dbt/tests/hr/assert_class_people_one_row_per_person.sqlsrc/ingestion/scripts/connectors-ddl/silver.sqlsrc/ingestion/silver/_shared/schema.yml
💤 Files with no reviewable changes (1)
- src/ingestion/scripts/connectors-ddl/silver.sql
Comment on lines
+67
to
+79
| > **Implementation status.** This document is the original HR Silver design. Parts of it describe components that were never built, and they must not be read as the current contract: | ||
| > | ||
| > - There is **no "SCD2 Merge" component**. Bronze → Silver is plain dbt (`<source>__to_class_people` staging views unioned by `union_by_tag` into `silver.class_people`). Every section describing row-closing writes (`UPDATE ... SET valid_to = ...`) is unimplemented design. | ||
| > - **`class_people` has no `valid_to`** and holds no version history — see below. | ||
| > - **`class_org_units` does not exist** as a table or a dbt model. `class_people.org_unit_id` is consequently unpopulated; department attribution currently flows through `department_name`. | ||
| > | ||
| > For the shipped data-flow contract see [ADR-0001](../../../domain/ingestion-data-flow/specs/ADR/0001-rmt-with-version-and-unique-key.md) and [ADR-0004](../../../domain/ingestion-data-flow/specs/ADR/0004-unique-key-formula.md). | ||
|
|
||
| The HR Silver Layer transforms raw HR directory data from Bronze source tables (BambooHR, MS Entra, Workday, LDAP/Active Directory) into two canonical, workspace-isolated Silver tables: `class_people` and `class_org_units`. | ||
|
|
||
| `class_people` is a **current-state snapshot**: exactly one row per person per source, keyed on an entity-level `unique_key`. `valid_from` records when the source last changed the record; there is no `valid_to`. It is `materialized='table'` and rebuilt in full on every run, so it cannot accumulate history by construction. | ||
|
|
||
| HR attribute history lives in the per-source SCD2 chain — `<source>__<entity>_snapshot` and `<source>__<entity>_fields_history` — which are `incremental`/`append` and therefore genuinely accumulate versions across syncs, tracking strictly more fields than `class_people` exposes. Do not add version rows to `class_people`: see ADR-0004 and ADR-0001 for the headcount-inflation bug this caused. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the claim that class_org_units is currently produced.
Line 71 states that class_org_units does not exist. Line 75 states that the flow transforms data into class_people and class_org_units. Mark class_org_units as planned, or remove it from the shipped-flow description.
🤖 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 `@docs/components/connectors/hr-directory/README.md` around lines 67 - 79,
Remove the shipped-flow claim that HR Silver produces class_org_units in the
paragraph describing canonical tables, keeping class_people as the only
currently produced table. Preserve the existing statement that class_org_units
does not exist, and describe it as planned only if that terminology is already
supported by the surrounding documentation.
Comment on lines
+521
to
+522
| | `src/ingestion/connectors/hr-directory/bamboohr/dbt/bamboohr__to_class_people.sql` | bare `tenant_id` | `insight_tenant_id` (section 3.1) | | ||
| | `src/ingestion/connectors/hr-directory/bamboohr/dbt/bamboohr__to_class_people.sql` | `valid_from` | `effective_from` (section 4.2) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the incorrect valid_from convention violation.
Section 4.2 applies to records with an effective validity range. class_people is a current-state snapshot with no valid_to, and its valid_from records the source change time. Remove this entry, or document the snapshot timestamp as an explicit exception.
🤖 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 `@docs/shared/glossary/README.md` around lines 521 - 522, Remove the
`valid_from` to `effective_from` entry for `bamboohr__to_class_people.sql` from
the glossary convention-violations table; treat `class_people` as a
current-state snapshot whose `valid_from` is the source change timestamp,
without adding an exception unless required by the surrounding documentation.
| -- ORDER BY unique_key) collapses to one row per user. Do NOT add a version | ||
| -- axis here — that would make every changed record a second | ||
| -- permanently-"current" row (see ADR-0004). | ||
| CAST(coalesce(u.unique_key, '') AS String) AS unique_key, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep missing entity keys detectable in every connector.
All three models convert a missing unique_key into ''. This bypasses not_null validation and can let malformed identities enter silver.class_people.
src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql#L24-L24: preserveNULL, or quarantine the invalid row.src/ingestion/connectors/hr-directory/bamboohr/dbt/bamboohr__to_class_people.sql#L24-L24: preserveNULL, or quarantine the invalid row.src/ingestion/connectors/hr-directory/ms-entra/dbt/ms_entra__to_class_people.sql#L24-L24: preserveNULL, or quarantine the invalid row.
📍 Affects 3 files
src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql#L24-L24(this comment)src/ingestion/connectors/hr-directory/bamboohr/dbt/bamboohr__to_class_people.sql#L24-L24src/ingestion/connectors/hr-directory/ms-entra/dbt/ms_entra__to_class_people.sql#L24-L24
🤖 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/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql`
at line 24, Preserve missing unique_key values so not_null validation can detect
them, or quarantine rows lacking an entity key, instead of coalescing them to an
empty string. Apply this change to the unique_key projection in
active_directory__to_class_people.sql (line 24), bamboohr__to_class_people.sql
(line 24), and ms_entra__to_class_people.sql (line 24), while retaining the
existing valid-key behavior.
Comment on lines
+5
to
+13
| description: > | ||
| Unified person registry from all HR sources (BambooHR, MS Entra, Workday). Current-state snapshot: exactly one row per person per source. NOT an SCD2 history table — HR attribute history lives in the per-source `*_snapshot` / `*_fields_history` chain, which accumulates across syncs (this model is `materialized='table'` and is rebuilt in full every run, so it cannot retain history). `valid_from` records when the source last changed the record; there is no `valid_to` — see ADR-0004. | ||
| columns: | ||
| - name: unique_key | ||
| description: > | ||
| Entity-level key (`{tenant}-{source}-{source_person_id}`) and the RMT ORDER BY column. MUST NOT carry a version axis: a per-version key stops silver RMT from collapsing versions, so every changed record becomes a second permanently-current row. | ||
| tests: | ||
| - not_null | ||
| - unique |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the shared class_people contract.
Line 6 omits Active Directory, although active_directory__to_class_people.sql emits source = 'active-directory'. Line 6 also states that valid_from records the last source change, but Active Directory and MS Entra project whenCreated and createdDateTime. Define the provider-specific timestamp semantics accurately, or align all connector projections.
🤖 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/ingestion/silver/_shared/schema.yml` around lines 5 - 13, Update the
shared class_people contract description to include Active Directory among the
HR sources and accurately document provider-specific valid_from semantics,
including that Active Directory and MS Entra use creation timestamps
(whenCreated and createdDateTime) rather than last-change timestamps; keep the
contract aligned with the existing connector projections.
cyberantonz
approved these changes
Aug 3, 2026
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
3 participants
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.
Problem
A data-quality audit on virtuozzo reported two findings against
silver.class_people:They are one root cause, and the dedup is not broken — ReplacingMergeTree is behaving correctly. Live numbers: 1440 rows for 1420 employees, 414
activerows for 394 active people (~5% headcount inflation),countIf(valid_to IS NOT NULL) = 0.Root cause: two defects compounding
Neither is sufficient alone.
__to_class_peoplestaging views read append-only bronze withoutFINAL. Airbyte appends a full snapshot per sync, and bronze isReplacingMergeTree(_airbyte_extracted_at), which only collapses on background merge. A bare read therefore emits every unmerged snapshot row. (Caught live: bronze went from 1420 rows / 1 part to 2840 rows / 2 parts mid-investigation.)unique_key(lastChanged/createdDateTime/whenCreated/Last_Functionally_Updated), as ADR-0004 required. That turns each transient bronze duplicate into a permanently distinct silver key — so the versionless RMT can never collapse it, andFINALcannot either, because the keys genuinely differ.SELECT count() FROM silver.class_people FINALreturns the same 1440.valid_towas hardcodedNULLin every producer, so nothing marked which row was current.Because the table is
materialized='table', the surplus is regenerated on every run and drifts with bronze merge timing — which is exactly why it surfaced as two separate symptoms. The nightly rebuilt it mid-investigation and the surplus moved 19 → 20.The arithmetic, end to end
The residual vs bronze's 393
status='Active'is +1, and it is not a status disagreement between versions. It is theELSE 'active'catch-all: exactly one bronze row hasstatus=''withemploymentHistoryStatus='Third party'and falls through toactive. 393 + 1 = 394. ✔Why the contract allowed this
Two normative documents disagreed:
ADR-0004:58unique_keyis mandatory forclass_peopleDESIGN.md:178class_peopleis a snapshot "with no event history kept at this layer"hr-directory/README.md:168valid_to IS NULL", enforced by an "SCD2 Merge component"ADR-0001:109class_peoplefrom the read-dedup cleanup because it "already collapse[s] to one row per key"ADR-0001:109is the load-bearing error: "one row per key" only equals "one row per person" whileunique_keyis the entity key — which ADR-0004 had already stopped being true.History is genuinely covered elsewhere, which is what makes the snapshot reading safe: each connector already has
*_employees_snapshot/*_fields_history(incremental+append, 17 tracked columns) that accumulate across syncs.class_peopleismaterialized='table'and cannot retain history by construction. Noteclass_hr_eventsis not that history — it readsleave_requestsonly.Changes
__to_class_peopleviews (active-directory, bamboohr, ms-entra, workday): entity-levelunique_key,valid_todropped, bronze read withFINAL. All four projections verified aligned 1:1 (25 columns) for the positionalUNION ALL.ELSE 'active'→'unknown'in all four, withaccepted_valuesupdated. Defaulting an unrecognised source status toactivesilently inflates headcount.FINALinbamboohr__working_hours/workday__working_hours. No row inflation there (unique_keywas already entity-level), butWHERE status='Active'ran before dedup, so a leaver could still qualify via a stale snapshot row.assert_class_people_one_row_per_persondata-quality check (data_quality,tier: error), plusunique/not_nullonclass_people.unique_key. There was previously no uniqueness test on this table and zerodata_qualitytests touching it.union_by_tagdocstring, HR README, four connectorschema.yml, glossary.Verification (live virtuozzo, read-only)
Simulating the fixed transform against live bronze:
activeunknownThe new DQ check returns exactly the 20 current violations against the deployed table, and goes green after a rebuild.
Also demonstrated why
FINALis mandatory rather than defensive: the versionlessLIMIT 1 BY unique_keyinunion_by_taghas noORDER BY, so the surviving row is undefined. Atmax_threads=1it picks the stale row for 20 of 20 changed entities; atmax_threads=4/16, the fresh one.Consumers
gold/metric_entity_cohorts_current.sql— already correct (LIMIT 1 BYordered byvalid_from DESC); keeps working, now over a unique input.crm-gold-views.sql(4xsilver.class_people FINAL) —FINALwas a false safety signal; thepeopleCTE isLEFT JOINed onlower(email), so duplicates fan out and doublecount()/sum(). Currently latent (verified 0 overlap between CRM owner emails and the 20 duplicated emails); fixed at the source by this PR.insight.people— reads bronze directly withargMax; unaffected.class_peopleinservices/analytics.valid_tohad zero readers anywhere insrc/.Migration
None needed.
class_peopleismaterialized='table', so onedbt runreplaces it wholesale and the surplus rows disappear — unlike the column-order heal precedent in20260716000000_class_contract_heal.sql, which exists for incremental tables with positional inserts.Expect
count() = uniqExact(source, source_person_id) = 1420andcountIf(status='active') = 393.Deliberately not in this PR
audit_rmt_read_dedup.pyhas a false negative that hid this bug. It matchessource('<name>','<table>')against the physical<schema>.<table>recorded bypromote_bronze_to_rmt, so connectors declaring an unprefixed source name (bamboohr,workday— bothclass_peopleproducers) are silently classified non-RMT and skipped;ms-entrais caught because its source is namedbronze_ms_entra. The one-line fix surfaces 5 further genuine gaps. Left out because the file predates the repo'sruffconfig and touching it trips 31 pre-existing lint errors plus a whole-file reformat, which would bury this change. Follow-up needed.silver/hr/schema.yml:7referencesdbt_utils.unique_combination_of_columns, butdbt_utilsis not installed anywhere (nopackages.yml, nodbt deps, not vendored) — a pre-existing latent breakage. Follow-up needed.hr_eventsmodels also read bronze withoutFINAL, butunion_by_tag's versioned dedup path (QUALIFY ROW_NUMBER ... ORDER BY _version DESC) protects them, unlike the versionless path used here.Rebase note
Rebased onto
upstream/main(178 commits) and moved from themitasovrfork into this repository; supersedes #2045, which is closed.One conflict, resolved in favour of upstream:
active_directory__to_class_people.sqlhadmanager_person_idretypedNullable(UUID)→Nullable(String)andhire_date/termination_dateDate→DateTimeon main (theNO_COMMON_TYPEalignment fix). Those types are kept as-is; this PR only removesvalid_to, drops the version axis fromunique_key, addsFINAL, and changes the status catch-all. Re-verified after the rebase: all four producers havevalid_togone,FINALpresent, noELSE 'active', no version concat, and their column projections are byte-identical.🤖 Generated with Claude Code
Summary by CodeRabbit
Data Model Updates
valid_tofield from people records.unknownemployment status across HR directory sources.Data Quality
Documentation