Repository navigation
fix(snapshot): insert new keys and invalidate hard deletes without relying on join_use_nulls - #785
Open
alepietrobon wants to merge 2 commits into
Open
alepietrobon wants to merge 2 commits into
alepietrobon wants to merge 2 commits into
Conversation
…lying on join_use_nulls The staging SQL found new keys and removed keys with `left join ... where <right key> is null`. With ClickHouse's default join_use_nulls = 0 an unmatched row is filled with type defaults, so a non-nullable key is never NULL: new keys were only inserted when row_changed happened to be true for the default-filled row, and hard_deletes='invalidate' never closed anything. Use LEFT ANTI JOIN for both, plus an inner join for changed keys. Fixes ClickHouse#784 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
koletzilla
requested review from
koletzilla
and
a balanced review from Copilot
November 6, 2026 14:50
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block approval of the snapshot key-handling changes.
No open issues 🎉
What changed in this PR
Fixes snapshot handling of new and removed keys under ClickHouse’s default join_use_nulls = 0, addressing #784 and the hard-delete invalidation issue tracked in #662.
Changes:
- Uses anti joins to detect new and deleted keys, while retaining inner joins for changed rows.
- Adds regression coverage for timestamp and check strategies with single and compound keys, nullable columns, and repeat-run stability.
- Documents the fix in the changelog;
hard_deletes='new_record'remains out of scope.
| File | Description |
|---|---|
tests/integration/adapter/basic/test_snapshot_new_and_deleted_keys.py |
Adds four regression cases for new-key insertion and hard-delete invalidation. |
dbt/include/clickhouse/macros/materializations/snapshot.sql |
Replaces NULL-based unmatched-row detection with anti joins in both staging macros. |
CHANGELOG.md |
Records the user-visible snapshot fixes and links to the PR. |
Review effort: Balanced
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.
Summary
Fixes #784. Related: #662 (snapshot tracking issue), #291, #562.
After the first build, snapshots could silently skip keys that are new in the source, and
hard_deletes='invalidate'never closed keys removed from it. Every run still reported OK.Cause
Both staging macros (timestamp and check strategy) found these rows with the ANSI anti-join idiom:
With ClickHouse's default
join_use_nulls = 0, an unmatched LEFT JOIN row is filled with type defaults, not NULL. A non-nullable key becomes0/'', sois nullis never true:row_changedhappens to be true for the default-filled snapshot row:snapshotted_data.dbt_valid_from < source.updated_at. Ifupdated_atis non-nullable, the defaultdbt_valid_fromis1970-01-01and the comparison is true by accident. Ifupdated_atisNullable, the default is NULL and the new key is dropped.Nullablecheck columns.left join deletes_source_data ... where source_data.dbt_unique_key is null) never match a non-nullable key, so removed keys are never closed.This is why the existing tests passed: seeds load as non-nullable types (
Int32,DateTime, ...), soBaseSnapshotTimestamponly hits the accidental1970-01-01 < updated_atcase. No test coveredhard_deletes='invalidate'.In a real project on 1.10.3 (single
Stringkey,Nullable(DateTime)updated_at), a grown source ended with 4,920 of 9,775 current keys in the snapshot, and 0 of 4,855 removed keys were closed. With this fix: 9,775 current, 4,855 closed, none closed wrongly.Fix
Use
LEFT ANTI JOIN, which does not depend on NULL-filling. It keeps using the existingunique_key_join_onhelper, so single and listunique_keys (#751) both work:left anti join snapshotted_data, and changed keys from an inner join onrow_changed(the same joinupdatesalready uses). The two are combined withunion all.from snapshotted_data left anti join deletes_source_data. The right-sidesource_data.*columns are type defaults. They only keep the column layout for the positionalunion all; the merge reads onlydbt_change_type,dbt_scd_idanddbt_valid_tofrom a delete row, as before.Alternative considered: append
settings join_use_nulls = 1to the staging query. I decided against it. It is session-dependent, it interacts with the SETTINGS thatcreate_table_asemits, and it makes the right-hand columnsNullable, which changes the staging table's types. An anti join is correct under either setting.hard_deletes='new_record'(#562) is out of scope.Tests
New
tests/integration/adapter/basic/test_snapshot_new_and_deleted_keys.pyuses a seed with a non-nullable key andNullablename/updated_at(column_types). After the first snapshot it adds two keys (one with a NULL check column) and removes one. It then asserts the new keys are current, the removed key is closed, and a further run changes nothing. It covers timestamp and check strategies, each with a single and a compoundunique_key(4 classes).main: all 4 fail. The removed key is never closed; the timestamp strategy drops both new keys; the check strategy drops the one whose check column is NULL.tests/integration/adapter/basic/test_snapshot_*.py,grants,dbt_clone: 21 passed, 1 skipped (test_distributed_grants, not on a cluster) on ClickHouse 26.10.test_snapshot_*.pyalso passes on 26.3.DBT_CH_TEST_CLUSTER).make lintpasses.🤖 Generated with Claude Code
Note
Medium Risk
Touches core snapshot SQL generation for inserts and hard-delete invalidation, so incorrect joins could misclassify rows; mitigated by focused anti-join logic and new integration coverage but still impacts snapshot data correctness.
Overview
ClickHouse snapshot staging now finds brand-new keys and hard deletes using
LEFT ANTI JOINplus a targeted inner join, instead ofLEFT JOIN ... WHERE key IS NULL, which failed whenjoin_use_nullsstays at the default (0) and defaulted keys aren’t actuallyNULL.The
insertionsCTE now unions “purely new keys” with “existing keys whose monitored columns changed,” anddeletesunderhard_deletes='invalidate'now anti-joins snapshotted rows against the current source so removed keys reliably close even for non-nullable keys.CHANGELOG calls out that this fixes silently dropped new snapshot rows (especially with nullable
updated_at/ check columns) andinvalidatehard deletes never closing rows.A new integration suite
test_snapshot_new_and_deleted_keys.pyseeds nullable fields, adds/removes rows, and covers timestamp vs check strategies with scalar and compoundunique_keys to lock the regression in.Reviewed by Cursor Bugbot for commit 5ed3f66. Bugbot is set up for automated code reviews on this repo. Configure here.