fix(ol-dbt-cli): make composite --primary-key work in ol-dbt diff

[quazi-h]

What are the relevant tickets?

No GitHub issue — this was tracked in the internal task graph (tk-ol-dbt-diff-composite-primary-key-generates-inva-617789, tk-ol-dbt-diff-k-a-b-c-space-separated-truncates-th-65117e).

Context, not closed by this PR: discovered while validating #2088 (migrate marts__micromasters_dedp_exam_grades to the dimensional layer). That migration needs a trustworthy diff on the mart's real grain, which is what this unblocks.

Description (What does it do?)

Makes composite --primary-key work in ol-dbt diff. Passing the flag more than once previously died with a SQL parser error:

Parser Error: syntax error at or near "["
LINE 14: coalesce(a_query.['user_micromasters_email', 'user_mitxonline_email', 'proct...

Root cause. audit_helper takes primary_key as an opaque string it interpolates straight into SQL, never as a list. The Jinja list literal we were building reached the database as the literal text ['k1', 'k2']. Two call sites were affected, and they need different forms:

Consumer	Uses primary_key as	Fix
compare_column_values	a_query.{{ pk }} = b_query.{{ pk }} — a scalar column present in both queries	Collapse the composite key into one hashed join column via the new diff_composite_key macro inside the a_query/b_query blocks, and pass that column's name
compare_queries (via compare_relations)	interpolated into order by on the summarize=false branch	Render comma-joined (primary_key='k1, k2')

The second one is worth flagging for reviewers: the bug report assumed compare_relations was fine. It isn't. primary_key is unused when summarize=true (which is why row counts looked healthy), but the summarize=false sample path renders order by ['k1', 'k2'], in_a desc. Same parser error, only reached once a diff actually has unmatched rows to sample — so it would have come back as a separate bug report later.

Second commit accepts a comma-separated key. Cyclopts consumes exactly one token per flag occurrence, so -k a -k b -k c was the only way to express a composite key, and the two forms people reach for first both failed with errors that never mentioned primary keys:

• -k a b c → Cannot specify token "b" positionally for parameter dbt-dir, or with old/new positional, dbt project not found at <repo>/b — the stray token is absorbed as dbt_dir_path and the key list silently truncates to one column.
• -k "a,b,c" → Invalid primary key: 'a,b,c' (correctly loud, since _IDENTIFIER_RE rejects the comma, but it turns away a form most CLIs accept).

_split_columns() now flattens both repeated and comma-separated arguments for --primary-key and --exclude-columns, so -k a,b,c ≡ -k a -k b -k c. Order preserved, duplicates dropped (a repeated key column would otherwise be emitted twice into the surrogate-key expression), empty segments discarded rather than surfacing as an empty-identifier error.

-k a b c still cannot be made to work — that's cyclopts' arity, not ours — so the help text, docstring, and docs now say so explicitly rather than leaving people to hit the --dbt-dir error.

Why this matters beyond the crash. The correct grain for many marts is composite, and the single-column fallback the tool forced isn't a lesser answer, it's a misleading one. Same comparison, same data, 20,908-row mart:

Key	Result
user_micromasters_email alone	65,000+ mismatches across 8 columns, 7,826 rows reported "missing from old"
the real 3-column grain	1 differing column (semester, 8,891 rows), nothing missing on either side

All of that first row is join artifact from many-to-many pairing on a non-unique key.

How can this be tested?

Reproduce the original failure on main, then confirm it's fixed. Requires a local DuckDB (dev_local) with both relations materialized:

# On main: fails with `Parser Error: syntax error at or near "["`
ol-dbt diff --target dev_local \
  --old marts__micromasters_dedp_exam_grades_baseline --old-raw \
  --new marts__micromasters_dedp_exam_grades \
  --primary-key user_micromasters_email \
  --primary-key user_mitxonline_email \
  --primary-key proctoredexamgrade_created_on

# On this branch: completes. The comma form is equivalent.
ol-dbt diff --target dev_local \
  --old marts__micromasters_dedp_exam_grades_baseline --old-raw \
  --new marts__micromasters_dedp_exam_grades \
  -k user_micromasters_email,user_mitxonline_email,proctoredexamgrade_created_on

Both forms were verified to produce byte-identical output:

primary_key      : ['user_micromasters_email', 'user_mitxonline_email', 'proctoredexamgrade_created_on']
row_counts       : {'old': 20908, 'new': 20908, 'delta': 0}
column_mismatches: [('semester', 8891, 0, 0)]     # (column, mismatched, missing_in_old, missing_in_new)
verdict          : mismatch

Note this exercises both fixed paths — the 20 rendered sample-mismatch rows mean the summarize=false order by path ran too, not just the per-column path.

Automated coverage:

cd src/ol_dbt_cli && uv run pytest -q          # 476 passed
uv run pre-commit run --files <changed files>  # ruff format, ruff check, mypy, detect-secrets

16 tests added/rewritten. Note that test_compare_relations_composite_pk was asserting the buggy primary_key=['k1', 'k2'] output, so it was replaced rather than kept. New tests include one asserting the comma and repeated forms emit identical SQL.

Additional Context

Docs. docs/specs/DBT_WAREHOUSE_CI_QA_SPEC.md §3.4 step 3 said the key is simply "passed to compare_relations as the join key" — that's the assumption that produced this bug, so it's corrected to record the real audit_helper contract. src/ol_dbt/README.md gets a "Choosing the primary key" section with the numbers above and how to find a model's real grain (its dbt_expectations_expect_compound_columns_to_be_unique test). Both ol-dbt-* skill docs updated.

Two judgment calls worth a reviewer's opinion:

1. Comma-splitting was applied to --exclude-columns as well as --primary-key. Identical declaration, identical trap, and leaving the two inconsistent seemed worse than the small scope increase — but it is scope creep, so say if you'd rather it were dropped.
2. I did not make --dbt-dir keyword-only, which would stop a stray token being absorbed as a path. It changes the CLI's positional contract for a case that already errors rather than silently misbehaving, so it didn't seem worth the blast radius. Reconsider if anyone actually trips on it.

Known asymmetry, filed as follow-up (tk-ol-dbt-diff-single-column-primary-key-drops-null-1d1a40, p3): generate_surrogate_key coalesces NULLs to a sentinel, so the composite path pairs rows with NULL key components while the single-column path (a plain equi-join, unchanged here) does not. Not a wrong answer on a well-formed key — a primary key column shouldn't be nullable — but the two paths now differ, and that's worth tidying separately rather than expanding this PR.

Scope note: no dbt models are touched. The change is CLI + docs + one new dbt macro (src/ol_dbt/macros/diff_composite_key.sql).

---

Review round 1 — Copilot feedback addressed in 52b3c49

All three comments were valid; each was verified before changing anything.

1. dbt_utils.generate_surrogate_key is unsafe as a join key. It joins components with a literal - before hashing without encoding boundaries. Reproduced on the pinned dbt_utils 1.3.3 / DuckDB — ('a-b', 'c') and ('a', 'b-c') both hash to 7b193b3d3318446..., so two distinct keys would pair as one row and reintroduce the exact mispairing this PR prevents.

Replaced with a new diff_composite_key macro that length-prefixes each component as <len>:<value> (NULL as ~). Reading digits to the first : gives the length and the next <len> characters are the value, so nothing inside a value can shift a boundary; it stays injective whether the adapter's length() counts bytes or characters, since both sides of a comparison are computed by the same engine.

It's a project macro rather than inline SQL because it needs dbt.concat/dbt.hash/dbt.length — those dispatch per adapter (Trino needs to_hex(md5(to_utf8(..))), StarRocks has no ||) and Jinja doesn't re-render inside string literals, so they can't be nested into generated SQL.

Verified in DuckDB: the colliding pair now yields distinct keys (ea478e8e... vs 77550cbf...), identical tuples still match, and NULL, '', '~' and '1:x' — chosen to attack the new encoding — all stay distinct. End-to-end results on the repro are unchanged, so the swap is behaviour-preserving on real data.

2. The grain-lookup guidance named a test spelling that doesn't exist. Model YAML uses the dotted dbt_expectations.expect_compound_columns_to_be_unique (230 occurrences); the underscored form has 0 occurrences — it only appears in compiled test names, which is where I picked it up. Anyone following the original guidance would have grepped for a string that isn't in the project. Corrected, with a note on why the two spellings differ.

3. unique alone doesn't make a single column safe. dbt's unique ignores NULLs, so a nullable column can pass it while the single-column path's plain a.k = b.k never pairs NULL to NULL — those rows inflate the "missing from" buckets on both sides. Docs now require both unique and not_null.

[github-actions]

🔎 ol-dbt impact — column-level blast radius

0 breaking, 0 warning, 1 info across 1 changed model(s).

Details

• ℹ️ macro: diff_composite_key.sql → 0 downstream model(s)

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

[Copilot]

Pull request overview

Adds composite primary-key support to ol-dbt diff.

Changes:

• Supports repeated and comma-separated column arguments.
• Generates composite join keys and valid audit-helper SQL.
• Expands tests and usage documentation.

Reviewed changes

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

Show a summary per file

File	Description
src/ol_dbt/README.md	Documents composite keys.
src/ol_dbt_cli/tests/test_diff.py	Tests parsing and SQL generation.
src/ol_dbt_cli/ol_dbt_cli/commands/diff.py	Implements composite-key handling.
skills/data/ol-dbt-local-dev/SKILL.md	Updates local-development guidance.
skills/data/ol-dbt-fast-validation/SKILL.md	Updates validation guidance.
docs/specs/DBT_WAREHOUSE_CI_QA_SPEC.md	Records audit-helper integration behavior.

Suppressed comments (1)

src/ol_dbt_cli/ol_dbt_cli/commands/diff.py:470

• This alias is not collision-proof. If the relation already has a non-key column named ol_dbt_diff_surrogate_key, the generated query selects both the synthetic key and that real column under the same name; compare_column_values will then either fail with an ambiguous reference or compare the synthetic key instead of the requested real column. Derive an alias that does not match the key or current comparison column.

        select_cols = f"{key_expr} as {_SURROGATE_PK}, {column}"

---

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

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

docs/specs/DBT_WAREHOUSE_CI_QA_SPEC.md:176

• This claimed distinction is incorrect: persisted *_pk hashes are also used to pair rows, so a boundary collision can silently join distinct natural keys just as it can in the diff. A unique test catches the problem only when both colliding tuples coexist in that tested relation and the test runs; it does not make generate_surrogate_key collision-safe. Please avoid documenting those joins as protected from mispairing.

     This caveat is specific to using a hash as a **row-pairing join key across two
     relations**. It is not a claim about the `*_pk` surrogates in the dimensional models,
     which legitimately use `generate_surrogate_key`: there both sides of a join compute the
     hash identically, and a boundary collision would make two rows share a `*_pk` and fail
     that column's `unique` test rather than silently mispair.