feat: add Dataset::referenced_files for external orphan cleanup - #8097
feat: add Dataset::referenced_files for external orphan cleanup#8097LuciferYang wants to merge 3 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.
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 |
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.