Skip to content

fix(incremental): perform a real SCD2 load for the scd2 strategy#235

Merged
amotl merged 1 commit into
panodata:mainfrom
hampsterx:fix/incremental-strategy-scd2
Jul 17, 2026
Merged

fix(incremental): perform a real SCD2 load for the scd2 strategy#235
amotl merged 1 commit into
panodata:mainfrom
hampsterx:fix/incremental-strategy-scd2

Conversation

@hampsterx

Copy link
Copy Markdown
Contributor

Summary

--incremental-strategy scd2 has never performed an SCD2 load. IncrementalStrategy flattens dlt's two axes (write disposition x merge strategy) onto one, and scd2 lives on the second, but it was passed straight onto the first. Ordinary sources died in extract (field `write_disposition=scd2` is not one of: ['skip', 'append', 'replace', 'merge']), and sources that own their write disposition ignored it silently. It is now mapped to dlt's structured {"disposition": "merge", "strategy": "scd2"}, next to the existing delete+insert unflattening.

  • A changed row is now retired and succeeded rather than overwritten, with _dlt_valid_from / _dlt_valid_to materialized, and a re-run over unchanged data is a no-op.
  • SCD2 has dlt compare the whole source table against the records it holds open and retire whatever is missing from it, so the combinations that misrepresent that table are refused rather than left to rewrite history silently:
    • --incremental-key, --sql-limit, and --yield-limit each read back only part of the table, retiring every record they leave out while the source still holds it unchanged.
    • A --mask algorithm that draws a fresh value per run (date_shift, noise, random, uuid) rewrites the rows being compared, so every record reads as changed. masking.NON_DETERMINISTIC_ALGORITHMS names them; deterministic algorithms such as sha256 are unaffected.
  • dlt computes the row hash SCD2 compares with only for rows it receives as dicts, so SQL sources are read with the sqlalchemy backend for SCD2, and an explicit pyarrow / connectorx backend or an mmap:// source is refused rather than dying later in normalize on an unpopulated _dlt_id. Populating it via normalize.parquet_normalizer.add_dlt_id is not an alternative: those ids are random per run rather than a hash of the row, so every record would be retired and re-inserted on every load. A query: table is exempt from the backend rule, being read with sqlalchemy whichever backend is named.
  • --primary-key is not required, as dlt hashes the whole row to spot a change. It stays optional and marks the natural key on the destination.
  • No change for sources that manage their own incrementality (scd2 stays ignored, never applied) or for the filesystem family (key-based strategies still rejected).
  • Docs: the incremental-loading guide gains an SCD2 section, and scd2 joins the --incremental-strategy option list, which previously named it only among the strategies filesystem rejects.

The flaw is inherited from ingestr (3174960, 2024-07-12) and survived because the only scd2 coverage was the filesystem reject path, so nothing exercised it where it should work.

Follows up on the scd2 flaw raised in #188.

Test plan

  • 19 new unit tests in the Docker-free lane: a changed record retired and succeeded, a re-run over unchanged data being a no-op, tracking without a primary key, partial-read rejection (--incremental-key / --sql-limit / --yield-limit), non-deterministic mask rejection and a deterministic mask still loading, Arrow backend and mmap:// rejection, rejection surfacing under --dry-run, the backend rule not firing for non-SQL or query: sources, default backend selection, and a managed-source regression guard
  • Each behavioural test verified to fail against the pre-fix code; the managed-source and negative guards pass on both sides by design
  • Every rejection reproduced as real corruption before being added (e.g. --sql-limit 1 retired two unchanged records; a random mask gave 4 rows after 2 identical runs)
  • poe lint clean (ruff format/check, validate-pyproject, ty)
  • poe test-fast: 641 passed, 44 skipped (skips are unset SaaS credentials)
  • poe docs-html clean under -W
  • Existing filesystem scd2 reject-path test stays green
  • Docker integration lane not run locally; no integration test exercises scd2, and the new logic is gated behind it

- `--incremental-strategy scd2` now performs an SCD2 load: the validity columns
  (`_dlt_valid_from` / `_dlt_valid_to`) are materialized, and a row that changes
  in the source is retired and succeeded rather than overwritten. Previously the
  option performed an SCD2 load on no path at all. dlt rejected it during
  extract for ordinary sources (`write_disposition=scd2` is not one of
  `skip`/`append`/`replace`/`merge`), and sources that own their write
  disposition ignored it silently.
- Mechanism: `IncrementalStrategy` flattens dlt's two axes (write disposition x
  merge strategy) onto one, and `scd2` lives on the second. It is now mapped to
  dlt's structured `{"disposition": "merge", "strategy": "scd2"}`, next to the
  existing `delete+insert` unflattening, instead of being passed through as a
  write disposition dlt has no such value for.
- SCD2 has dlt compare the whole source table against the records it holds open
  and retire whatever is missing from it. The combinations that misrepresent
  that table are now refused rather than left to rewrite history silently:
  - `--incremental-key`, `--sql-limit`, and `--yield-limit` each read back only
    part of the table, retiring every record they leave out while the source
    still holds it unchanged.
  - A `--mask` algorithm that draws a fresh value per run (`date_shift`,
    `noise`, `random`, `uuid`) rewrites the rows being compared, so every record
    reads as changed and is retired and re-inserted on every load.
    `masking.NON_DETERMINISTIC_ALGORITHMS` names them.
  - The Arrow-yielding SQL backends (`pyarrow`, `connectorx`) and the `mmap://`
    source carry no row hash, which dlt computes only for rows it receives as
    dicts, leaving `_dlt_id` unpopulated and the load dead in `normalize`.
    Populating it via `normalize.parquet_normalizer.add_dlt_id` is not an
    alternative, as those ids are random per run rather than a hash of the row,
    so every record would be retired and re-inserted on every load. SQL sources
    are read with `sqlalchemy` for SCD2 instead. A `query:` table is exempt from
    the backend rule, being read with sqlalchemy whichever backend is named, and
    the guide states that such an expression is itself the tracked relation.
- `--primary-key` is not required for SCD2, as dlt hashes the whole row to spot
  a change. It stays optional and marks the natural key on the destination.
- Unchanged: sources that manage their own incrementality still ignore the
  run-level strategy, and the filesystem family still rejects the key-based
  strategies.
- The flaw predates the fork (`3174960`, 2024-07-12) and survived because the
  only `scd2` coverage was the filesystem reject path. The new tests exercise it
  on the sources where it applies, including that a re-run over unchanged data
  is a no-op rather than a retire-and-reinsert of every row.
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)
  • coderabbit-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 340d505a-9ff2-4959-aa43-96d9d0333461

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@read-the-docs-community

Copy link
Copy Markdown

Documentation build overview

📚 omniload | 🛠️ Build #33627409 | 📁 Comparing 87f2854 against latest (5c25aeb)

  🔍 Preview build  

2 files changed
± commands/ingest.html
± getting-started/incremental-loading.html

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.37%. Comparing base (5c25aeb) to head (87f2854).

Files with missing lines Patch % Lines
src/omniload/codec/masking.py 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
+ Coverage   55.23%   55.37%   +0.13%     
==========================================
  Files         210      210              
  Lines        9796     9830      +34     
==========================================
+ Hits         5411     5443      +32     
- Misses       4385     4387       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amotl amotl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. I wasn't aware SCD2 was suppressed or even defunct due to different circumstances. Thank you so much for unlocking it swiftly! 💯

@amotl
amotl merged commit 4bc3bd9 into panodata:main Jul 17, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants