refactor: extract the shared manifest walk behind tracked_files - #8449
Merged
wjones127 merged 2 commits intoAug 17, 2026
Merged
Conversation
`tracked_files` inlines two pipeline stages that have nothing to do with the rows it emits: a lister that enumerates manifest locations and applies `min_version`, and a reader that fetches manifests with bounded parallelism and a memory budget. Both are about walking manifests, not about what a caller builds from them. Move them to `dataset::files::scan`, which yields a `ScannedManifest` per present manifest. `tracked_files` keeps its emitter and index lister and now consumes that stream. The budget accounting changes shape. Previously the reader charged bytes before sending and the emitter released them after processing, which worked because the emitter was the only consumer and sat in the same file. A shared walk cannot rely on that: a consumer who 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 matters, and the comment says so: the permit drops after the manifest it accounts for. Tests cover what the previous arrangement had no way to observe: that the budget returns to zero once every manifest is dropped, that holding manifests keeps it charged, and that `min_version` really does skip manifests. Each fails with the permit leaked or the filter bypassed.
Fixes found by reviewing the extraction itself. The reader's error-reporting branch was unreachable: no `?` remained in its spawned block after the extraction, so the enclosing `Result<()>` wrapper and both `return Ok(())` statements were scaffolding for a failure that can no longer happen. Read failures travel as `Err` items in the stream instead, and the docstring now says so. The emitter fanned out index metadata before building file-row batches, so a full index channel blocked row output while the manifest still held its share of the memory budget. Upstream did the fan-out on the reader side, after handing the manifest off, where it could not stall rows. Move it after the batches and release the manifest first; `mem::take` on the indexes also drops a clone per indexed manifest. `ManifestScan::inflight_bytes` is private with a test-only accessor rather than a `pub` field, so the struct has one shape and the construction site needs no mirrored `cfg`. The module doc claimed a consumer that holds every manifest stalls the walk. It does not: one read is always allowed when nothing is in flight, which is what keeps an oversized manifest from deadlocking, so a hoarding consumer gets serial reads instead. That escape hatch is unchanged from upstream and this walk's only consumer drops each manifest after use, so the doc was the defect, not the condition. Tests cover the two paths the extraction changed and nothing exercised: a failed manifest read surfaces one `Err` per manifest rather than being skipped, and dropping the stream early releases every in-flight permit. Each fails against a reader that stops at the first error or leaks a permit.
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
This extraction puts manifest listing, bounded reads, and memory ownership behind the right crate-private stream while preserving tracked_files output and cancellation behavior. Drop-owned permits are a cleaner fit for reuse than consumer-managed accounting.
Contributor
Author
|
cc @wjones127 |
Contributor
Author
|
friendly ping @wjones127 |
Contributor
Author
|
Could you please take a look at this patch when you get a chance, thanks. @wjones127 |
wjones127
self-requested a review
August 17, 2026 14:55
wjones127
approved these changes
Aug 17, 2026
wjones127
left a comment
Contributor
There was a problem hiding this comment.
This looks good! Thanks for working on this.
Contributor
Author
|
Thank you @wjones127 |
LuciferYang
added a commit
to LuciferYang/lance
that referenced
this pull request
Aug 18, 2026
`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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
tracked_fileswalks every present manifest with a four-stage pipeline: a lister that enumerates manifest locations and appliesmin_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_filesin #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_filesis the only caller here and its behavior is unchanged.What moved
The lister and reader now live in
dataset::files::scan, which yields aScannedManifestper present manifest: the manifest, its own path, and the index metadata read alongside it.tracked_fileskeeps its emitter and index lister and consumes that stream. Channel capacities, thecan_launchpredicate, thebiasedselect ordering, and themin_versionfilter 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
MemoryPermitheld byScannedManifestand 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:min_versionreally does skip manifests, which is why a keep-set must leave it unset;totalcounts every manifest the walk yields;Errper manifest rather than being skipped, asserted aserrors == 3becauseerrors > 0would also pass on a reader that stopped at the first failure or on a listing failure;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, andcargo fmt --all --checkare clean;dataset::files(18) anddataset::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.