fix(dimensional): fix dim_discount grain mismatch causing tfact_order fan-out by blarghmatey · Pull Request #2411 · mitodl/ol-data-platform

blarghmatey · 2026-07-09T12:05:07Z

Summary

dim_discount.discount_pk was hashed from (source_discount_id, discount_code, platform_code), but the model docs and tfact_order's join both assume (source_discount_id, platform_code) is the unique grain.
When a discount's discount_code is updated over its lifetime, dim_discount emitted multiple rows per (source_discount_id, platform_code), so tfact_order's dimension join on those two columns fanned out — and the final dedup in tfact_order picked an arbitrary discount_fk because order_updated_on was identical across the fanned rows.
Dedupe dim_discount to one row per (source_discount_id, platform_code), keeping the most recently updated discount_code, and hash the surrogate key from just those two columns to match the documented and consumed grain.
Added discount_code as a deterministic tie-breaker in the dedup ROW_NUMBER() and an explicit dbt_utils.unique_combination_of_columns test on (source_discount_id, platform_code) per review feedback.
Documented (in dim_discount.sql and tfact_order's discount_fk column) that this surrogate-key change requires a --full-refresh of tfact_order in the same deploy, since dim_discount is full-refresh but tfact_order is incremental and won't otherwise recompute discount_fk for historical rows.

Test plan

dbt parse and dbt compile --select dim_discount tfact_order succeed against the dev_local duckdb target
pre-commit (sqlfluff, yamllint, etc.) passes on changed files
CI dbt build/test run

Deployment notes

After this merges, run dbt run --select tfact_order --full-refresh (or the equivalent full pipeline full-refresh) to avoid orphaned discount_fk values on historical order lines — see inline comments in dim_discount.sql / _fact_tables.yml.

Fixes #2379

🤖 Generated with Claude Code

Copilot

Pull request overview

This PR fixes a dimensional grain mismatch in dim_discount that could cause tfact_order to fan out when joining discounts, by ensuring dim_discount emits exactly one row per (source_discount_id, platform_code) and aligning the surrogate key with that grain.

Changes:

Dedupes dim_discount to one row per (platform_code, source_discount_id), keeping the most recently updated record.
Updates discount_pk generation to hash only (source_discount_id, platform_code) to match documented/consumed grain.
Clarifies discount_code documentation to reflect that it may represent the latest code on file.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/ol_dbt/models/dimensional/dim_discount.sql	Adds a dedupe step and adjusts surrogate key generation to prevent downstream fact fan-out.
src/ol_dbt/models/dimensional/_dim__models.yml	Updates `discount_code` column description to match the new deduped “latest code” behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…test Add discount_code as a final ORDER BY tie-breaker in the dedup row_number() so the surviving row is deterministic even when updated_on/created_on are identical (or both null) across duplicate source rows for the same (source_discount_id, platform_code) — previously the pick could vary between runs. Also add an explicit dbt_utils.unique_combination_of_columns test on (source_discount_id, platform_code), the now-documented and consumed grain, so future grain drift is caught directly rather than only via the downstream discount_pk uniqueness test. Addresses review feedback on #2411 from Copilot.

…discount_fk dim_discount is materialized='table' (full-refresh every run) but tfact_order is incremental with delete+insert on order_key, not discount_fk. Changing discount_pk's hash inputs (as this PR does) regenerates every discount_pk on the next dim_discount run, orphaning discount_fk on historical tfact_order rows until tfact_order is also full-refreshed. Document the required deploy step inline (matching the existing tfact_problem_events full-refresh note) so it isn't missed. Addresses review feedback on #2411 from sentry[bot].

…test Add discount_code as a final ORDER BY tie-breaker in the dedup row_number() so the surviving row is deterministic even when updated_on/created_on are identical (or both null) across duplicate source rows for the same (source_discount_id, platform_code) — previously the pick could vary between runs. Also add an explicit dbt_utils.unique_combination_of_columns test on (source_discount_id, platform_code), the now-documented and consumed grain, so future grain drift is caught directly rather than only via the downstream discount_pk uniqueness test. Addresses review feedback on #2411 from Copilot.

…discount_fk dim_discount is materialized='table' (full-refresh every run) but tfact_order is incremental with delete+insert on order_key, not discount_fk. Changing discount_pk's hash inputs (as this PR does) regenerates every discount_pk on the next dim_discount run, orphaning discount_fk on historical tfact_order rows until tfact_order is also full-refreshed. Document the required deploy step inline (matching the existing tfact_problem_events full-refresh note) so it isn't missed. Addresses review feedback on #2411 from sentry[bot].

github-actions · 2026-07-11T00:56:06Z

🔎 ol-dbt impact — column-level blast radius

✅ No column-level downstream impact detected for the changed models.

_{Posted by ol-dbt impact (annotate-only — does not block merge).}

…test Add discount_code as a final ORDER BY tie-breaker in the dedup row_number() so the surviving row is deterministic even when updated_on/created_on are identical (or both null) across duplicate source rows for the same (source_discount_id, platform_code) — previously the pick could vary between runs. Also add an explicit dbt_utils.unique_combination_of_columns test on (source_discount_id, platform_code), the now-documented and consumed grain, so future grain drift is caught directly rather than only via the downstream discount_pk uniqueness test. Addresses review feedback on #2411 from Copilot.

…discount_fk dim_discount is materialized='table' (full-refresh every run) but tfact_order is incremental with delete+insert on order_key, not discount_fk. Changing discount_pk's hash inputs (as this PR does) regenerates every discount_pk on the next dim_discount run, orphaning discount_fk on historical tfact_order rows until tfact_order is also full-refreshed. Document the required deploy step inline (matching the existing tfact_problem_events full-refresh note) so it isn't missed. Addresses review feedback on #2411 from sentry[bot].

… fan-out dim_discount hashed discount_pk from (source_discount_id, discount_code, platform_code), but the docs and tfact_order's join both assume (source_discount_id, platform_code) is the unique grain. When a discount's code changes over its lifetime, that assumption broke: dim_discount emitted multiple rows per (source_discount_id, platform_code), tfact_order's join fanned out, and the final dedup picked an arbitrary discount_fk since order_updated_on was identical across the fanned rows. Dedupe dim_discount to one row per (source_discount_id, platform_code), keeping the most recently updated discount_code, and hash the surrogate key from just those two columns. Fixes #2379

…test Add discount_code as a final ORDER BY tie-breaker in the dedup row_number() so the surviving row is deterministic even when updated_on/created_on are identical (or both null) across duplicate source rows for the same (source_discount_id, platform_code) — previously the pick could vary between runs. Also add an explicit dbt_utils.unique_combination_of_columns test on (source_discount_id, platform_code), the now-documented and consumed grain, so future grain drift is caught directly rather than only via the downstream discount_pk uniqueness test. Addresses review feedback on #2411 from Copilot.

…discount_fk dim_discount is materialized='table' (full-refresh every run) but tfact_order is incremental with delete+insert on order_key, not discount_fk. Changing discount_pk's hash inputs (as this PR does) regenerates every discount_pk on the next dim_discount run, orphaning discount_fk on historical tfact_order rows until tfact_order is also full-refreshed. Document the required deploy step inline (matching the existing tfact_problem_events full-refresh note) so it isn't missed. Addresses review feedback on #2411 from sentry[bot].

… coupons mitxpro b2b bulk-purchase coupons have a null coupon_id (see int__mitxpro__ecommerce_allcoupons), so every b2b row got source_discount_id = NULL. The new dedup CTE partitions by (platform_code, source_discount_id), and SQL groups all NULLs into one partition, so _row_num = 1 kept only a single b2b coupon and silently dropped the rest. Coalesce to b2bcoupon_id (negated to avoid colliding with the disjoint coupon_id numeric range) so each b2b coupon keeps a distinct source_discount_id.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 9, 2026 12:05

Copilot started reviewing on behalf of blarghmatey July 9, 2026 12:05 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread src/ol_dbt/models/dimensional/dim_discount.sql

Comment thread src/ol_dbt/models/dimensional/_dim__models.yml

sentry Bot reviewed Jul 9, 2026

View reviewed changes

Comment thread src/ol_dbt/models/dimensional/dim_discount.sql

sentry Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread src/ol_dbt/models/dimensional/dim_discount.sql

blarghmatey force-pushed the worktree-dbt-warehouse-audit branch from 4d48985 to 2ff33e3 Compare July 11, 2026 00:55

blarghmatey force-pushed the worktree-dbt-warehouse-audit branch from 2ff33e3 to 943dec6 Compare July 16, 2026 14:06

blarghmatey mentioned this pull request Jul 16, 2026

fix(ol_dbt_cli): sql_parser — don't collapse a macro-as-function-argument into a CTE #2456

Merged

5 tasks

blarghmatey added 4 commits July 16, 2026 11:27

blarghmatey force-pushed the worktree-dbt-warehouse-audit branch from 0fe8271 to 6450936 Compare July 16, 2026 15:28

blarghmatey requested a review from rachellougee July 16, 2026 17:50

blarghmatey requested a balanced review from Copilot August 18, 2026 14:12

Copilot started reviewing on behalf of blarghmatey August 18, 2026 14:13 View session

Copilot AI reviewed Aug 18, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix(dimensional): fix dim_discount grain mismatch causing tfact_order fan-out - #2411

fix(dimensional): fix dim_discount grain mismatch causing tfact_order fan-out#2411
blarghmatey wants to merge 4 commits into
mainfrom
worktree-dbt-warehouse-audit

blarghmatey commented Jul 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

blarghmatey commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Deployment notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 11, 2026

🔎 ol-dbt impact — column-level blast radius

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blarghmatey commented Jul 9, 2026 •

edited

Loading