feat: add Dataset::referenced_files for external orphan cleanup - #8097
feat: add Dataset::referenced_files for external orphan cleanup#8097LuciferYang wants to merge 7 commits into
Conversation
Adds an experimental `Dataset::referenced_files()` that returns the set of
storage paths referenced by all currently-present manifests, so an external
distributed orphan-cleanup driver can list storage itself and delete files the
set does not cover. Unlike version cleanup it unions references across every
present version (not just the latest), so a file referenced only by an
older-but-present version is retained.
The result is an opaque `ReferencedFileSet` exposing `is_referenced(path)`,
which encapsulates exact matching, the `_indices/{uuid}/` prefix rule, and the
blob v2 sidecar rule (a sidecar is referenced iff its parent data file is), so
callers cannot reintroduce a data-loss bug via a naive anti-join. Reads
manifests concurrently and errors on datasets with branches, detached versions,
external bases, or external row-id files, whose files it cannot fully represent.
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
There was a problem hiding this comment.
Serialized worker reconstruction can classify live percent-encoded objects as unreferenced. ReferencedFileSet::new applies Path::from to strings already in object-store canonical form; exact_paths() then emits that encoding and worker reconstruction encodes % once more. This violates the documented distribute/reconstruct flow, so an age-eligible live data object can be deleted. Please preserve an idempotent canonical representation across serialization.
Reproducer
Add this to referenced_file_set_matcher_rules:
let producer = ReferencedFileSet::new(
vec!["data/live%25name.lance".to_string()],
vec![],
);
assert!(producer.is_referenced("data/live%25name.lance"));
let worker = ReferencedFileSet::new(
producer.exact_paths(),
producer.index_prefixes().to_vec(),
);
assert!(worker.is_referenced("data/live%25name.lance"));Invocation:
cargo test -p lance referenced_file_set_matcher_rules --lib -- --nocapture
The producer assertion passed, but the worker assertion failed. The producer stored data/live%2525name.lance; reconstruction changed it to data/live%252525name.lance.
External row-version metadata can also be committed under managed data/, but referenced_files() returns Ok without including or rejecting it. The collector handles RowIdMeta::External but not created_at_version_meta or last_updated_at_version_meta, so a cleanup driver can delete those live files. Please include both paths or fail closed for either external variant.
Reproducer
Using the same overwrite setup as the external-row-id test, set both fields and write the corresponding objects:
fragments[0].created_at_version_meta = Some(RowDatasetVersionMeta::External(
ExternalFile {
path: "data/created-at.versions".to_string(),
offset: 0,
size: 16,
},
));
fragments[0].last_updated_at_version_meta = Some(RowDatasetVersionMeta::External(
ExternalFile {
path: "data/last-updated-at.versions".to_string(),
offset: 0,
size: 16,
},
));
let refs = db.referenced_files().await?;
assert!(refs.is_referenced("data/created-at.versions"));
assert!(refs.is_referenced("data/last-updated-at.versions"));Invocation:
cargo test -p lance referenced_files_covers_external_row_version_metadata --lib -- --nocapture
Expected either NotSupported or both assertions to pass. The call returned Ok, and the first assertion failed.
…gnature Rebased onto upstream/main, where new_unstarted takes a ConcreteFileVersion instead of (major, minor). Update the overlay test accordingly.
ee11d32 to
9337ed6
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
The keep-set API is the right boundary for distributed cleanup, but a cleanup authority must be fail-closed: every live object must remain represented across collection, transport, and worker-side matching. The current implementation still violates that invariant, so it is not safe to use as a deletion predicate yet.
| Self { | ||
| exact: exact_paths | ||
| .into_iter() | ||
| .map(|p| Path::from(p.as_str()).to_string()) |
There was a problem hiding this comment.
exact_paths() already returns object-store canonical strings, so parsing them again here is not idempotent for % escapes. After driver serialization and worker reconstruction, a live path can be reported unreferenced and become deletion-eligible. Preserve a lossless serialized key representation, or decode exactly once at a defined boundary, so reconstruction is identity-preserving.
Reproducer
Add to referenced_file_set_matcher_rules:
let producer = ReferencedFileSet::new(
vec!["data/live%25name.lance".to_string()],
vec![],
);
assert!(producer.is_referenced("data/live%25name.lance"));
let worker = ReferencedFileSet::new(
producer.exact_paths(),
producer.index_prefixes().to_vec(),
);
assert!(worker.is_referenced("data/live%25name.lance"));I ran cargo test -p lance referenced_file_set_matcher_rules --lib -- --nocapture at this head. The producer assertion passed; the worker assertion failed.
| // enumerate; refuse rather than under-report (matches | ||
| // `collect_paths`). Checked here so we cover every present | ||
| // version, not just the latest. | ||
| if let Some(RowIdMeta::External(external_file)) = &fragment.row_id_meta { |
There was a problem hiding this comment.
The fail-closed guard covers row_id_meta, but external created_at_version_meta and last_updated_at_version_meta are neither retained nor rejected. A committed live metadata object under managed data/ is therefore reported unreferenced and can be deleted. Include both external paths in the exact set, or reject either external variant here.
Reproducer
Using the existing external-row-id fixture/overwrite setup, I wrote real objects at data/created-at.versions and data/last-updated-at.versions, then committed:
fragments[0].created_at_version_meta = Some(RowDatasetVersionMeta::External(
ExternalFile { path: "data/created-at.versions".into(), offset: 0, size: 16 },
));
fragments[0].last_updated_at_version_meta = Some(RowDatasetVersionMeta::External(
ExternalFile { path: "data/last-updated-at.versions".into(), offset: 0, size: 16 },
));
// commit Operation::Overwrite with these fragments
let refs = db.referenced_files().await.unwrap();
assert!(refs.is_referenced("data/created-at.versions"));
assert!(refs.is_referenced("data/last-updated-at.versions"));I ran cargo test -p lance referenced_files_covers_external_row_version_metadata --lib -- --nocapture at this head. referenced_files() returned Ok; the first assertion failed.
Normalizing paths with `object_store::path::Path::from` was not idempotent: it percent-encodes `%`, so a key stored as `data/live%25name.lance` became `data/live%2525name.lance` on ingest and grew another `%25` every time a driver round-tripped the set through `exact_paths()` and `new()`. The worker that reconstructed the set then failed to match a live file, and a caller using this as a deletion predicate would delete it. Normalize path shape only — strip leading/trailing delimiters and collapse empty segments — and leave percent-encoding untouched, since both the producer's keys and the paths a caller lists from storage are already in object-store canonical form. Also accept a percent-decoded spelling of an exact key, which can only over-retain, and skip that retry for the ordinary alphanumeric path so the hot scan does not pay for it. Reject external row-version metadata (`created_at_version_meta`, `last_updated_at_version_meta`) alongside external row-id files: it is a root-relative referenced file this set does not enumerate, so returning `Ok` let a driver see live data as unreferenced. Reject external-base indices for the same reason the per-fragment data and deletion guards exist — the top-level `base_paths` check only inspects the latest manifest. Document `_mem_wal/` and directory-marker objects as never-delete categories.
|
I'm wondering if we can combine this effort with tracked_files(). My intention was to evolve that API so it operated like a SQL view. Different use cases could send queries to it. For example, to get the distinct files this can be streamed into a distinct operator. |
|
Thank you @wjones127, I think you're right, and I'd like to do this. Both APIs walk every present manifest and collect the same data, deletion, transaction, and manifest paths, and The part I don't think survives th@e merge is On the guards, you're right that they don't need a separate type — every one of them is a precondition on the same scan and would work as a One thing I ran into that argues for your consolidation regardless of how we shape the API: there are four independent manifest-walking collectors right now — So concretely: I'd build the keep-set on |
|
I like the second option—"factor out the reusable part first and rebase this on top of it". I'm open to the first if you prefer that, though. |
|
Going with the second option. #8267 is up: it is a prerequisite for the consolidation, not the consolidation itself. #8097 waits and gets rebased onto the shared collector, as you proposed. #8267 factors out The collector consolidation is what I still owe you, and two design choices decide its shape, so I would rather have your call than guess. The first is the incompleteness contract. The second is independent of that one: whether prefixes belong in the stream. |
A fragment references its base files through `files` and its overlay files through `overlays[].data_file`. Six paths read `files` directly and so miss every overlay data file. Three enumerate, and three rewrite or retain. Enumeration: - `process_manifest` builds the cleanup keep set, so an overlay old enough to be a deletion candidate is irreversibly deleted from the live dataset. - `collect_paths` feeds deep clone's copy loop. It copies exactly the paths returned, then commits an `Operation::Clone` whose manifest carries the source fragments verbatim, overlays included, so the clone references files that were never copied and reading it fails with a not-found error. - `manifest_file_rows` powers `tracked_files`, which under-reports its documented "every file referenced in any manifest" contract. That becomes a deletion risk of its own once its output drives an external cleanup. Rewriting and retention: - Shallow clone stamps `base_id` on every local file so the clone resolves it against the parent. Skipping overlays left theirs at `None`, so the clone looked for the overlay under its own root, where it was never written. - Deep clone clears `base_id` on the same fields. An overlay kept a `base_id` naming a base the new manifest no longer lists. - Branch lineage retention promotes a path from `verified_files` into `referenced_files` when its `base_id` resolves to the parent's own URI. An overlay a branch inherited never got promoted, so the parent's cleanup deleted a file the branch still reads. The last two are inseparable. Before this change an inherited overlay carried no `base_id` at all, so retention never examined it; fixing the clone alone would give overlays a `base_id` while retention still skipped them, which is what turns the omission into a deletion. All six misbehave in any build that can open an overlay-bearing dataset: debug builds unconditionally, release builds only with `LANCE_ENABLE_UNSTABLE_DATA_OVERLAY_FILES` set, since a release reader otherwise refuses the manifest at open and these paths never see one. Within that gate none of them needs a further code change to be reachable. ## The fix `Fragment::data_files()` yields the base files chained with each overlay's data file, and `data_files_mut()` is its counterpart for the two paths that rewrite `base_id`. Both destructure `Fragment` exhaustively, so a new field fails to compile there until someone decides whether it references files — the prompt that was missing when `overlays` was added in #7535. The mutable one also needs the destructure so the disjoint field borrows are visible to the borrow checker. `manifest_file_rows` derives its `exact_size` precount from the same accessor. Counting `files.len()` separately would underflow the moment an overlay appeared, since `ExactSize::next` decrements an unsigned counter. `cleanup_data_fragments` deliberately stays on `files`. It deletes the files a caller hands it, and callers decide which files belong to the failed write: `schema_evolution` clones a live committed fragment and narrows `files` to the newly written ones while leaving `overlays` untouched, so including overlays there would delete live data. The second commit records that reasoning at the loop. Each converted caller keeps its own `base_id` handling: `collect_paths` resolves it to a base root, `manifest_file_rows` to an external base URI, and `process_manifest` ignores it. Those differ by design, so unifying them is out of scope. ## Tests Six cases, one per path, each verified to fail with the corresponding accessor reverted to `files`-only: - `keep_set_covers_referenced_overlay_files` — the keep set contains the overlay path. - `deep_clone_copies_overlay_files` — the clone returns the overlaid values rather than failing to find the file. - `test_manifest_file_rows_per_file_base_id` (extended) — the overlay row appears with its own `base_id` resolved. - `shallow_clone_stamps_base_id_on_overlay_files` — the overlay resolves against the parent. - `deep_clone_of_shallow_clone_clears_overlay_base_id` — deep-cloning a shallow clone leaves the overlay with no `base_id`. - `lineage_retention_covers_inherited_overlay_files` — the parent's keep set promotes an overlay its branch inherited. It asserts the branch actually inherited a `base_id` first, so it cannot pass vacuously. The keep-set and lineage cases assert on `process_manifests` and `retain_branch_lineage_files` output rather than driving cleanup end to end. An end-to-end version cannot reach the deletion decision: `build_listing_stream` passes `earliest_retained_manifest_time` to `read_dir_all`, which lists only files whose mtime predates it, and the test clock does not move real file mtimes, so a file written during the test is never a candidate. One pre-existing test helper needed a fix to support this: `commit_overlay` wrote through a store-root-relative path that only resolved on an in-memory store, and the clone cases need real stores. `cargo test -p lance --lib` across `dataset::cleanup`, `dataset::files`, `dataset::fragment`, `dataset::write`, `io::commit`, plus `cargo test -p lance-table --lib format`, all pass. `cargo clippy -p lance -p lance-table --all-targets -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc -p lance -p lance-table --no-deps`, and `cargo fmt --all --check` are clean. ## Relation to #8097 This came out of the #8097 discussion, where the suggestion was to factor out the reusable part before rebasing that PR onto it. The overlay drift is an independent defect, so it is split out here rather than mixed into the API discussion. It is not by itself the consolidation asked for there — `Fragment::data_files()` is a fragment-level accessor, and whether that is the "reusable part" or whether `referenced_files` should be rebuilt on `tracked_files`' pipeline is still open. I will follow up on #8097 with that question.
The accessor landed in lance-format#8267, so the hand-rolled base-plus-overlay chain here is now a second spelling of it.
|
I'll be occupied with another task these days, and will resume working on this PR next week. |
|
refactor work: #8449 |
`tracked_files` walks every present manifest with a four-stage pipeline: a lister that enumerates manifest locations and applies `min_version`, a reader that fetches them with bounded parallelism under a memory budget, an emitter that turns each manifest into file rows, and an index lister that materializes index directories. Only the last two are about the rows it emits. The first two are about walking manifests, and a second consumer needs exactly them. That consumer is `Dataset::referenced_files` in #8097, the keep-set an external orphan-cleanup driver uses to decide what it may delete. In the discussion there the suggestion was to factor out the reusable part before rebasing that PR onto it, which is what this does. Nothing in this PR depends on #8097; `tracked_files` is the only caller here and its behavior is unchanged. ## What moved The lister and reader now live in `dataset::files::scan`, which yields a `ScannedManifest` per present manifest: the manifest, its own path, and the index metadata read alongside it. `tracked_files` keeps its emitter and index lister and consumes that stream. Channel capacities, the `can_launch` predicate, the `biased` select ordering, and the `min_version` filter are carried over unchanged. ## Why the budget accounting changed shape Previously the reader charged bytes before sending and the emitter released them after processing. That worked because the emitter was the only consumer and sat in the same file, so the charge was bounded by the reader's in-flight reads plus two channel slots. A shared walk cannot rely on that: a second consumer that forgets to release would silently stall the reader. The charge now lives in a `MemoryPermit` held by `ScannedManifest` and released on drop, so backpressure follows the manifest's lifetime rather than a convention. Field order is load-bearing and commented: the permit drops after the manifest it accounts for. The bound is on the reader's prefetch, not on what a consumer retains. One read is always allowed when nothing is in flight, which is what keeps a manifest larger than the whole budget from deadlocking the walk, so a consumer that holds every manifest gets serial reads rather than a stall. That escape hatch is unchanged from before, and the module doc now states this rather than promising a bound it does not provide. ## Tests Six cases in `scan::tests`, covering what the previous arrangement had no way to observe: - the budget returns to zero once every manifest is dropped, and stays charged while a consumer holds them; - `min_version` really does skip manifests, which is why a keep-set must leave it unset; - `total` counts every manifest the walk yields; - a failed manifest read surfaces one `Err` per manifest rather than being skipped, asserted as `errors == 3` because `errors > 0` would also pass on a reader that stopped at the first failure or on a listing failure; - dropping the stream early releases every in-flight permit. Each fails against the corresponding mistake: a leaked permit, a bypassed filter, a reader that aborts on first error. `cargo clippy -p lance --all-targets -- -D warnings`, `RUSTDOCFLAGS="-D warnings" cargo doc -p lance --no-deps`, and `cargo fmt --all --check` are clean; `dataset::files` (18) and `dataset::cleanup` (41) pass. The full suite is left to CI. ## Reviewing this The second commit is the result of reviewing the first, so the two are worth reading separately. It removes an unreachable error branch the extraction left behind, moves the index fan-out after the row batches so a full index channel cannot block row output while holding budget, makes the test-only budget accessor private, and corrects the module doc described above.
`referenced_files` had its own walk: `list_manifest_locations` fed into `try_for_each_concurrent`, with a `Mutex` around the two path sets and an atomic manifest counter. The shared walk from lance-format#8449 does the same listing and reading, so consume that instead. Two things fall out. The sets and the counter become plain locals, since the stream is consumed sequentially and the concurrency now lives in the walk. And the keep-set inherits the walk's memory budget, which it did not have before: it read manifests at `io_parallelism()` with no bound on how much manifest it held at once, on exactly the datasets this API targets. `min_version` is deliberately left unset, with the reason at the call site: a keep-set has to cover every present manifest, and skipping one would authorize deleting the files it is the last to reference.
This comment was marked as spam.
This comment was marked as spam.
…8562) `Fragment::referenced_lance_files` documents which of a fragment's referenced files it does *not* yield, and the list named two of the four kinds. A fragment can also reference external row-version metadata through `created_at_version_meta` and `last_updated_at_version_meta`, both `RowDatasetVersionMeta::External(ExternalFile)` just like the external row-id file. Classify by format instead of enumerating, so the boundary stays correct as kinds are added: a deletion file is `.arrow` or `.bin` per `DeletionFileType::suffix`, and external row-id or row-version metadata is an `ExternalFile`, a `(path, offset, size)` byte range rather than a Lance file. This matters because the omission reads as a statement about what is referenced at all. `Dataset::referenced_files` in #8097 refuses outright on each of those three external kinds, precisely because they are referenced but not enumerated by any per-file walk; a reader of this doc could reasonably conclude row-version metadata is not a referenced file. Docs only, no code change. `cargo fmt --all --check`, `cargo clippy -p lance-table --all-targets -- -D warnings`, and `RUSTDOCFLAGS="-D warnings" cargo doc -p lance-table --no-deps` are clean; `lance-table` `format` tests pass (32). Co-authored-by: Xuanwo <github@xuanwo.io>
2 participants
What
Adds an experimental
Dataset::referenced_files()that returns the set of storage paths referenced by all of a dataset's currently-present manifests. It is meant for an external, distributed orphan-cleanup driver: the driver lists storage itself and deletes files the set does not cover, which lets orphan cleanup scale out (list + delete in parallel) instead of running entirely inside a singlecleanup_old_versionscall.Unlike version cleanup, this unions references across every present version rather than just the latest, so a file referenced only by an older-but-present version is retained. It reads only manifests (concurrently) and never lists the potentially huge
data/,_indices/, or_deletions/trees, so its cost scales with the number of present versions rather than the number of files.Why
Orphan cleanup on a large table is dominated by listing storage, which a single-process
cleanup_old_versionscannot parallelize. Exposing the "keep set" as a read-only, metadata-only call lets an external engine (for example a distributed job) do the listing and deletion while relying on the core format to decide what is still referenced. Re-deriving that keep-set outside the core is a correctness hazard, because the caller has to reproduce format-specific rules (blob v2 sidecars, index layout) exactly or it deletes live data.Design
The result is an opaque
ReferencedFileSetrather than raw path lists, because a naiveall_listed_files - referencedanti-join is unsafe: blob v2 sidecars, index files, tags, and staging manifests are not enumerated verbatim. The caller instead asksset.is_referenced(path)per listed file, which encapsulates the three matching rules so they cannot be reimplemented incorrectly:_indices/{uuid}/directory-prefix match for index artifacts, whose individual filenames are not in the manifest;data/{key}/is referenced if and only if its parentdata/{key}.lanceis, since a sidecar's obfuscated filename is not recorded in the manifest and must not require listing the data tree to discover.Paths are normalized through
object_store::path::Pathon both the stored and the queried side, so a caller that lists with a leading or trailing slash still matches; a mismatch here would be a false negative and delete a live file.The call errors (rather than returning an incomplete set a caller could act on) for datasets it cannot fully represent: branches, detached versions, external multi-base fragments, and external row-id files. It also errors if it observes zero manifests, because an empty keep-set would authorize deleting the whole dataset.
How the caller uses it safely
Only files in the managed subtrees (
data/,_deletions/,_transactions/,_indices/, and_versions/*.manifest) are candidates. Files under_refs/(tags/branches), staging manifests (_versions/.tmp*), and the version-hint file must never be treated as orphans, and the set intentionally does not enumerate them. Deletion is additionally gated on a caller-enforced age threshold, because the set is a point-in-time snapshot and a file written just before its commit lands is referenced by no present manifest yet.Testing
cargo test -p lance --lib dataset::cleanup::tests::referenced_file(12 tests), plus the doctest,cargo clippy -p lance --all-targets -- -D warnings, andRUSTDOCFLAGS="-D warnings" cargo doc -p lance --no-deps, all green. Coverage includes: a file referenced only by an older-but-present version is retained; manifests, deletion, and transaction files are reported; index prefixes cover every on-disk index file; a blob v2 sidecar's parent is reported and the sidecar itself is matched viais_referenced; overlay data files are kept (a tripwire, since they live only infragment.overlays); multi-fragment union; deterministic sorted output with a serialize/reconstruct round-trip; and rejection of branches, detached versions, external bases, and external row-id files (asserting both the error variant and message).Follow-ups / known limitations
This is marked experimental and its shape may change. A few items are intentionally out of scope and worth tracking:
referenced_files, the in-crateprocess_manifestused bycleanup_old_versions, andcollect_pathsused by deep clone). They currently agree except that onlyreferenced_filescollects data-overlay files. Overlay files are an unstable, opt-in feature with no production write path today, so this is latent rather than a live bug, but before overlays are enabled by default the built-in GC and deep-clone paths should collect them too. Extracting a shared fragment-to-data-files iterator would keep a future artifact kind from silently diverging again. Best done as a separate focused change.#[doc(hidden)]rather than a plain public method, given it currently refuses branches and multi-base datasets (both growing features), is a judgment call left to maintainers.