Skip to content

fix(ingestion): drop always-NULL class_people.org_unit_id column - #2124

Merged
mitasovr merged 8 commits into
mainfrom
fix/drop-dead-org-unit-id-column
Aug 3, 2026
Merged

fix(ingestion): drop always-NULL class_people.org_unit_id column#2124
mitasovr merged 8 commits into
mainfrom
fix/drop-dead-org-unit-id-column

Conversation

@mitasovr

@mitasovr mitasovr commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Recreated from #2044 so the branch lives in this repo rather than a fork (fork PRs skip part of CI). Same commits, now also syncing the committed DDL snapshot.

Problem

A data-quality audit flagged silver.class_people.org_unit_id as "high in bronze inputs but 0% populated in silver — lost in transform", citing MariaDB identity.org_chart (252 rows) as proof that org data exists.

That framing is wrong on all three counts:

  1. Not lost in transform. All four HR staging models hardcode CAST(NULL AS Nullable(UUID)) AS org_unit_id, following the type-uniformity convention in docs/domain/ingestion-data-flow/specs/DESIGN.md.
  2. No org-unit UUID exists anywhere. SELECT … FROM system.tables WHERE name ILIKE '%org_unit%' returns nothing in ClickHouse, and there is no such table in MariaDB either. The org-chart design (docs/domain/org-chart/specs/) that would mint these UUIDs is unimplemented — its PRD even claims all its tables live in ClickHouse, which is not the case.
  3. identity.org_chart is not an org-unit table. Its columns are child_person_idparent_person_id — a person-to-supervisor edge graph rebuilt by the identity-resolution service from BambooHR supervisorEmail. It has no org_unit_id column, so it could never have been the source.

What org_unit_id actually means downstream

Everything in the serving path called org_unit_id is already a department name string:

bronze department
  → insight.people.org_unit_id             (argMax(department), read straight from bronze)
  → insight.team_member                    (FROM insight.people AS p)
  → frontend RawTeamMemberRow.org_unit_id  (typed `string | null`)
  → frontend sends back `org_unit_id in ('Engineering', …)`

Live values on virtuozzo are VZ - R&D - Engineering, VZ - Support, etc. The frontend round-trips the value and never sees the silver column; the four CRM gold views likewise use coalesce(department_name, 'Unknown') AS org_unit_id.

The latent trap this removes

The only reader of the silver column was gold/metric_entity_cohorts_current.sql:

coalesce(nullIf(toString(org_unit_id), ''), nullIf(department_name, ''))

Because that coalesce preferred org_unit_id, anyone "fixing" the reported 0% by populating the column would have flipped cohort_id from department names to UUIDs, while insight.people and the frontend kept sending names — silently emptying every peer metric tenant-wide. Dropping the column removes that possibility and stops the audit false positive from recurring. The replacement expression carries a comment warning against reintroducing a UUID branch without migrating insight.people and the frontend in the same change.

Changes

File Change
active_directory__to_class_people.sql drop the org_unit_id projection
bamboohr__to_class_people.sql drop the org_unit_id projection
ms_entra__to_class_people.sql drop the org_unit_id projection
workday__to_class_people.sql drop the org_unit_id projection
gold/metric_entity_cohorts_current.sql coalesce(...)nullIf(department_name, '')
connectors-ddl/silver.sql drop org_unit_id Nullable(UUID) from the silver.class_people snapshot
connectors-ddl/insight.sql same cohort expression change in the snapshotted view

No DDL migration is needed: silver.class_people is materialized='table', so dbt rebuilds it without the column.

Every other org_unit_id in the snapshot is deliberately untouched and is a different thing: the insight.sql occurrences are the department NAME string (String columns, argMax(department), coalesce(department_name, 'Unknown')), and the person.sql occurrences belong to person.persons, the identity-owned golden record created by the init-identity migration.

