snapshots: compute a day range in one set-based pass - #310
Conversation
calculate_snapshot_diff_for_day_range looped over every day in the range, emitting three statements per day and seeding the first day from the target's rows where valid_to IS NULL. That seed is the state as of *now*, not as of snapshot_start_date, which is why re-running a historical range could not reproduce it. SCD Type-2 valid_to is always the valid_from of the entity's next version, so it is exactly LEAD(valid_from) over that entity's ordered versions. No day in the range depends on the previous day's output, and the whole range chains with one window function. Statement count is now fixed at two regardless of range length. The one value the source cannot supply is each entity's version already open when the range started. It is now read from the target positionally -- the entity's latest version with valid_from before snapshot_start_date -- so the baseline is the state as of the start of the range. The redundant valid_to predicate on that read exists to prune the valid_to-partitioned target. Open versions are chained to new versions via PARTITION BY rather than a join, because the unique key may contain nulls (a trustline has either liquidity_pool_id or asset_code/asset_issuer set, never both) and PARTITION BY treats nulls as equal while = does not. Behaviour preserved: one version per entity per day with the last source row of that day winning; entities with no source activity in the range contribute nothing to the diff; open-version rows carry their full payload so the materialization's merge on (source_unique_key, valid_from) does not null out columns. Target rows are aligned to the source column list so a target that has drifted (on_schema_change: append_new_columns, or a source column not yet added to the target) still unions cleanly. Not addressed here, tracked separately: the diff still only inserts and updates, so a target row whose source row has since disappeared is not removed. Chunked batching and key-scoped (partial) refreshes are also follow-ups. calculate_snapshot_diff_for_day is left in place, now unreferenced, to serve as the reference implementation while equivalence fixtures are written.
There was a problem hiding this comment.
🟡 Not ready to approve
Historical partial ranges do not preserve the next target version as a boundary, potentially creating multiple open versions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Replaces per-day snapshot processing with a set-based range computation.
Changes:
- Builds daily versions in one query and chains them with
LEAD. - Adds historical range validation and documentation.
- Updates snapshot limitations.
File summaries
| File | Description |
|---|---|
macros/calculate_snapshot_diff_for_day_range.sql |
Implements set-based snapshot chaining. |
macros/calculate_snapshot_diff_for_day_range.yml |
Updates macro documentation. |
docs/snapshot.md |
Documents the new workflow and limitations. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| , lead({{ valid_from_col_name }}) over ( | ||
| partition by {{ unique_key_csv }} | ||
| order by {{ valid_from_col_name }} | ||
| ) as {{ valid_to_col_name }} |
…tures
Running the existing dummy_snapshot_incremental fixture against the new macro
surfaced two defects, both from only reading the pre-range version out of the
target.
1. A version whose valid_from falls exactly on snapshot_start_date was excluded
by the `valid_from < start` boundary predicate, so it never got re-closed.
Fixture account 2 has valid_from 2025-01-01 00:00:00 with a range starting
2025-01-01, and its open version was left open while a later version was
inserted in front of it.
2. When the target held a version newer than a source row in the range, the
source version was inserted without being chained, leaving two open versions
for the entity. Fixture account 3 covers this.
Both are fixed by also reading the target's in-range versions and chaining all
three sets -- source versions, the pre-range version, and in-range target
versions -- through one LEAD. A source version supersedes the target copy at the
same valid_from so the source payload wins and no duplicate valid_from reaches
the merge. Entities with no source activity in the range are still excluded
entirely, so they are left untouched.
Behaviour change, deliberate: case 2 previously dropped the source version.
docs/snapshot.md listed that as a known limitation ("slight chance of missing
history ... when a new version of record is already present") and the fixture
description recorded it as intended. The range is now rebuilt from every version
that belongs in it, so the earlier version is recovered rather than discarded --
which is the point of making re-runs reproducible. dummy_snapshot_incremental's
expected data gains that row.
New fixtures:
* dummy_snapshot_rerun -- re-running a range the snapshot already covers must
leave the target byte-identical. This is the property the day-by-day
implementation could not provide.
* dummy_snapshot_null_unique_key -- composite source_unique_key with nulls in a
key column, as trustlines_snapshot has. Matching such keys with = fails on the
null column and inserts a second open version instead of closing the existing
one; this asserts the version is closed.
Both need adding by name to stellar-dbt's ci.yml, which invokes
run_snapshot_test.sh per fixture, and to the dummy_snapshot exclusion lists in
its other workflows.
|
Pushed Two defects the existing
|
What
calculate_snapshot_diff_for_day_rangelooped over every day in the requested range, emitting three statements per day. This replaces the loop with a single set-based computation. Statement count is now fixed at two regardless of whether the range is one day or several years.The macro signature is unchanged, so
incremental_snapshot.sqlis untouched and no snapshot model changes are needed — the eight snapshots here and the three instellar-dbtare unaffected.Why it works
SCD Type-2
valid_tois always thevalid_fromof the entity's next version.calculate_snapshot_diff_for_daynever closes a row without a successor (set 3 requires a matching new source row), and it emits one version per entity per day with the last source row of that day winning. Sovalid_tois exactlyLEAD(valid_from)over that entity's ordered versions — no day in the range depends on the previous day's output, and the whole range chains with one window function.The correctness fix
The day loop seeded its first iteration from the target's rows
WHERE valid_to IS NULL. That is the state as of now, not as ofsnapshot_start_date, which is why re-running a historical range could not reproduce it (stellar/stellar-dbt#733).Each entity's version already open at the range start is now read from the target positionally — its latest version with
valid_frombeforesnapshot_start_date. The additionalvalid_to IS NULL OR valid_to >= startpredicate on that read is redundant for correctness and present only to prune thevalid_to-partitioned target.Two statements
temp_source_table— one row per entity per day in the range, withvalid_fromset.temp_target_table— those rows unioned with the versions open at the range start,valid_tochained across both sets at once. Open-version rows that receive novalid_tobelong to entities with no activity in the range and are dropped, so untouched entities contribute nothing to the diff.Notes for review
PARTITION BY, not a join. The unique key may contain nulls — a trustline has eitherliquidity_pool_idorasset_code/asset_issuerset, never both, andstg_trust_linesdoes not coalesce them.PARTITION BYtreats nulls as equal;=does not. Worth checking separately whether the existing equality joins incalculate_snapshot_diff_for_dayandmerge_source_into_targethave been duplicating versions for those keys — that is a live-data question, not answerable from the code.CAST(NULL AS <type>)for columns the target lacks. The per-day merge absorbed schema drift viadest_cols_csv; aUNION ALLdoes not. This matters becauseprocess_schema_changesadds new columns to the target only after the presql runs.(source_unique_key, valid_from)updates all columns on match.snapshot_start_date >= snapshot_end_date. Previously that produced an empty range silently.calculate_snapshot_diff_for_dayis deliberately left in place, now unreferenced, to serve as the reference implementation while equivalence fixtures are written. It is removed in a follow-up.Validation
macros/is in.sqlfluffignore, and both sqlfluff pre-commit hooks fail identically on untouched files in this repo (macOSsedinpre-commit/for_pre_commit.sh, andDBT_TARGETnot reaching dbt at hook time). All other hooks pass. Not yet run against BigQuery.Validated offline instead:
stg_trust_lines-shaped schema for the incremental, full-refresh, and inverted-range cases. This caught a real defect: a{#-- --#}comment stripped whitespace on both sides and gluedAS target_tabletoWHERE. Fixed.valid_to = LEAD(valid_from)holds across the result.Not addressed here
docs/snapshot.mdlimitations. Needs the rollback step — stellar/stellar-dbt#988.dummy_snapshot_*harness — both engines over one(source_data, target_data, expected_target)triple, plus genesis-recompute equals--full-refresh. These should land before this is trusted on a large snapshot.incremental_snapshot.sqlis a fork of dbt-bigquery's incremental materialization that has drifted from it — no grants, no view→table guard, nomicrobatch, nocopy_partitions, no_dbt_max_partition. Un-forking it is worth its own change.Related: stellar/stellar-dbt#733, stellar/stellar-dbt#988