Filesystem: Improve parameter validation/selection for write-disposition strategy (--incremental-strategy, --full-refresh)

[coderabbitai]

Summary

Track a proposal to improve how strategy-selection parameters (e.g. --incremental-strategy, --full-refresh) are validated and honoured for filesystem-family sources (local, s3, gcs, sftp), as raised in #137.

Background

From the discussion in #137 (comment by @hampsterx):

Because filesystem sources override incremental_strategy to none, an explicit --incremental-strategy flag is currently swallowed: append becomes a no-op and replace is unreachable through that flag (only --full-refresh can reset the destination). Two complications noted:

1. replace is also the current default strategy value, and the run only sees the resolved value, not whether the flag was explicitly passed. So "keep append as the default but honour an explicit replace" requires a was-provided sentinel; simply dropping the override would flip these sources to replace-by-default unintentionally.
2. Any fix must be scoped to the filesystem family (or gated on a source capability), since several handled SaaS/streaming sources set their own resource-level merge/replace dispositions, and a run-level disposition would override those.

@amotl additionally suggested considering a broader channelling/validation layer for such parameters based on pydantic, potentially across all pipeline elements, as a longer-term direction (kept here as context, not a hard requirement).

Task

• Design a mechanism to detect whether --incremental-strategy was explicitly provided by the user (vs. defaulted), scoped to filesystem sources.
• Honour an explicit replace/append value from --incremental-strategy for filesystem sources without breaking the append-by-default behaviour.
• Ensure the fix does not affect other sources that set their own resource-level merge/replace dispositions.
• Optionally evaluate introducing pydantic-based validation for pipeline parameters as a longer-term follow-up.

Acceptance criteria

• Explicit --incremental-strategy replace (or append) works as expected for filesystem sources.
• Default behaviour (append on re-run, --full-refresh for reset) is preserved when the flag is not passed.
• No regression for non-filesystem sources with resource-level dispositions.

References

• Follow-up from Incremental data loading behaviour for local and remote files #137
• Incremental data loading behaviour for local and remote files #137 (comment)

Requested by: @amotl

[amotl]

Hi @hampsterx,

I just found this. Should this somehow influence our plans on "naming things"?

• Deprecated "full_refresh" with "dev_mode" dlt-hub/verified-sources#565

With kind regards,
Andreas.

[hampsterx]

Hi Andreas,

Short answer: it doesn't force our hand, but it's a useful signal, and the obvious reading of it is a trap.

We never pass dlt's full_refresh. --full-refresh maps to refresh="drop_resources" on the pipeline (omniload/api.py:271), so when dlt finally removes the deprecated argument, nothing here breaks.

In fact the migration 565 represents already happened here, and it predates 565: the very commit that introduced the strategy enum (3174960, 2024-07-12, upstream ingestr) also did -full_refresh=full_refresh / +refresh="drop_resources" if full_refresh else None. So the code moved to the new concept years ago; only the flag name stayed behind. That's the whole of what's left to decide.

The trap: dlt's full_refresh → dev_mode rename is a different concept from ours. dlt aliases the deprecated full_refresh straight onto dev_mode, which appends a timestamp suffix to the dataset name (once per pipeline instance, pipeline.py:352 / _create_pipeline_instance_id) and drops local pipeline state. It loads into a separate dataset and never touches the existing destination tables. Ours drops and reloads the resource's tables in place. So renaming --full-refresh to --dev-mode in sympathy with 565 would attach dlt's name to a materially different behaviour. What's actually true is narrower: we're using the name dlt retired, for the concept dlt calls refresh=.

The current map:

dlt (current)	omniload	note
write_disposition: skip/append/replace/merge	--incremental-strategy	flattened, see below
refresh: drop_sources/drop_resources/drop_data	--full-refresh (bool)	retired dlt name; only 1 of 3 modes reachable
dev_mode (ex-full_refresh)	not exposed	probably correctly so

What I'd propose, if you want a naming pass: replace --full-refresh with --refresh[=MODE] mirroring dlt's TRefreshMode, where a bare --refresh keeps today's drop_resources behaviour. That frees the retired name, aligns the vocabulary with the live dlt API, and incidentally unlocks drop_data (truncates the resource's tables and clears their state, leaving the schema intact), which isn't reachable today. Pre-1.0, so I'd do a straight rename rather than carry an alias, but that's your call.

I would not rename --incremental-strategy. It isn't write_disposition under another name: it flattens dlt's write_disposition × merge-strategy onto a single axis (plus an omniload-only none), so aligning the name would misrepresent it.

Which is how I found a bug, and I think it's in scope here. The flattening puts scd2 on the write-disposition axis, where dlt has no such disposition (TWriteDisposition is skip|append|replace|merge; scd2 is a TLoaderMergeStrategy). --incremental-strategy scd2 is accepted by the CLI and then takes one of three paths, none of which is an SCD2 load:

• Ordinary sources (handles_incrementality() is False, which is every SQL source via core/router.py:44-46, and where SCD2 would actually mean something): api.py:457-459 passes the enum value straight through, so dlt gets write_disposition="scd2" and rejects it during extract, before the destination is involved. Local sqlite → duckdb repro: Path `new_table/people`: field `write_disposition=scd2` is not one of: ['skip', 'append', 'replace', 'merge']. The same run with merge loads fine.
• Filesystem sources: rejected earlier, inside omniload, with a clear error (api.py:296-305, from feat(filesystem): honour explicit append/replace strategy #204).
• Sources that manage their own incrementality (the SaaS/streaming ones): the run-level strategy is suppressed to none at api.py:308-309, so scd2 is silently ignored rather than failing.

delete+insert got the treatment scd2 never did: api.py:216-224 remaps it to merge and marks the incremental key as a merge key. And the valid structured form already exists in the tree, at source/shopify/adapter.py:755: write_disposition={"disposition": "merge", "strategy": "scd2"}, used at the resource level.

It stayed hidden because the only scd2 coverage is the filesystem reject-path test from GH-204 (tests/main/filesystem/test_incremental_strategy.py:130); nothing exercises it where it's meant to work. It predates the fork: the enum value and the bare mapping both arrive in 3174960 ("migrate to v0.5.1", 2024-07-12).

This is a decent argument for the pydantic-based validation you floated: a typed layer would have caught a flattened enum whose values don't all land on the same dlt axis.

Do you want the scd2 fix (map to the merge+strategy shape, plus coverage on a non-filesystem source), inside GH-188 or as its own issue? And should the --refresh rename ride along in that pass, or stay separate?

[amotl]

So the code moved to the new concept years ago; only the flag name stayed behind.
The trap: dlt's full_refresh → dev_mode rename is a different concept from ours.

All right, thank you for the clarification.

Do you want the scd2 fix?
Should the --refresh rename ride along?

If you discovered any other flaws, I will be happy to follow your decisions to fix them. Otherwise, or afterwards, feel free to close the ticket without further ado. 🌻