feat(person-attributes): make HR person attributes groupable with history - #2263
Merged
Conversation
Connector-discovered person attributes become effective-dated facts keyed by source account, ready for query-time cohorting. - Claims derive from the SCD2 snapshots through a dedicated macro rather than fields_history: that macro compares with bare toString, so a transition into or out of NULL propagates to false and vanishes. A lost clear would leave a stale value open forever. - A clear comes only from a delivered empty value. Record absence never closes an interval: no sync-completeness signal exists, and closing on a possibly-partial snapshot would fabricate end dates. - The silver watermark is per source instance. A single global maximum over the union would permanently skip a slower connector's claims. - Account values carry no canonical person id, so an identity correction needs no rebuild; assignment is joined at query time. Refs #2028 Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
📝 WalkthroughWalkthroughThe PR adds connector-specific person-attribute claims models, a shared snapshot-diff macro, Silver claim consolidation, and Gold effective-dated account attribute values. It also adds ClickHouse tables, dbt schemas, integrity tests, and connector documentation. ChangesPerson attribute facts
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
aleksdotbar
marked this pull request as draft
August 6, 2026 07:10
… set Review follow-up. - Claim custom fields from the <conn>_custom_fields var instead of discovering raw_data keys: a key the snapshot does not version on gets claims stamped with the next tracked change's time, not its own. One allow-list now drives versioning and claims; dynamic discovery returns with fetch-all extraction and raw-payload snapshot versioning. - Drop three data tests asserting properties the interval builder guarantees by construction; keep the grain and overlap guards as untagged build-integrity tests failing the build. - Add the missing insight.account_attribute_values bootstrap DDL and order the silver placeholder block; list the new models in the connector docs; trim duplicated prose. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
One staging relation holds every tenant and connection of a connector, and their sync workflows run concurrently. Filtering against a table-wide max(_version) let whichever instance committed first discard another instance's older-stamped claims, which then never reached silver, so the per-instance watermark there had nothing to recover. State that custom-field claims depend on a run supplying the tracked-fields var, matching the snapshot versioning they follow. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Customer-defined fields in the raw payload are no longer claimed. The snapshot versions on a configured subset of them, so a change to any other key produced no version of its own and its claim carried the timestamp of the next tracked change — a wrong effective date is worse than an absent one. Collecting them correctly needs full-payload extraction and versioning together, which arrives with the connector rework. All four producers now have the same shape and read no raw payload. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar
marked this pull request as ready for review
August 6, 2026 08:37
aleksdotbar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/ingestion/dbt/macros/attribute_claims.sql`:
- Around line 66-70: Update the history window used for prev_attrs in the
attribute-claims query to partition by the source account identity rather than
unique_key, while retaining the observed_at ordering and one-row lookback.
Ensure consecutive snapshot versions for the same account share a window so
changed attributes produce both set and clear claims correctly.
🪄 Autofix
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: 8cf8a9e1-9f88-49f7-94b3-ea457d5c08b1
📒 Files selected for processing (20)
docs/components/connectors/hr-directory/ms-entra/README.mddocs/components/connectors/hr-directory/workday/specs/DESIGN.mddocs/components/connectors/hr-directory/workday/workday.mdsrc/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__person_attribute_claims.sqlsrc/ingestion/connectors/hr-directory/active-directory/dbt/schema.ymlsrc/ingestion/connectors/hr-directory/bamboohr/dbt/bamboohr__person_attribute_claims.sqlsrc/ingestion/connectors/hr-directory/bamboohr/dbt/schema.ymlsrc/ingestion/connectors/hr-directory/ms-entra/dbt/ms_entra__person_attribute_claims.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__person_attribute_claims.sqlsrc/ingestion/dbt/macros/attribute_claims.sqlsrc/ingestion/dbt/tests/hr/assert_account_attribute_values_no_overlap.sqlsrc/ingestion/dbt/tests/hr/assert_person_attribute_claims_unique_grain.sqlsrc/ingestion/gold/account_attribute_values.sqlsrc/ingestion/gold/schema.ymlsrc/ingestion/scripts/connectors-ddl/insight.sqlsrc/ingestion/scripts/connectors-ddl/silver.sqlsrc/ingestion/silver/hr/class_person_attribute_claims.sqlsrc/ingestion/silver/hr/schema.yml
Comment on lines
+66
to
+70
| lagInFrame(attrs) OVER ( | ||
| PARTITION BY unique_key | ||
| ORDER BY observed_at | ||
| ROWS BETWEEN 1 PRECEDING AND 1 PRECEDING | ||
| ) AS prev_attrs |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Partition the history window by the source account.
unique_key identifies a snapshot version. The Active Directory snapshot documents that it includes _row_hash, so each changed version enters a different window partition. prev_attrs then defaults to an empty map for each transition.
This emits repeated set claims and suppresses clear claims. Gold does not close the prior interval when a field is cleared.
Proposed fix
lagInFrame(attrs) OVER (
- PARTITION BY unique_key
+ PARTITION BY
+ insight_tenant_id,
+ insight_source_id,
+ source_account_id
ORDER BY observed_at
ROWS BETWEEN 1 PRECEDING AND 1 PRECEDING
) AS prev_attrs📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lagInFrame(attrs) OVER ( | |
| PARTITION BY unique_key | |
| ORDER BY observed_at | |
| ROWS BETWEEN 1 PRECEDING AND 1 PRECEDING | |
| ) AS prev_attrs | |
| lagInFrame(attrs) OVER ( | |
| PARTITION BY | |
| insight_tenant_id, | |
| insight_source_id, | |
| source_account_id | |
| ORDER BY observed_at | |
| ROWS BETWEEN 1 PRECEDING AND 1 PRECEDING | |
| ) AS prev_attrs |
🤖 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/dbt/macros/attribute_claims.sql` around lines 66 - 70, Update
the history window used for prev_attrs in the attribute-claims query to
partition by the source account identity rather than unique_key, while retaining
the observed_at ordering and one-row lookback. Ensure consecutive snapshot
versions for the same account share a window so changed attributes produce both
set and clear claims correctly.
cyberantonz
approved these changes
Aug 6, 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
2 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
Analytics cannot group people by HR attributes. Connector data arrives as
current-state records and per-field change logs; neither says which value was
in effect during a measured period. Cohorting needs effective-dated values
joinable to metric facts.
Change
Ingestion half of person attributes: claims in silver, effective-dated values
in gold.
silver.class_person_attribute_claims— one row per account, field andtransition.
claim_actionseparates a value taking effect from the sourceemptying the field.
insight.account_attribute_values— half-open[valid_from, valid_to)peraccount and field. A cleared attribute has no current row, not an empty one.
attribute_claimsmacro plus one producer per HR connector (BambooHR,Workday, Microsoft Entra, Active Directory), each claiming a curated set of
job, organisation, location and employment fields.
Both relations key on source account and carry no canonical person id.
Assignment joins at query time, so identity corrections need no rebuild.
Out of scope
Policy and governance, snapshot publishers, the analytics catalog, grouping
and comparison APIs, named groups.
Customer-defined fields are also out of scope. The snapshot versions on a
configured subset of them, so claiming the rest would stamp a change with the
timestamp of the next tracked change instead of its own. Claiming them
correctly needs full-payload extraction and versioning together, which lands
with the connector rework (#2270).
Validation
dbt parse, DAG and tag selection, dbt-convention audit.dbt buildon a local ClickHouse over synthetic fixtures across threestates, all four connectors: value change, clear by NULL, clear by empty
string, re-set after clear, custom-field change, unchanged record.
confirmed to fail on their injected defect.
Closes #2262
Summary by CodeRabbit
New Features
Data Quality
Documentation