feat(filesystem): add file-level incremental loading#239
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds opt-in filesystem incremental loading based on modification-time cursors, with stable source identities, CLI/API validation, state-sync handling, resource wiring, documentation, and local and remote integration coverage. ChangesFilesystem incremental loading
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Documentation build overview
3 files changed± commands/ingest.html± getting-started/incremental-loading.html± supported-sources/filesystem.html |
Add opt-in modification-time selection before filesystem reader I/O while preserving the existing default behavior. Isolate cursor state by source and glob, restrict the mode to append loads, and require durable pipeline state. Document reset and boundary semantics and cover local and remote transports.
dc49bf6 to
e848936
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
==========================================
+ Coverage 55.37% 55.59% +0.22%
==========================================
Files 210 210
Lines 9831 9878 +47
==========================================
+ Hits 5444 5492 +48
+ Misses 4387 4386 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/omniload/api.py (1)
279-284: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTemp pipeline dir is not cleaned up when
pipeline.run()fails.
remove_temp_pipelines_dir()is called on the state-sync validation failure, the dry-run early return, and the success path, but not in theexcept BaseExceptionhandler aroundrun_pipeline(). A failed run with a temp pipelines dir (the default when--pipelines-diris omitted) leaks that directory on every failure, which is more consequential now that filesystem-incremental relies on this same temp-dir mechanics being correctly cleaned up.♻️ Suggested fix: wrap cleanup in try/finally
try: max_retries = 3 for attempt in range(max_retries): ... report_errors(run_info) destination.post_load() getattr(source, "post_load", lambda: None)() except BaseException: release = getattr(source, "release", None) if callable(release): release() raise + finally: + remove_temp_pipelines_dir() end_time = datetime.now() elapsed = end_time - start_time elapsedHuman = f"in {humanize.precisedelta(elapsed)}" - - remove_temp_pipelines_dir()Also applies to: 637-670
🤖 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 `@src/omniload/api.py` around lines 279 - 284, Ensure the exception handler surrounding run_pipeline() invokes remove_temp_pipelines_dir() before propagating the failure, preferably via a try/finally structure that guarantees cleanup for every run outcome. Preserve the existing cleanup behavior for validation failures, dry-run returns, and successful execution, and use the existing remove_temp_pipelines_dir function.
🤖 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 `@tests/dlt_filesystem/test_source_incremental.py`:
- Around line 191-204: Add focused tests for _endpoint_namespace covering an
IPv6 host and an endpoint with no hostname, such as a path-only or malformed
URL. Assert the IPv6 result preserves bracketed host formatting and the
no-hostname case follows the helper’s existing fallback behavior, using the same
default namespace context as the current tests.
---
Nitpick comments:
In `@src/omniload/api.py`:
- Around line 279-284: Ensure the exception handler surrounding run_pipeline()
invokes remove_temp_pipelines_dir() before propagating the failure, preferably
via a try/finally structure that guarantees cleanup for every run outcome.
Preserve the existing cleanup behavior for validation failures, dry-run returns,
and successful execution, and use the existing remove_temp_pipelines_dir
function.
🪄 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: 7aa0a68c-b8e6-49b5-80a2-2ba7051a4cd7
📒 Files selected for processing (17)
docs/commands/ingest.mddocs/getting-started/incremental-loading.mddocs/supported-sources/filesystem.mdsrc/dlt_filesystem/source/adapter.pysrc/dlt_filesystem/source/base.pysrc/dlt_filesystem/source/impl/local.pysrc/dlt_filesystem/source/impl/remote.pysrc/dlt_filesystem/source/model.pysrc/omniload/api.pysrc/omniload/core/model.pysrc/omniload/main.pysrc/omniload/model.pytests/dlt_filesystem/test_source_incremental.pytests/main/filesystem/test_file_incremental.pytests/main/test_app.pytests/util/__init__.pytests/warehouse/filesystem/test_remote.py
| def test_endpoint_namespace_excludes_credentials_and_query_values(): | ||
| assert ( | ||
| _endpoint_namespace( | ||
| "https://user:password@MINIO.example:9000/storage/?token=secret", | ||
| "default", | ||
| ) | ||
| == "minio.example:9000/storage" | ||
| ) | ||
|
|
||
|
|
||
| def test_endpoint_namespace_normalizes_bare_host_and_url_forms(): | ||
| assert _endpoint_namespace("account.blob.example", "default") == ( | ||
| _endpoint_namespace("https://account.blob.example", "default") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing edge-case coverage for _endpoint_namespace's IPv6 and no-hostname branches.
Both _endpoint_namespace tests only exercise plain hostnames. The IPv6-bracketing branch (if ":" in host: host = f"[{host}]") and the "no hostname" fallback (e.g. a malformed or path-only endpoint) in src/dlt_filesystem/source/impl/remote.py (lines 26-27, 30-31) aren't covered. Since this helper directly feeds storage_namespace, which is hashed into the persistent incremental cursor key, an untested regression here could silently change/collide cursor identity for IPv6-hosted S3-compatible endpoints.
As per path instructions for tests/**/test_*.py: "Prefer flagging missing edge-case coverage over style nits."
🧪 Suggested additional test cases
def test_endpoint_namespace_normalizes_bare_host_and_url_forms():
assert _endpoint_namespace("account.blob.example", "default") == (
_endpoint_namespace("https://account.blob.example", "default")
)
+
+
+def test_endpoint_namespace_brackets_ipv6_hosts():
+ assert _endpoint_namespace("https://[::1]:9000/bucket", "default") == "[::1]:9000/bucket"
+
+
+def test_endpoint_namespace_falls_back_when_hostname_is_missing():
+ assert _endpoint_namespace("/just/a/path", "default") == "default"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_endpoint_namespace_excludes_credentials_and_query_values(): | |
| assert ( | |
| _endpoint_namespace( | |
| "https://user:password@MINIO.example:9000/storage/?token=secret", | |
| "default", | |
| ) | |
| == "minio.example:9000/storage" | |
| ) | |
| def test_endpoint_namespace_normalizes_bare_host_and_url_forms(): | |
| assert _endpoint_namespace("account.blob.example", "default") == ( | |
| _endpoint_namespace("https://account.blob.example", "default") | |
| ) | |
| def test_endpoint_namespace_excludes_credentials_and_query_values(): | |
| assert ( | |
| _endpoint_namespace( | |
| "https://user:password@MINIO.example:9000/storage/?token=secret", | |
| "default", | |
| ) | |
| == "minio.example:9000/storage" | |
| ) | |
| def test_endpoint_namespace_normalizes_bare_host_and_url_forms(): | |
| assert _endpoint_namespace("account.blob.example", "default") == ( | |
| _endpoint_namespace("https://account.blob.example", "default") | |
| ) | |
| def test_endpoint_namespace_brackets_ipv6_hosts(): | |
| assert _endpoint_namespace("https://[::1]:9000/bucket", "default") == "[::1]:9000/bucket" | |
| def test_endpoint_namespace_falls_back_when_hostname_is_missing(): | |
| assert _endpoint_namespace("/just/a/path", "default") == "default" |
🤖 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/dlt_filesystem/test_source_incremental.py` around lines 191 - 204, Add
focused tests for _endpoint_namespace covering an IPv6 host and an endpoint with
no hostname, such as a path-only or malformed URL. Assert the IPv6 result
preserves bracketed host formatting and the no-hostname case follows the
helper’s existing fallback behavior, using the same default namespace context as
the current tests.
Source: Path instructions
There was a problem hiding this comment.
@amotl, thanks for tracking this in #241. Keeping the focused IPv6 and no-hostname coverage follow-up out of this PR makes sense.
There was a problem hiding this comment.
Fixed - added focused coverage for bracketed IPv6 endpoints and missing-host fallback behavior.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Very sweet, thank you so much. 💯
Trivia: I've derived two tickets from minor comments by @coderabbitai that can be processed later at your disposal.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
Review
Changes
17 files changed, with 926 insertions and 30 deletions across the CLI/API, filesystem source adapters, documentation, and tests.
Test plan
Closes #209