Fix/ma sat v0 inserting non deltas on incremental runs - #486
Fix/ma sat v0 inserting non deltas on incremental runs#486pkumar-data wants to merge 14 commits into
Conversation
… fixing identifier casing and adding rn=1 check
…adapter implementations
There was a problem hiding this comment.
Pull request overview
This PR updates the ma_sat_v0 incremental delta logic across multiple adapter-specific implementations to prevent re-inserting non-deltas and to correctly retain intra-batch changes (including “revert within batch” scenarios). It does so by removing identifier casing manipulation in window partitions and introducing an rn (row number) to distinguish the first change in a batch from subsequent changes.
Changes:
- Removed
|lowercasing manipulation fromPARTITION BYin “latest in sat” lookups. - Added
ROW_NUMBER()(rn) into the source dedupe/change-tracking pipeline. - Updated incremental
records_to_insertfiltering to always keeprn > 1rows while only comparing the first row per key to existing satellite history.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| macros/tables/bigquery/ma_sat_v0.sql | Removes ` |
| macros/tables/databricks/ma_sat_v0.sql | Removes ` |
| macros/tables/exasol/ma_sat_v0.sql | Removes ` |
| macros/tables/fabric/ma_sat_v0.sql | Adds rn and changes incremental insertion predicate logic (Fabric already escapes identifiers). |
| macros/tables/oracle/ma_sat_v0.sql | Removes ` |
| macros/tables/postgres/ma_sat_v0.sql | Removes ` |
| macros/tables/redshift/ma_sat_v0.sql | Adds rn and changes incremental insertion predicate logic. |
| macros/tables/snowflake/ma_sat_v0.sql | Removes ` |
| macros/tables/synapse/ma_sat_v0.sql | Removes ` |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tkiehn
left a comment
tkiehn
left a comment
There was a problem hiding this comment.
Additionally to the Review from Copilot:
✅ SQL Server (newly included)
✅ Trino (newly included)
Are these adapter already following the new logic? I don't seem them included in this PR.
|
Companion regression test: ScalefreeCOM/datavault4dbt-automatic-tests#240 It adds a dedicated multi-batch fixture ( Note it exercises the default multi-batch path (
|
…r/Trino adapters - Wrap deduped_rows.rn > 1 in if not source_is_single_batch across all 9 adapters (bigquery, snowflake, postgres, databricks, exasol, fabric, oracle, redshift, synapse) to prevent invalid CTE reference when source_is_single_batch=true - Add ROW_NUMBER() rn column to SQL Server and Trino lag_source_data, deduped_row_hashdiff, and deduped_rows CTEs to bring them in line with the other adapters - Apply the same guarded WHERE clause to SQL Server and Trino records_to_insert
tkiehn
added
the
waiting-on-maintainers
…apters Window functions in latest_entries_in_sat and deduped_row_hashdiff were partitioned by parent_hashkey only, causing all MA key rows in the same hashkey group to share a single rn sequence. Rows with rn>1 were always inserted, bypassing the NOT EXISTS delta check. Fixed by adding src_ma_key_list to every PARTITION BY so each MA key gets its own independent row sequence.
…in condition deduped_row_hashdiff (and lag_source_data for subquery adapters) were partitioned by MA key but did not project those columns into the CTE output. The join back to source_data on (hashkey, ldts, hashdiff) alone could multiply rows or assign the wrong rn when multiple MA key rows share the same ldts and hashdiff. MA key columns are now selected in deduped_row_hashdiff and included in the join condition across all 11 adapters.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
macros/tables/synapse/ma_sat_v0.sql:140
- The incremental NOT EXISTS check only compares parent_hashkey + hashdiff, but
latest_entries_in_satis now deduped per (parent_hashkey, src_ma_key). If two different multi-active keys share the same hashdiff, this can incorrectly suppress inserts for one key. Include the MA key column(s) inlatest_entries_in_satand add amultikey(src_ma_key, ...)predicate to this NOT EXISTS filter (and ensure the MA key columns are selected in the latest_entries CTE).
AND {{ datavault4dbt.multikey(ns.hdiff_alias, prefix=['latest_entries_in_sat', source_cte], condition='=') }}
)
{%- endif %}
)
…S check Add MA key column(s) to latest_entries_in_sat SELECT (and latest_entries_in_sat_prep for subquery-based adapters) so the NOT EXISTS deduplication is scoped per (hashkey, MA key) rather than just per hashkey. Add multikey(src_ma_key, ...) predicate to the NOT EXISTS WHERE clause across all 11 adapters, preventing false suppression when two distinct MA keys share the same hashdiff.
tkiehn
added
waiting-on-response
…s filter
Ghost records ("unknown"/"error" defaults) carry a NULL multi-active key.
Since NULL = NULL evaluates to unknown in SQL, the plain equality checks
in multikey() never matched an already-loaded ghost row against the
NOT EXISTS dedup check or the deduped_rows join, causing them to be
re-inserted as duplicates on every incremental run (or silently dropped,
depending on which code path a model uses).
Adds an opt-in null_safe flag to multikey() and applies it to the two
MA key comparisons in ma_sat_v0, across all 11 adapters. Also removes a
BigQuery-only stray filter in latest_entries_in_sat that excluded rows
with ldts = end_of_all_times, which permanently hid the "error" ghost
record from the dedup lookup regardless of the null-safety fix.
Verified against BigQuery and SQL Server: reproduced the original
failure on both by reverting the fix, confirmed the fix resolves it
across full-refresh, incremental, and repeat-incremental runs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
CI kept failing on ma_sat_v0_03's test with a duplicate-row error. It looked like it might be about business-key deltas being re-inserted, but reproducing it against for example in BigQuery and inspecting the actual duplicate rows showed the culprit was auto-generated ghost records (the "unknown" and "error" placeholder rows every staging model emits) not real data. Solve: Ran the test again locally and all passed. |
pkumar-data
added
waiting-on-maintainers
|
dbt test combined result: ❌ DetailsRESULTS for Synapse: RESULTS for Postgres: RESULTS for BigQuery: RESULTS for Redshift: RESULTS for Snowflake: RESULTS for Exasol: RESULTS for Fabric: RESULTS for Oracle: RESULTS for Databricks: RESULTS for SQL Server: RESULTS for Trino: Link to workflow summary: https://github.com/ScalefreeCOM/datavault4dbt-ci-cd/actions/runs/29575568515 |
|
The ma_sat_v0 non-delta insertion bug has been fixed across all 11 adapters (BigQuery/default, Snowflake, Redshift, Exasol, Databricks, Postgres, Oracle, Fabric, SQL Server, Synapse, Trino). Root cause: Window functions and the latest_entries_in_sat lookup were partitioned by parent_hashkey only — all MA keys for the same hashkey were grouped together. This caused rows with rn > 1 to always be inserted, and the NOT EXISTS check could suppress valid inserts when two MA keys shared the same hashdiff. Fix applied (3 commits): Partition all window functions by (parent_hashkey, MA key) so each MA key deduplicates independently |
Description
Title: fix: ma_sat_v0 incremental delta logic - all adapters including SQL Server & Trino
Body:
Summary
Fixes the ma_sat_v0 incremental delta failure affecting all database adapters:
Root Cause & Solution
Problem: PARTITION BY with |lower caused casing issues; batch reverts weren't detected
Solution:
Adapters Covered
✅ BigQuery, Databricks, Exasol, Fabric, Oracle, Postgres, Redshift, Snowflake, Synapse
✅ SQL Server (newly included)
✅ Trino (newly included)
Status
Branch has been rebased/merged with main to include SQL Server and Trino implementations.
Ready for merge to main.