#162 The consolidation models delete their scope, not the batch's keys (#154)

[Anonymous]

Originally created by: grynn-in

Closes [#154].

What was broken

The four consolidation models were incremental with delete+insert (A2, [#116]). That strategy deletes only the target rows whose key appears in the new batch. A key the SELECT no longer produces is never deleted. That covers an ownership window ending, a currency that stops resolving, a method moving to equity/none, or an entity leaving the tree. So its last consolidated rows stay in gold forever, and feed every downstream model. A2's claim that "no vars ⇒ byte-for-byte equal to a full build" was false for exactly those keys.

It was already live on the local stack. The incrementally maintained gold_fully_consolidated_tb held 1636 rows, while its own SQL on --full-refresh produces 1628. The 8 extra rows came from keys that had since left the SELECT.

Change

• gold_consolidated_trial_balance, gold_cash_flow_indirect, gold_ytd_trial_balance:
• incremental_strategy='append', with a pre_hook that runs DELETE FROM {{ this }} WHERE 1 = 1 {{ period_filter() }} {{ scope_filter() }}: the same predicate the SELECT applies. YTD uses period_filter(include_period=false), the whole closed year.
• With no vars, the whole table is replaced. A scoped close still touches only its own slice.
• The hook runs only when is_incremental().
• gold_fully_consolidated_tb: it has no filters of its own; its SELECT always reads every upstream row (#124). So it becomes a table, rebuilt and swapped each run.
• DBT-HANDOFF.md: a note on the change, and on the A2 claim it corrects.

Limit: an entity removed from the group tree falls outside scope_filter, so a scoped run can't see it. The next unscoped build clears it.

Live proof

Local stack. Main's and this branch's dbt_project were copied outside the bind mount. ClickHouse 24.8, dbt-clickhouse 1.9.3. Stale keys were planted that no SELECT can produce: entity ZZ154, and AMDE in a fake group ZZ154, for 2024 P11 and P12.

run	stale keys left (CTB / cash flow / YTD / fully consolidated)	other rows
planted	2 / 2 / 2 / 2	1519 / 260 / 1185 / 1636
main, unscoped	2 / 2 / 2 / 2: all survive	unchanged
this PR, scoped to AMDE 2024-12	1 / 2 / 2 / 1: only the in-scope P12 key goes; the others are out of scope	1519 / 260 / 1185 / 1628
this PR, unscoped	0 / 0 / 0 / 0	1519 / 260 / 1185 / 1628
this PR, unscoped again	0 / 0 / 0 / 0 (idempotent)	same
main's own SQL, --full-refresh	—	1519 / 260 / 1185 / 1628

A2/A3 slice tests still pass. After a scoped close for DEMF/2024: assert_incremental_slice_preserved (USMF preserved) and assert_scoped_cash_flow_ytd_confined both PASS. dbt parse is clean.

The live tables were restored with main's project afterwards, since the bind-mounted project stays on main until this merges.

Not added: the issue's suggested test, that the model holds no key its inputs no longer produce. It would restate the model's inclusion logic in a second place. The live proof above is the check; say if you want the test as well.

🤖 Generated with Claude Code

https://claude.ai/code/session_013WewQKFQgG7o2M3mUDPRR5

[Anonymous]

Originally posted by: grynn-in

Review round (ac3f4f2, plus a comment fix)

The review found no blockers. It verified the adapter details:

• the pre-hook runs before the insert
• is_incremental() is false on the first build and on --full-refresh
• an append with no unique_key inserts straight into {{ this }}
• lightweight DELETE is synchronous: the adapter forces mutations_sync=2, and the server default is lightweight_deletes_sync=2
• the scope subquery is allowed in the DELETE
• the scoped DELETE and the scoped SELECT cover the same entities across every group

Should-fix 1 is accepted and documented, not changed: a failed run leaves its slice empty. The pre-hook's DELETE commits before the SELECT. The adapter's delete+insert used to build the new rows first and delete after. So if the INSERT…SELECT fails now, the slice stays empty until the next successful run; unscoped, that's the whole table. I chose this over a custom materialization. The old failure mode was silent: stale rows forever, as the 8 rows found here show. The new one is loud: the build fails, and the next run restores the slice.

Should-fix 2 and minors 4–5 are recorded in DBT-HANDOFF.md:

• The empty window is now longer.
• The plain-append path skips on_schema_change, so a new column needs --full-refresh.
• The DELETE has no ON CLUSTER, which matters only for the cluster target, which isn't live.

Minor 3 is fixed: the two tests' comments now describe the scope delete.

Minor 6 is pre-existing, and I left it alone: full_refresh combined with scope vars rebuilds the table from the scoped slice only.

🤖 Generated with Claude Code

https://claude.ai/code/session_013WewQKFQgG7o2M3mUDPRR5

[Anonymous]

Originally posted by: grynn-in

Docs re-review: no blockers. Its two comment nits are fixed in the last commit: the slice test's comment now says gold_fully_consolidated_tb is a table again, and the handoff has the missing blank line. That commit is comment-only, so no third review round. Merging as a squash, so the intermediate commit with garbled comments doesn't reach main.

🤖 Generated with Claude Code

[Anonymous]

Ticket changed by: grynn-in

• status: open --> closed