Skip to content

feat(#2104): migrate engagement_problem_completion_summary to the dimensional layer - #2686

Open
quazi-h wants to merge 1 commit into
mainfrom
feat/2104-problem-completion-summary-dimensional
Open

quazi-h wants to merge 1 commit into
mainfrom
feat/2104-problem-completion-summary-dimensional

Conversation

@quazi-h

@quazi-h quazi-h commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

What are the relevant tickets?

Closes #2104 (epic #2072)

Description (What does it do?)

engagement_problem_completion_summary built its course-title lookup directly off stg__mitxonline__app__postgres__courses_courserun and ..._courses_course, bypassing both the intermediate and dimensional layers. It now reads dim_course_run joined to dim_course on course_fk = course_pk, so the model references only dim_*/afact_* models. Layering baseline shrinks 198 → 196.

Two things in the diff worth knowing. The lookup keeps a platform = 'mitxonline' filter, because the staging pair read the MITx Online course tables only — course_title has always been null for every other platform's course run here, and without the filter 1,865 more course runs (1,321 mitxpro, 492 edxorg, 52 bootcamps) would suddenly get titles. Both dimension CTEs also carry the SCD2 fan-out guard from dim_product.sql; the staging version leaned on course_id being a primary key to get exactly one title per run, and a bare group by doesn't reproduce that bound.

How can this be tested?

Run from the repo root on this PR's branch (feat/2104-problem-completion-summary-dimensional). Needs AWS credentials.

Everything below uses a throwaway DuckDB under /tmp. ~/.ol-dbt/local.duckdb is shared by every worktree on the machine, and a locally-built dim_course_run/dim_course left there by another session shadows the Glue view and silently skews the diff — that happened while validating this PR and produced a false MISMATCH.

1. Register sources and build both sides.

export ISO=/tmp/iso-2686 REAL_HOME="$HOME"
mkdir -p $ISO/.ol-dbt
export HOME=$ISO AWS_CONFIG_FILE=$REAL_HOME/.aws/config \
       AWS_SHARED_CREDENTIALS_FILE=$REAL_HOME/.aws/credentials \
       AWS_REGION=us-east-1 AWS_DEFAULT_REGION=us-east-1

# --duckdb-path is required; HOME alone does not redirect it
for db in staging intermediate dimensional; do
  uv run --frozen ol-dbt local register --database "ol_warehouse_production_$db" \
    --duckdb-path $ISO/.ol-dbt/local.duckdb --quiet
done

M="$(git rev-parse --show-toplevel)/src/ol_dbt/models/reporting/engagement_problem_completion_summary"
git fetch origin main -q && git show origin/main:src/ol_dbt/models/reporting/engagement_problem_completion_summary.sql > "${M}_pre.sql"

DBT_PROFILES_DIR=src/ol_dbt uv run --frozen dbt run --project-dir src/ol_dbt \
  --select "engagement_problem_completion_summary engagement_problem_completion_summary_pre" \
  -t dev_local --full-refresh

Expect: 276, 159, 58 tables registered, Errors: 0 each. Then PASS=3 ERROR=0 SKIP=0, with every ref() logging Falling back to Glue view.

2. Test the changed model.

DBT_PROFILES_DIR=src/ol_dbt uv run --frozen dbt test --project-dir src/ol_dbt \
  --select "engagement_problem_completion_summary engagement_problem_completion_summary_pre" \
  -t dev_local --indirect-selection=cautious

Expect: PASS=2 WARN=0 ERROR=0 — the compound-uniqueness test.

3. Diff old against new.

uv run --frozen ol-dbt diff \
  --old engagement_problem_completion_summary_pre \
  --new engagement_problem_completion_summary \
  -k user_email,course_title,courserun_readable_id,subsection_title,subsection_block_index,chapter_block_fk

Expect: MATCH, exit 0.

Row counts: ..._pre=3377077  ...=3377077  Δ=0
Summary: match — row Δ 0, 0 unmatched row-side(s), 0 column value mismatch(es).

Negative control — copy the built model, mutate one course run's course_title, and diff that with --old-raw. Expect MISMATCH, exit 1 (I get 28,420 unmatched sides from 14,210 altered rows).

4. Clean up.

rm -f "${M}_pre.sql" && rm -rf $ISO && export HOME="$REAL_HOME"

Expect: git status clean.

Note for other #2072 migrations: dim_course/dim_course_run cannot be built on dev_local at all — dim_course hits Binder Error: Cardinality can only operate on MAPs on Trino-only cardinality(), and three ancestors use Trino-only JSON SQL. Both sides above read them as Glue views, which is why the registration has to be frozen for the comparison to mean anything.

🤖 Generated with Claude Code

…ensional layer

Replace the `(course_title, courserun_readable_id)` lookup that read
`stg__mitxonline__app__postgres__courses_courserun` inner-joined to
`stg__mitxonline__app__postgres__courses_course` with `dim_course_run` joined
to `dim_course` on `course_fk = course_pk`. This model bypassed both the
intermediate and dimensional layers; it now references only `dim_*`/`afact_*`.

The lookup is restricted to `platform = 'mitxonline'`. The staging pair it
replaced read the MITx Online course tables only, so `course_title` has always
been null for every other platform's course run in this report. Dropping the
filter would newly populate 1,865 more course runs (1,321 mitxpro, 492 edxorg,
52 bootcamps) — a data change rather than a layer migration.

Both dimension CTEs carry the SCD2 expiration-gap guard documented in
dim_product.sql. The staging version relied on `course_id` being the primary key
of courses_course to yield exactly one title per course run, and grouped to
dedupe; the `row_number()` guards are what preserve that bound here.

Verified by building the pre- and post-migration models locally from one Glue
registration in a single `dbt run`, so both sides read byte-identical sources:
3,369,327 rows both sides (delta 0, distinct = total), whole-row symmetric
`EXCEPT ALL` across all 9 columns returns 0 rows in both directions, and every
column is 0/0 with identical fill rates. `course_title` is 35.62% populated on
both sides, so its 0/0 is real verification rather than a null-to-null artifact.
The comparison was negative-controlled: nulling the column, mutating one course
run's title, and dropping a single row out of 3.37M are all detected.

Shrinks the #2072 layering baseline from 198 to 196.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 15, 2026 20:05
@github-actions

Copy link
Copy Markdown

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The migration satisfies the layering requirements and preserves the lookup’s cardinality and platform scope.

Pull request overview

Migrates the problem-completion summary to the dimensional layer while preserving MITx Online title behavior.

Changes:

  • Replaces staging course lookups with guarded dim_course_run and dim_course lookups.
  • Removes the resolved layering violations from the baseline.
File summaries
File Description
engagement_problem_completion_summary.sql Uses dimensional course models with SCD2 deduplication.
dimensional_layering_baseline.txt Removes two obsolete staging dependencies.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

quazi-h added a commit that referenced this pull request Sep 16, 2026
Both from a real end-to-end run of this procedure against #2686 in another
session — the first time it has been exercised on a live migration rather
than reasoned about. Verified both here before writing.

THE +<upstream> INSTRUCTION IS UNFOLLOWABLE FOR THE COMMONEST TARGET.
`+dim_course_run` / `+dim_course` cannot build on dev_local:
dim_course_run.sql:191 calls `regexp_like` raw instead of through the
cross-db macro, and that is a Trino builtin DuckDB lacks (confirmed in the
file; tracked as tk-t1-unblock-local-validation-...-20b1b3). The #2686 run
measured ERROR=3 SKIP=5 with the model under test among the skipped,
leaving only the _pre side. New subsection: reconstruct the needed slice
from buildable ancestors, state that this verifies the derivation rather
than the built relation, and do NOT substitute a Glue view for the
unbuildable ancestor — that is the uncontrolled divergent-path case.

VERIFY FROM run_results.json, NOT THE LOG. A selected model can be skipped
while the command looks fine. Two mechanisms, both reproduced here:
`| tail` hides the per-model ERROR/SKIP lines behind dbt's deprecation
summary, and `| tail` also masks the exit status — bare run exits 1, piped
exits 0, piped under `set -o pipefail` exits 1. Noted that
${PIPESTATUS[0]} is a bash-ism and empty in zsh, where it is
${pipestatus[1]}.

One correction to the reported finding: dbt itself does exit 1 when a model
errors — I could not reproduce "dbt run exited 0". The 0 comes from the
pipe, so the fix is pipefail plus the artifact check rather than anything
about dbt's exit behaviour.

The artifact snippet is scoped to {error, fail, skipped, runtime error}
rather than != success, because test results use pass/warn and the naive
form reports every passing test as a failure. Verified both ways: none on a
real test-run artifact, and it catches error+skipped on a model run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
quazi-h added a commit that referenced this pull request Sep 16, 2026
…nd the stale-registration evidence

CORRECTION to d6806b0. I wrote that `+dim_course_run` fails because
dim_course_run.sql:191 calls `regexp_like` raw. That is false on current
main: PR #2658 dispatched it and merged 2026-09-11, and the dispatched form
is present in this branch's merge-base too. I read the raw version out of
the main checkout, which is 35 commits behind origin/main (153a76e vs
bb2bcfc) — I verified the claim against a stale tree, which is the same
class of mistake as testing a different command from the one I shipped.

The real blockers are three ANCESTORS carrying Trino-only JSON SQL, per
tk-three-trino-only-json-models-block-building-dim--59c2db and confirmed
against origin/main: stg__edxorg__api__course.sql:20 and
stg__mitxpro__app__postgres__cms_certificatepage.sql:17 (`json_query(...
with array wrapper)` wrapped in json_parse and cast to array), and
int__mitxpro__coursesfaculty.sql:9 (`cast(json_parse(...) as array (json))`).
The cascade is what makes it total: those error, int__edxorg__mitx_courseruns
and int__mitxpro__courses skip, and since the dims `union all` every platform
one broken branch skips the whole dim. Measured on #2686: PASS=48 ERROR=3
SKIP=5 TOTAL=56. Also noted #2658 explicitly so nobody hunts for the
regexp_like that is already gone.

Added the gate the #2686 run argued for, which is sharper than what I had:
both comparison sides must appear with status: success in run_results.json
before any number is measured -- not "the run finished", not "no errors
scrolled past".

Added the evidence for step 1's stability rule, which is the best argument
in the file for it: on #2686 a staging layer registered hours earlier read
4,513 rows where a fresh registration read 4,996, and that 483-row gap was
published and retracted. It was convincing because the stale side was
internally consistent — the _pre model and the built dim both came from it,
so the diff was a clean 0/0. A stale registration does not look stale, it
looks like agreement.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
quazi-h added a commit that referenced this pull request Sep 16, 2026
…nd state the scope up front

Folds in what two manual validation runs on #2403 and #2686 learned but the
skill did not yet say.

CAUTIOUS DROPS THE PR'S OWN TEST. `--indirect-selection=cautious` includes a
test only when EVERY model it references is selected, so a new singular test
the PR itself adds — referencing the changed model plus something outside the
selection — is silently excluded. On #2403 that was the PR's own new test, the
one test most worth seeing. Switched the recommendation to `buildable`, which
relaxes exactly that condition and still excludes the eager relationships
noise. Re-measured both modes rather than trusting the July figures: on
dim_course_run eager 17 / buildable 8 / cautious 8, and the single test
buildable adds is the course_fk -> dim_course relationship.

RUN THE TESTS BEFORE THE DIFF, with the reason: a named failing test is more
diagnostic than a 20k-row mismatch report, and a broken model makes a MATCH
uninterpretable — you cannot distinguish agreement from two identically-wrong
sides.

WHAT TO EXPECT was missing entirely — the skill said which figures to trust
but never what the report looks like or what a pass is. Step 5(d) now says to
diff EVERY changed model with a per-model verdict written down first:
unaffected -> MATCH/exit 0; intentionally changed -> MISMATCH/exit 1 where the
SHAPE is the assertion (row delta 0, only the intended columns, the intended
direction). So a MISMATCH is not a failure and a MATCH is not automatically a
pass. Includes the #2403 numbers that turned an asserted claim into a measured
one (semester 12 -> 99 non-null over 87 re-versioned rows, body had said
"~100"), and the --exclude-columns effective_date,end_date that SCD2 models
need or the report is unreadable.

Also rewrote the frontmatter description and the opening paragraph, which
undersold the skill as "the acceptance procedure" when it is the whole
register/build/test/diff workflow. Anyone scanning the description could not
tell whether it covered the higher-level sequence or just the diff command.

Two stale spots caught by grepping for what the change touched: "test
separately, cautiously" now misread as the mode name, and a "runs 7 tests"
figure that re-measurement put at 8.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
quazi-h added a commit that referenced this pull request Sep 16, 2026
Two gaps found by checking the skill against everything the #2403 and #2686
manual runs produced, rather than against the review findings.

NO NEGATIVE CONTROL. The skill said what a clean result looks like but never
how to prove the comparison can fail. A MATCH is unfalsifiable on its own —
it is indistinguishable from a diff that compared nothing. Added the recipe
(replace the column under test with a literal in _pre, rebuild that model
only, rerun) with the #2403 evidence that it does fail loudly: perturbing
semester gave `mismatch — 17888 unmatched row-side(s), 1 column value
mismatch(es)`, exit 1, rows listed. Also says to put the one-liner in the PR
body, since a reviewer cannot otherwise distinguish the two.

THE SHARED DUCKDB CAN STOP YOU, not just mislead you. The section covered
staleness and clobbering but not the hard failure: with another session
holding the file, ol-dbt diff dies on `_duckdb.IOException: Could not set
lock on file ... Conflicting lock is held` — 5 retries at ~20s on #2403.
Called out as contention rather than a defect in the change under review, so
nobody reports it as one.

Renumbered that list's intro from "Two consequences" to "Three", caught by
grepping what the edit touched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@quazi-h
quazi-h requested a balanced review from Copilot September 17, 2026 19:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The migration satisfies the layering requirements and preserves the existing lookup scope and cardinality.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

quazi-h added a commit that referenced this pull request Sep 18, 2026
…n PRs (#2659)

The two existing dbt skills drive the tools — `ol-dbt-local-dev` for
  register/build, `ol-dbt-fast-validation` for validate/impact/diff — but neither
  answers the question a migration PR turns on: is the data the same? This adds
  the end-to-end local validation workflow (register, build both sides, test,
  diff, decide), derived from validating #2403 and #2686.

  The load-bearing part is which local numbers are trustworthy. 651 of 667
  non-raw Glue views (97.6%) point at `__dbt_tmp` metadata locations, which
  return duplicated or partially-missing rows while the build succeeds — and the
  pollution is heterogeneous, from 1.00x to 61.85x across views in a single
  registration. So an absolute row count or fill rate read through a `glue__`
  view is not evidence, and several have already been quoted in PR bodies as
  though they were. A delta between two sides built locally from the SAME
  registration is evidence, because identical inputs cancel. The skill is
  organised around that distinction: register once, build both sides in one
  invocation, compare.

  The rules that cost the most to learn, each with the measurement behind it:

  - Re-register immediately before every build. A registry 60 minutes old 404'd
    mid-validation; 336 pointers moved over 26 hours (65% staging, 68%
    intermediate, 79% dimensional). And a stale registration does not look
    stale, it looks like agreement — on #2686 a stale staging layer read 4,513
    rows where a fresh one read 4,996, and because both comparison sides came
    from the stale side the diff was a clean 0/0. That 483-row gap was published
    and retracted.
  - `--full-refresh` whenever the model under test is incremental. An
    incremental run does execute the model SQL, but re-derives only what
    `is_incremental()` reselects; `dim_course_run` reported OK in 0.11s without
    re-deriving the column under test, for two months.
  - `~/.ol-dbt/local.duckdb` is shared by every worktree and session on the
    machine. A concurrent rebuild took `semester` from 4,513/4,513 populated to
    12/4,513, with no warning from either side.
  - `dbt run`, not `dbt build`. `+error_if: ">10"` makes the skip intermittent:
    4 failing rows WARN and the downstream model builds, 24 FAIL and it is
    skipped, leaving a comparison side that silently does not exist.
  - Test with `--indirect-selection=buildable`, on the two comparison models
    only. `eager` pulls in `relationships_*` tests owned by other models, which
    compare your local build against production Glue (17 tests vs 8 on
    dim_course_run); `cautious` drops the PR's own new test.
  - Take the join key from the model's own uniqueness test, including its `where`
    clause. Unfiltered, an SCD2 relation reads 6 rows / 5 distinct keys — a
    fan-out that is ordinary history; filtered, 3/3. And with an unproven key the
    per-column mismatch rate is the key-sensitive figure, not the unmatched-row
    count: two byte-identical 4-row tables with one repeated key report 0
    unmatched rows alongside a 33.33% column mismatch.
  - Negative-control the diff before believing a MATCH. Perturbing `semester` in
    the `_pre` side produced `mismatch — 17888 unmatched row-side(s)`, exit 1.
  - Gate the build on the registration log, not the exit status. `ol-dbt local
    register` exits 0 with failed tables, and never counts a Glue
    database-level failure or an empty layer at all, so `✗ Errors: 0` can print
    while an entire layer went unrefreshed.

  Alongside the new skill, `ol-dbt-local-dev` gains the freshness rules, the
  shared-DuckDB warning, and a PATH prerequisite (`ol-dbt` shells out to bare
  `dbt`, so a stray 1.8.1 earlier on PATH fails with "Could not find adapter type
  duckdb!"); its "prefer incremental" rule is reframed as iterating vs
  concluding, which is the distinction that actually decides it.
  `agent-config.toml` registers the skill and adds a `migration` profile.

  Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

Migrate engagement_problem_completion_summary to use dimensional layer

3 participants