Verification

  • Output equivalence against live virtuozzo data. The rewritten cohort query returns 1402 rows / 63 cohorts / 1402 entities — identical to the current view. The symmetric set difference of (tenant_id, entity_id, cohort_id) is 0 in both directions.
  • Column parity. All four staging models still project an identical 25-column set, satisfying cpt-dataflow-constraint-staging-class-column-types-match (removing the column from only some models would raise Code: 386).
  • No remaining references. org_unit_id no longer appears as a projection or column anywhere in src/ingestion/connectors/, silver/, gold/, or connectors-ddl/silver.sql — only in explanatory comments.
  • Frontend checked (insight-front, 51 occurrences): typed string | null, documented as "Department the member belongs to", zero UUID validation or parsing.
  • The Connector wiring invariants check added in fix: active-directory half-landed connector — unbreak bootstrap-db, wire image ref, add PR-time wiring guard (#2048) #2056 passes on this branch.

Out of scope

  1. insight.people is bamboohr-only. It reads bronze_bamboohr.employees directly (bronze → gold, skipping silver), so a tenant on ms-entra, workday, or active-directory gets an empty insight.people and loses all org attribution. Virtuozzo only survives because BambooHR is its sole HR source. Repointing it at silver would need a fix for supervisor_email first: class_people.manager_person_id holds supervisorEId (an ID, not an email).
  2. Missing AccessScope check on the org_unit_id / person_id $filter values in analytics/src/api/handlers.rs (a TODO at the injection site), which docs/components/backend/specs/analytics-views-api.md requires. Tenant isolation is enforced; within-tenant cross-team reads are not.

The manager_person_id type mismatch originally reported here was fixed upstream in #2056 and is no longer part of this PR.

Refs: docs/domain/ingestion-data-flow/specs/DESIGN.md, docs/domain/org-chart/specs/DESIGN.md

🤖 Generated with Claude Code

Roman Mitasov and others added 7 commits July 30, 2026 13:45
`silver.class_people.org_unit_id Nullable(UUID)` was hardcoded to NULL by all
four HR staging models and had no producer anywhere: no `org_units` table
exists in ClickHouse or MariaDB, so no org-unit UUID is ever minted. The
org-chart domain design (`docs/domain/org-chart/specs/`) that would own such
UUIDs is unimplemented.

The column was reported by a data-quality audit as "high in bronze inputs but
0% in silver — lost in transform". It is neither: bronze carries `department`
(a name), not a UUID, and MariaDB `identity.org_chart` is a
`child_person_id -> parent_person_id` supervisor edge table with no
`org_unit_id` column at all.

Everything downstream that is called `org_unit_id` is already a department
NAME string, sourced from `department`:

  bronze department
    -> insight.people.org_unit_id  (argMax(department))
    -> insight.team_member         (FROM insight.people)
    -> frontend RawTeamMemberRow.org_unit_id (typed `string | null`)
    -> frontend sends back `org_unit_id in ('Engineering', ...)`

The sole reader of the silver column was
`gold/metric_entity_cohorts_current.sql`, which coalesced past it to
`department_name`. That coalesce was also a latent trap: because it preferred
`org_unit_id`, populating the column would have flipped cohort ids from
department names to UUIDs while `insight.people` and the frontend kept using
names, silently emptying every peer metric. Dropping the column removes the
trap and stops the audit false positive from recurring.

Verified on the virtuozzo cluster: the rewritten cohort query returns 1402
rows / 63 cohorts / 1402 entities, and the symmetric set difference of
(tenant_id, entity_id, cohort_id) against the current view is 0 in both
directions. All four staging models still project an identical 25-column set,
satisfying cpt-dataflow-constraint-staging-class-column-types-match.

Refs: docs/domain/ingestion-data-flow/specs/DESIGN.md,
docs/domain/org-chart/specs/DESIGN.md

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
The committed DDL snapshot landed upstream (via the active-directory
bootstrap-db fix) while `class_people.org_unit_id` still existed, so it
declares a column this branch removes:

- `connectors-ddl/silver.sql`: drop `org_unit_id Nullable(UUID)` from
  `silver.class_people`.
- `connectors-ddl/insight.sql`: `metric_entity_cohorts_current` now reads
  `nullIf(department_name, '')` instead of coalescing the dropped column
  ahead of the department name.

Every other `org_unit_id` in the snapshot is left untouched and is a
different thing: `insight.sql` occurrences are the department NAME string
(`String` columns, `argMax(department)`, `coalesce(department_name,
'Unknown')`), and `person.sql` occurrences belong to `person.persons`, the
identity-owned golden record created by the init-identity migration.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
@mitasovr
mitasovr requested a review from a team as a code owner August 3, 2026 03:36
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@mitasovr, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

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, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3397d55f-9334-46c3-844e-303e63c0306f

📥 Commits

Reviewing files that changed from the base of the PR and between 59e87f0 and 898b7af.

📒 Files selected for processing (7)
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql
  • src/ingestion/connectors/hr-directory/bamboohr/dbt/bamboohr__to_class_people.sql
  • src/ingestion/connectors/hr-directory/ms-entra/dbt/ms_entra__to_class_people.sql
  • src/ingestion/connectors/hr-directory/workday/dbt/workday__to_class_people.sql
  • src/ingestion/gold/metric_entity_cohorts_current.sql
  • src/ingestion/scripts/connectors-ddl/insight.sql
  • src/ingestion/scripts/connectors-ddl/silver.sql

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

⚠️ Regenerate the connectors-ddl snapshot

This PR changes src/ingestion/**. If your change affects any
bronze / silver / gold schema, regenerate the committed DDL snapshot
and include it in this PR.

Prerequisites (details: src/ingestion/scripts/bootstrap-db/README.md):

  • docker + a fresh throwaway ClickHouse 25.7.5 (README "Local ClickHouse for testing")
  • .env from .env.bootstrap.example pointing at it; use the host LAN IP,
    reachable from both the host and connector containers
    (host.docker.internal does not resolve on the macOS host itself)
  • python3.12 or python3.11 on PATH (pinned dbt venv)
  • HubSpot + Salesforce credentials in .env — their discover calls the
    live APIs; without them, apply ../connectors-ddl/{hubspot,salesforce}.sql
    (relative to bootstrap-db/) to seed their bronze, then run the dbt step
cd src/ingestion/scripts/bootstrap-db
set -a; source pins.env; source .env; set +a
./bootstrap-db.sh connectors-config.yaml   # fresh ClickHouse 25.7.5
./dump-ddl.sh                              # writes scripts/connectors-ddl/*.sql

Commit the resulting scripts/connectors-ddl/*.sql diff. If nothing
changed, no snapshot update is needed. (Regeneration is manual for now.)

@mitasovr
mitasovr enabled auto-merge August 3, 2026 03:48
@mitasovr
mitasovr added this pull request to the merge queue Aug 3, 2026
Merged via the queue into main with commit dcda409 Aug 3, 2026
47 checks passed
@mitasovr
mitasovr deleted the fix/drop-dead-org-unit-id-column branch August 3, 2026 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

People data exposes an empty org-unit field while cohorts use department names

3 participants