feat(target): add file:// local filesystem destination - #166
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #166 +/- ##
==========================================
+ Coverage 51.89% 52.43% +0.54%
==========================================
Files 196 197 +1
Lines 9139 9243 +104
==========================================
+ Hits 4743 4847 +104
Misses 4396 4396 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
Summary by CodeRabbit
WalkthroughAdds a write-side local filesystem destination for Changesfile:// Destination Support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Ingest
participant LocalFilesystemDestination
participant TempDir
participant OutputFile
Ingest->>LocalFilesystemDestination: dlt_dest(file:// uri)
LocalFilesystemDestination->>TempDir: create temp bucket
Ingest->>TempDir: load intermediate rows
Ingest->>LocalFilesystemDestination: post_load()
LocalFilesystemDestination->>TempDir: read intermediate data files
LocalFilesystemDestination->>LocalFilesystemDestination: strip _dlt_ columns
LocalFilesystemDestination->>OutputFile: write aggregated rows (csv/jsonl/parquet)
LocalFilesystemDestination->>TempDir: remove temp dir (finally)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
omniload/target/filesystem/local.py (1)
177-231: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTemp directory cleanup needs a failure path
tempfile.mkdtemp()is only reclaimed inpost_load(), so any exception before that hook runs (includingdlt_run_params()validation or a failedpipeline.run()) leaves the bucket behind. Clean it up fromdlt_dest()or a surroundingtry/finallyso failed runs don’t accumulate orphaned temp dirs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@omniload/target/filesystem/local.py` around lines 177 - 231, The temporary directory created in dlt_dest() is only removed in post_load(), so failures before that hook can orphan it. Add a cleanup path around the dlt pipeline setup/run flow, or ensure dlt_dest() registers a try/finally-style cleanup, so temp_path is deleted even if dlt_run_params() validation or pipeline.run() fails. Reference the dlt_dest(), dlt_run_params(), and post_load() flow when wiring the cleanup to cover all failure paths.
🧹 Nitpick comments (2)
omniload/target/filesystem/local.py (1)
106-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated "ordered union of keys" logic.
The first-seen-order key-union loop is duplicated verbatim in
_write_csv(lines 112-118) and_write_parquet(lines 144-150). Extracting a shared helper avoids the two copies silently diverging later.♻️ Proposed refactor
+def _ordered_union_keys(rows: list[dict]) -> list[str]: + """Union of keys across rows, in first-seen order.""" + fieldnames: list[str] = [] + seen: set[str] = set() + for row in rows: + for key in row: + if key not in seen: + seen.add(key) + fieldnames.append(key) + return fieldnames + + def _write_csv(path: str, rows: list[dict]) -> None: import csv - # Union of keys in first-seen order: dlt omits null keys per row, so a later row can - # carry a column the first row lacked. First-seen order preserves the source column - # order (rather than sorting), which is what an export is expected to look like. - fieldnames: list[str] = [] - seen: set[str] = set() - for row in rows: - for key in row: - if key not in seen: - seen.add(key) - fieldnames.append(key) + fieldnames = _ordered_union_keys(rows) with open(path, "w", newline="") as handle: writer = csv.DictWriter(handle, fieldnames=fieldnames, restval="") writer.writeheader() writer.writerows(rows)Apply the equivalent change to
_write_parquet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@omniload/target/filesystem/local.py` around lines 106 - 160, The first-seen-order key-union logic is duplicated in _write_csv and _write_parquet, so extract it into a shared helper and use that helper from both writers. Keep the helper responsible for collecting fieldnames from the row dicts in first-seen order, then have _write_csv use it for DictWriter fieldnames and _write_parquet use it for building the columns mapping.tests/main/filesystem/test_local_dest.py (1)
98-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage: overwrite of an existing destination file.
The PR explicitly claims the destination "overwrites existing files," but no test writes to a
file://destination that already contains a pre-existing file (e.g., with stale/differently-shaped content) and asserts it is cleanly replaced. Consider adding a case alongside the other e2e tests.As per path instructions, "Prefer flagging missing edge-case coverage over style nits."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/main/filesystem/test_local_dest.py` around lines 98 - 213, Add an end-to-end test in test_local_dest.py that covers writing to a file:// destination path that already exists, using the same helpers like _write_source_files, invoke_ingest_command, and _read_back. Create a pre-existing out file with stale or differently shaped content, run the ingest through the existing file-to-file flow, and assert the command succeeds and the destination is fully replaced with the expected rows for one of the existing formats. Keep it alongside the other destination tests and name it to reflect overwrite behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@omniload/target/filesystem/local.py`:
- Around line 106-160: The CSV and Parquet writers only infer columns from the
row data, so empty exports lose the source schema. Update _write_csv and
_write_parquet in local.py to accept or use upstream column metadata when rows
is empty, and make _WRITERS wire through that schema so header-only exports
still emit the expected columns instead of an empty header/table.
In `@tests/main/filesystem/test_local_dest.py`:
- Around line 84-87: The pytest.raises assertion in
test_dlt_run_params_requires_two_part_table is using a regex pattern where the
dot in "<schema>.<table>" is treated as a wildcard, so tighten the match by
escaping the literal dot in the match= string. Update the assertion in this test
to use a regex that exactly matches the intended "<schema>.<table>" message
while keeping the LocalFilesystemDestination.dlt_run_params coverage unchanged.
---
Outside diff comments:
In `@omniload/target/filesystem/local.py`:
- Around line 177-231: The temporary directory created in dlt_dest() is only
removed in post_load(), so failures before that hook can orphan it. Add a
cleanup path around the dlt pipeline setup/run flow, or ensure dlt_dest()
registers a try/finally-style cleanup, so temp_path is deleted even if
dlt_run_params() validation or pipeline.run() fails. Reference the dlt_dest(),
dlt_run_params(), and post_load() flow when wiring the cleanup to cover all
failure paths.
---
Nitpick comments:
In `@omniload/target/filesystem/local.py`:
- Around line 106-160: The first-seen-order key-union logic is duplicated in
_write_csv and _write_parquet, so extract it into a shared helper and use that
helper from both writers. Keep the helper responsible for collecting fieldnames
from the row dicts in first-seen order, then have _write_csv use it for
DictWriter fieldnames and _write_parquet use it for building the columns
mapping.
In `@tests/main/filesystem/test_local_dest.py`:
- Around line 98-213: Add an end-to-end test in test_local_dest.py that covers
writing to a file:// destination path that already exists, using the same
helpers like _write_source_files, invoke_ingest_command, and _read_back. Create
a pre-existing out file with stale or differently shaped content, run the ingest
through the existing file-to-file flow, and assert the command succeeds and the
destination is fully replaced with the expected rows for one of the existing
formats. Keep it alongside the other destination tests and name it to reflect
overwrite behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae680a56-90d8-41fa-bf48-ec6300e37042
📒 Files selected for processing (6)
docs/changelog.mddocs/supported-sources/file.mddocs/supported-sources/index.mdomniload/core/registry.pyomniload/target/filesystem/local.pytests/main/filesystem/test_local_dest.py
- Add a file:// destination that writes local CSV, JSONL and Parquet files, the write-side counterpart to the file:// source, fixing panodata#143 (Unsupported destination scheme: file). - Output format comes from the destination file extension or an explicit #format hint, with the same path grammar (relative, absolute, Windows drive and UNC) as the source side. - Output drops dlt's internal _dlt_* columns so files round-trip cleanly; parent directories are created, and globs are rejected (read-only). - Generalize the existing CSV-only local-write pattern to three formats, and register the file scheme in the destinations registry.
hampsterx
force-pushed
the
feat/file-destination
branch
from
July 5, 2026 07:08
af63dcc to
885ce70
Compare
amotl
approved these changes
Jul 5, 2026
This was referenced Jul 5, 2026
This was referenced Jul 13, 2026
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
2 participants
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
file://destination that writes local CSV, JSONL and Parquet files, the write-side counterpart to thefile://source, fixing ValueError: Unsupported destination scheme: file #143 (Unsupported destination scheme: file).#formathint, with the same path grammar (relative, absolute, Windows drive and UNC) as the source side._dlt_*columns so they round-trip cleanly; parent directories are created, an existing file is overwritten, and globs are rejected (globbing is read-only).filescheme in the destinations registry.Behaviour
--dest-uri file://<path>[#format]with--dest-table <dataset>.<table>(the table only names dlt's intermediate layout; the output file is the URI path). The mechanism mirrors the existingcsv://destination: load into a temp dir via dlt's filesystem destination, then reassemble a single clean file inpost_load().omniload ingest \ --source-uri 'postgres://user:password@host:5432/db' \ --source-table 'public.users' \ --dest-uri 'file://export/users.parquet' \ --dest-table 'public.users'Relates to #106 (file/table-format handling), and pairs with #135 (the
file://source).Known trade-off
post_load()buffers the whole load before writing. dlt omits null keys per row, so correct CSV/Parquet output needs the column union across all rows (Parquet also needs the full table), and streaming that correctly would reintroduce the per-row re-header logic the codebase is moving away from. Buffered is the simple, correct first cut; streaming is a follow-up if large local exports become a use case. Happy to change the approach if you'd prefer.Test plan
file://→file://round-trips for csv/jsonl/parquet including empty source,#formathint, unsupported-format error, nested-directory creation, and a heterogeneous-row case (a column missing from the first row must survive).poe lintclean (ruff, validate-pyproject, ty);poe docs-htmlclean (-W).