Skip to content

feat: add Dataset::referenced_files for external orphan cleanup - #8097

Draft
LuciferYang wants to merge 3 commits into
lance-format:mainfrom
LuciferYang:feat/dataset-referenced-files
Draft

feat: add Dataset::referenced_files for external orphan cleanup#8097
LuciferYang wants to merge 3 commits into
lance-format:mainfrom
LuciferYang:feat/dataset-referenced-files

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

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 single cleanup_old_versions call.

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_versions cannot 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 ReferencedFileSet rather than raw path lists, because a naive all_listed_files - referenced anti-join is unsafe: blob v2 sidecars, index files, tags, and staging manifests are not enumerated verbatim. The caller instead asks set.is_referenced(path) per listed file, which encapsulates the three matching rules so they cannot be reimplemented incorrectly:

  • exact match for data files (including data-overlay files), deletion files, transaction files, and manifest files;
  • an _indices/{uuid}/ directory-prefix match for index artifacts, whose individual filenames are not in the manifest;
  • the blob v2 sidecar rule: a file under data/{key}/ is referenced if and only if its parent data/{key}.lance is, 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::Path on 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, and RUSTDOCFLAGS="-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 via is_referenced; overlay data files are kept (a tripwire, since they live only in fragment.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:

  • The reference-collection logic now exists in three places (referenced_files, the in-crate process_manifest used by cleanup_old_versions, and collect_paths used by deep clone). They currently agree except that only referenced_files collects 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.
  • External version-metadata files are refused implicitly only because no production path creates them; if that changes, an explicit guard should be added alongside the external row-id guard.
  • Path normalization assumes the machine-generated, encoding-free filenames the format emits today. If user-controlled name fragments ever appear in a managed subtree, the normalization should be revisited to avoid double-encoding.
  • Whether this should be feature-gated or #[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.
  • The returned set holds one entry per live data file, deletion file, and index, plus two per present version (manifest and transaction). Blob v2 sidecars are folded into their parent (one entry, not one per sidecar) and each index is a single prefix, so the two categories that grow fastest do not inflate it. Size therefore scales with live data files and present versions, not total file count, and stays bounded when versions are cleaned up on schedule. On a table with many uncleaned versions and heavy fragment churn it can still reach hundreds of MB at tens of millions of live files, at which point building and broadcasting the whole set in one process is the limit rather than the format. Sharding the set by version or path prefix, or returning a compact representation, is a possible direction if that ceiling is reached.

@github-actions github-actions Bot added the enhancement New feature or request label Jul 30, 2026
@LuciferYang
LuciferYang marked this pull request as draft July 30, 2026 11:51
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 lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@LuciferYang
LuciferYang force-pushed the feat/dataset-referenced-files branch from ee11d32 to 9337ed6 Compare July 30, 2026 12:51
@LuciferYang
LuciferYang marked this pull request as ready for review July 30, 2026 13:11
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.06475% with 33 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance/src/dataset/cleanup.rs 94.03% 22 Missing and 11 partials ⚠️

📢 Thoughts on this report? Let us know!

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread rust/lance/src/dataset/cleanup.rs Outdated
Self {
exact: exact_paths
.into_iter()
.map(|p| Path::from(p.as_str()).to_string())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@LuciferYang
LuciferYang marked this pull request as draft July 30, 2026 13:52
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.
@wjones127

Copy link
Copy Markdown
Contributor

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.

@LuciferYang

LuciferYang commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

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 tracked_files has the better machinery for it — bounded pipeline, backpressure, min_version, progress reporting, and a Python binding that already hands back a RecordBatchReader. Since the use case driving this is a distributed Python cleanup driver, an Arrow stream that scatters to workers natively is a better transport than the exact_paths()/index_prefixes() serialization I added, which exists only because ReferencedFileSet can't cross the wire on its own. Your API also resolves base_id properly, where this PR just refuses multi-base datasets, so building on it is the path to lifting that limitation rather than enshrining it.

The part I don't think survives th@e merge is SELECT DISTINCT path as the consumer's query. Two of the things a cleanup driver must decide about are not rows in any per-file view: blob v2 sidecars, whose names are obfuscated blob ids that appear in no manifest, and index artifacts, which tracked_files only materializes by listing _indices/{uuid}/. You could emit both as prefix rows — a new FileType variant for data/{key}/ and _indices/{uuid}/ — with no extra I/O, and I'd be happy with that shape. But once a row can be a prefix, the consumer needs prefix-aware matching, so the anti-join stops being a plain distinct-plus-difference. That's the piece I'd want to keep behind a library boundary rather than re-derived per caller: an automated review on this PR caught a path-normalization bug where a percent-encoding round trip made a live file look unreferenced, which is the kind of mistake that costs data and that no caller should have to get right twice.

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 for_deletion mode on TrackedFilesOptions. My concern is narrower and it's about defaults: a caller who forgets the flag gets a set that is silently incomplete rather than an error. tracked_files today is a live example, since it drops detached manifests silently (parse_version rejects d{version}.manifest), so a driver that treats its output as a keep-set would delete every file reachable only from a detached version. If the strict mode is opt-in, that failure stays reachable; if the deletion-facing entry point is its own function, it isn't. I don't feel strongly about how we express that, only that the safe reading shouldn't depend on remembering a flag.

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 — tracked_files, process_manifest in cleanup, collect_paths for deep clone, and this PR's referenced_files — and they have already drifted. Only referenced_files collects fragment.overlays[].data_file, so the other three under-report once overlays are real. That's a reason to have one collector, which is what you're proposing.

So concretely: I'd build the keep-set on tracked_files' pipeline, add prefix rows for sidecars and index directories so it needs no listing, and keep a small library-side matcher plus the guards for the deletion use case. On sequencing I see three options and I'd rather you pick: land this PR as-is and factor out the shared collector in a follow-up, factor out the reusable part first and rebase this on top of it, or do both in this PR. I lean toward one of the first two, since the consolidation touches process_manifest and collect_paths as well and I'd rather that be reviewable on its own — but which one do you prefer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants