feat!(io): FileIO::list_prefix — recursively list files under a prefix#2754
feat!(io): FileIO::list_prefix — recursively list files under a prefix#2754raghav-reglobe wants to merge 6 commits into
Conversation
Adds a listing capability to FileIO, needed by maintenance operations
(e.g. orphan-file cleanup) that must enumerate everything under a table
location and compare against paths recorded in table metadata:
- ListEntry { path, size, last_modified_ms } — paths are returned
absolute, in the same form as the listed prefix, so they round-trip to
other FileIO operations and compare directly against metadata paths.
last_modified_ms is optional: age-based logic must treat files with an
unknown modification time as not eligible.
- Storage::list_prefix with a FeatureUnsupported default, implemented
for the OpenDAL storages (recursive lister; entries mapped back to
absolute form), the resolving router, local fs (recursive walk with
real mtimes), and memory (which now stamps write times so age logic
is testable).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Raghvendra Singh <raghav@cashify.in>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Raghvendra Singh <raghav@cashify.in>
|
Could we clarify the exact semantics of I did a quick local check on this PR head by adding temporary tests for
The same is true for local fs: if the input path is an existing file, That behavior may be exactly what we want for the intended first consumer, orphan-file cleanup, because the operation is normally scoped to a table location or a data/metadata directory. This also lines up with the Iceberg docs/spec side: the table metadata However, the name So I think either behavior can be defensible, but the contract should be explicit. Could we document whether
If the current directory-style semantics are intentional, a small test asserting that listing an exact file path returns empty would make the contract much clearer for future orphan-file cleanup callers. |
… with tests Per review: document on FileIO::list_prefix (and the Storage trait) that the prefix is treated as a directory-style location — implementations normalize a prefix without a trailing '/' by appending one, so a path naming an existing file yields an empty list, a trailing-slash prefix is equivalent to the non-slash form, and a missing prefix is empty rather than an error. Java's SupportsPrefixOperations.listPrefix leaves the exact-file/raw-prefix case implementation-defined; this contract fixes it to directory-style uniformly so callers do not have to defend against both behaviors. Add regression tests for MemoryStorage and LocalFsStorage asserting the exact-file-path case returns empty and the trailing-slash equivalence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Raghvendra Singh <raghav@cashify.in>
|
Thanks for digging into this — running temp tests against the branch head is exactly the check this needed, and you're right that the contract was implicit. The directory-style behavior is intentional, so I've gone with your option 1 and made it explicit. Two reasons: the intended callers are maintenance operations (listing a table location or its
Pushed as 4bc0260. If you think there's a real caller for the Hadoop-style "exact file path returns itself" edge, happy to reconsider — but one fixed semantic seems better than implementation-defined here. |
|
That makes sense to me. Pinning this to one directory-style contract seems easier for callers than inheriting implementation-defined behavior. I did one more quick local check against the OpenDAL memory backend: raw I opened a small PR with a regression test for that adapter behavior here, in case you think it would be useful to include in this PR: raghav-reglobe#1 |
Signed-off-by: kid <libingshuai1@huawei.com>
|
That raw- |
Summary
Adds a recursive file-listing capability to
FileIO, the missing primitive for table-maintenance operations (orphan-file cleanup foremost) that need to enumerate everything under a table location and compare it against paths recorded in table metadata.ListEntry { path, size, last_modified_ms }— paths come back absolute, in the same form as the listed prefix, so they round-trip directly to otherFileIOoperations and compare against metadata-recorded paths without normalization.last_modified_msisOption: storages that don't track modification times reportNone, and age-based callers must treat such files as not eligible (protects possibly-in-flight writes).Storage::list_prefixships with aFeatureUnsupporteddefault and is implemented for the OpenDAL storages (recursive lister, entries mapped back to absolute form), the resolving router, local fs (recursive walk with real mtimes), and memory (which now stamps write times so age-based logic is unit-testable).Motivation
First consumer is a
RemoveOrphanFilesAction(Java/Go parity — cf. the maintenance-operations proposal in #1453), which I'll submit as a follow-up PR stacked on this one. Kept separate so this stays a small, standalone review.Validation
list_prefix(prefix-form round-trip, sizes, mtimes, empty-prefix behavior).icebergsuite: 1377 passed.cargo fmtclean;public-api.txtregenerated.