Skip to content

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

Closed
mitasovr wants to merge 6 commits into
constructorfabric:mainfrom
mitasovr:fix/drop-dead-org-unit-id-column
Closed

fix(ingestion): drop always-NULL class_people.org_unit_id column#2044
mitasovr wants to merge 6 commits into
constructorfabric:mainfrom
mitasovr:fix/drop-dead-org-unit-id-column

Conversation

@mitasovr

@mitasovr mitasovr commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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, '')

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

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 in src/ingestion/connectors/, silver/, or gold/ outside explanatory comments. All ~40 p.org_unit_id occurrences in scripts/migrations/ resolve to insight.people, not silver.
  • Frontend checked (insight-front, 51 occurrences): typed string | null, documented as "Department the member belongs to", zero UUID validation or parsing.

dbt parse was not run locally with the project's engine — the available binary is dbt-fusion 2.0.0-preview.200 while the project pins dbt-core>=1.9 + dbt-clickhouse, and fusion reports ~1000 pre-existing errors in unrelated files. None of the five changed files appear among them; CI will validate on the correct engine.

Out of scope (found while investigating)

  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. active_directory__to_class_people.sql projects manager_person_id as Nullable(UUID) while the other three sources and the live column are Nullable(String) — this will raise Code: 386 once active-directory runs alongside another HR source.
  3. 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.

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

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Fixed “org_unit” cohort identification to consistently derive cohort_id from department_name.
    • Ensured empty department names no longer produce cohort identifiers.
    • Removed the org_unit_id field from class_people outputs across HR directory integrations, aligning schemas and downstream cohort behavior.

`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>
@mitasovr
mitasovr requested a review from a team as a code owner July 30, 2026 05:46
@github-actions

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

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8d524891-b233-40f9-bcfd-4475209f4b70

📥 Commits

Reviewing files that changed from the base of the PR and between 2bca164 and 258df50.

📒 Files selected for processing (3)
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql
  • src/ingestion/scripts/connectors-ddl/insight.sql
  • src/ingestion/scripts/connectors-ddl/silver.sql
💤 Files with no reviewable changes (1)
  • src/ingestion/scripts/connectors-ddl/silver.sql

📝 Walkthrough

Walkthrough

HR directory staging views and the Silver class_people schema no longer emit org_unit_id. The org-unit cohort now derives cohort_id directly from non-empty department_name values.

Changes

Org unit cohort derivation

Layer / File(s) Summary
Remove nullable org unit mappings
src/ingestion/connectors/hr-directory/*/dbt/*_to_class_people.sql, src/ingestion/scripts/connectors-ddl/silver.sql
Active Directory, BambooHR, Microsoft Entra, and Workday retain department_name while removing always-null org_unit_id projections and the matching Silver column.
Derive cohort IDs from departments
src/ingestion/gold/metric_entity_cohorts_current.sql, src/ingestion/scripts/connectors-ddl/insight.sql
The org-unit cohort uses nullIf(department_name, '') instead of a UUID-based fallback expression.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: ktursunov

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: removing the always-NULL class_people.org_unit_id column.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

Roman Mitasov and others added 4 commits July 30, 2026 21:35
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 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #2124 — recreated with the branch in this repository instead of a fork, so the full CI suite runs on it. Same commits, plus a sync of the committed connectors-ddl snapshot (silver.sql / insight.sql) that went stale on main after the active-directory bootstrap fix regenerated it.

Closing this one; please review #2124.

@mitasovr mitasovr closed this Aug 3, 2026
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