fix(reporting): reconcile program_completion_days sign and dual definitions#2416
Open
blarghmatey wants to merge 2 commits into
Open
fix(reporting): reconcile program_completion_days sign and dual definitions#2416blarghmatey wants to merge 2 commits into
blarghmatey wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR resolves a metric correctness/consistency problem in the dbt warehouse by fixing the sign of program_completion_days at the source mart layer and removing a redundant, independently-derived definition in the reporting layer.
Changes:
- Corrected
marts__combined_program_enrollment_detail.program_completion_daysto compute certificate date minus first course start date (positive day counts). - Updated
program_enrollment_with_user_reportto sourceprogram_complete_daysdirectly from the mart’sprogram_completion_days, and removed the no-longer-neededfirst_courserun_start_on_dateaggregate from the report’scourses_detailCTE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/ol_dbt/models/marts/combined/marts__combined_program_enrollment_detail.sql | Fixes the date_diff operand order so program_completion_days matches the documented metric semantics. |
| src/ol_dbt/models/reporting/program_enrollment_with_user_report.sql | Eliminates a duplicate metric derivation by selecting program_completion_days from the upstream mart and dropping the unused supporting aggregate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… reporting model marts__combined_program_enrollment_detail computed program_completion_days as start-minus-certificate (negative). program_enrollment_with_user_report independently re-derived the same metric as program_complete_days with the correct direction but re-parsed from the raw certificate timestamp string instead of consuming the mart's field, creating two competing definitions. Swap the date_diff operands in the mart to certificate-minus-start (positive, matching both models' documented description), and have the reporting model select enrollment_detail.program_completion_days directly instead of re-deriving it from a differently-filtered join. Part of #2375 (2026-07 dbt warehouse audit). Witan task: tk-reconcile-fifth-competing-metric-definition-prog-3160aa. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
blarghmatey
force-pushed
the
worktree-program-completion-days-consolidate
branch
from
90c6157 to
f6e4e9b
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
91
to
97
| combined_users.user_address_postal_code, combined_users2.user_address_postal_code | ||
| ) as user_address_postal_code | ||
| , case when courses_detail.capstone_sum > 0 then 'Y' else 'N' end as capstone_ind | ||
| , date_diff( | ||
| 'day' | ||
| , courses_detail.first_courserun_start_on_date | ||
| , cast(substring(enrollment_detail.programcertificate_created_on, 1, 10) as date) | ||
| ) as program_complete_days | ||
| , enrollment_detail.program_completion_days as program_complete_days | ||
| from enrollment_detail | ||
| left join combined_users | ||
| on enrollment_detail.platform_name = '{{ var("edxorg") }}' |
There was a problem hiding this comment.
Bug: The calculation for first_courserun_start_on_date now incorrectly includes unverified enrollments, which alters the program_completion_days metric by using an earlier start date for some users.
Severity: MEDIUM
Suggested Fix
The reporting model should revert to its previous logic for calculating first_courserun_start_on_date. Instead of consuming the field from the mart, re-implement the logic that filters for courserunenrollment_enrollment_mode = 'verified' when determining the minimum course run start date. This will restore the original definition of the program_completion_days metric.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/ol_dbt/models/reporting/program_enrollment_with_user_report.sql#L91-L97
Potential issue: The reporting model `program_enrollment_with_user_report.sql` now
sources the `first_courserun_start_on_date` from a mart model. The previous
implementation calculated this date using only 'verified' enrollments. The mart model,
however, calculates this date based on all enrollment types, including unverified ones.
This change in logic means that for any user with an unverified enrollment that started
before their first verified one, the `program_completion_days` metric will be calculated
from this earlier date, leading to a significant and unintended change in the report's
output.
Did we get this right? 👍 / 👎 to inform future reviews.
…CTE overrun sql_parser.py's Jinja-macro-tail collapse used an unbounded lazy loop to find the next `) as alias` after a placeholder. A bare (unaliased) macro call repeated in a GROUP BY list has no real terminal, so the search ran past the GROUP BY, the CTE's closing paren, and the outer SELECT keyword before seizing on an unrelated downstream alias -- corrupting the parsed SQL and producing false yaml_sql_sync/broken_ref_columns errors on marts__combined_program_enrollment_detail (which blocked this PR's CI after rebasing onto the new dbt PR CI gate). Bound the continuation to 6 lines, which still covers the two existing orphaned-tail regression tests, and make the leading ')' before 'as' optional so a plain multi-line macro+alias (no stray tokens) resolves on the very next line instead of searching further.
🔎 ol-dbt impact — column-level blast radius✅ No column-level downstream impact detected for the changed models. Posted by |
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
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.
What
Two competing definitions of the same metric under near-identical names:
marts__combined_program_enrollment_detail.program_completion_dayscomputeddate_diff('day', certificate_date, course_start_date)— start minus certificate, negative. This is the same inverted-sign bug as Fix NULL-unsafe SCD2 change detection in dim_course_run and dim_product #2380's family, tracked separately as Make remaining tracking-log event facts incremental #2385 / fixed via fix(dbt): resolve 3 P0 identity/fan-out/sign bugs from warehouse audit #2400.program_enrollment_with_user_report.program_complete_daysindependently re-derived the same concept, correct direction (certificate minus start), but re-parsed the rawprogramcertificate_created_onstring and recomputedfirst_courserun_start_on_datefrom its own verified-enrollment join instead of consuming the mart's field.Fix
date_diffoperands in the mart soprogram_completion_daysis certificate-minus-start (positive), matching both models' documented description ("days between first course start and program completion").program_enrollment_with_user_reportnow selectsenrollment_detail.program_completion_daysdirectly instead of re-deriving it, and dropped the now-unusedfirst_courserun_start_on_dateaggregate fromcourses_detail.Part of #2375 (2026-07 dbt warehouse audit).
Closes #2387
🤖 Generated with Claude Code