fix: error when a path cannot be made relative to the dataset base - #8653
Open
LuciferYang wants to merge 2 commits into
Open
fix: error when a path cannot be made relative to the dataset base#8653LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
`remove_prefix` existed twice, behaviourally identical, in `cleanup.rs` and `files.rs`, and both fell back to the input on a non-matching prefix. That fallback returns an absolute path where every caller expects one relative to the dataset root. `tracked_files` and `all_files` both document their `path` column as "Relative to `base_uri`", so the fallback silently breaks a documented output contract: a consumer joining `base_uri` with an absolute `path` gets a path that resolves nowhere. Erroring is the only honest answer, because the correct root-relative path is not recoverable at that point. Keep one copy, in `files.rs` because `scan.rs` is its child and shares the helper, and make it return `Result`. Name it `strip_prefix` to match `std::path::Path::strip_prefix`, whose fallibility is what a reader expects. `Error::internal` rather than `invalid_input`: the helper is private and every input is derived internally, from `data_dir().join(..)`, `deletion_file_path(..)`, `indices_dir().join(uuid)`, or a listing under `base`. No user-supplied path reaches it, so an unmatched prefix is a broken invariant rather than bad input, and the message should ask for a bug report. Seven of the eight production call sites build a keep-set or a reported path, where dropping an entry is what causes damage, so they propagate with `?`. The eighth, `cleanup_file_if_not_referenced`, classifies a listed object as a deletion candidate, and there the safe answer is the opposite: it logs and returns `Ok(None)`, the value the same function already returns for every other path shape it cannot classify. Aborting there would strand a pass that `remove_stream` has already partly executed, leaving old manifests behind while some of the data files they reference are gone. Covered by `cleanup_skips_listed_files_outside_the_dataset_base`, which installs a store wrapper that reports a rewritten location for the data files and asserts the pass skips the unclassifiable entry and still completes. Restoring `?` at that call site makes it fail. `cargo fmt --all --check`, `cargo clippy -p lance --all-targets -- -D warnings`, and `RUSTDOCFLAGS="-D warnings" cargo doc -p lance --no-deps` are clean; `dataset::files` (20) and `dataset::cleanup` (43) pass. The full suite is left to CI.
Make the skipped-file condition visible at the default log level without
flooding it: the per-file detail stays at `debug!`, and
`delete_unreferenced_files` warns once with a count. A store that
misreports one path misreports every path of that kind, so a per-file
`warn!` would print one line per object on an affected dataset.
Strengthen `cleanup_skips_listed_files_outside_the_dataset_base` so it
discriminates the change it is named for. Rewriting listed data-file
locations to `elsewhere/{filename}` did not: with the removed
absolute-path fallback that value fails the `starts_with("data")` check
and lands in the pre-existing "a .lance file outside the data directory
is left alone" arm, so every assertion held either way. It now rewrites
to `data/outside-base/{filename}`, which is outside the base and still
reads as a collectable data path. The rewritten entry is also stamped a
day before the epoch instead of carrying the fixture's own write time,
which had left it sitting exactly on the `unmodified_since` cutoff and
surviving only because the comparison is `<=`.
Add a test that `build_all_files_batch` refuses a location outside the
base rather than emitting an absolute path, and a near-miss sibling case
pinning that the match is per path segment. Document the new failure mode
on `all_files` and `tracked_files`, whose `path` column is documented as
relative to `base_uri`. Correct `listed_metas`'s doc comment, which
described what the cleanup pass classifies rather than what the helper
returns. Restore the `Vec` reservation that the `extend`-to-`push`
conversion dropped.
Contributor
There was a problem hiding this comment.
The shared segment-aware prefix check restores the documented relative-path contract and, critically, makes cleanup leave an unrelatable listing untouched while reporting the condition. This is the right fail-closed containment for the demonstrated deletion path.
On OpenDAL-backed stores, percent-encoded listed locations can now make all_files and tracked_files fail until #8652 normalizes the bridge output. The author has explicitly accepted and documented that bounded regression for this PR; no further change is requested here.
Contributor
Author
|
cc @wjones127 FYI |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
remove_prefixexisted twice, behaviourally identical, indataset/cleanup.rsanddataset/files.rs, and both fell back to returning the input unchanged whenPath::prefix_matchdid not match. That hands back an absolute path where every caller wants one relative to the dataset root.tracked_filesandall_filesboth document theirpathcolumn as relative tobase_uri, so the fallback silently broke a documented contract: a consumer joining the two gets a path that resolves nowhere. Erroring is the only honest answer, because the correct root-relative path is not recoverable at that point.This is not only tidying. The fallback could destroy data. If a misreported location happens to read as base-relative under
data— for example a base whose own first segment isdata, or the reported path simply starting with it — cleanup's classifier passes it through thestarts_with("data")check, fails to find it in the keep-set, and hands a file the manifest still references to the delete stream. The guard removes that path entirely.Hardening this now, rather than folding it into the work it unblocks, is deliberate. #8097 adds
Dataset::referenced_files, an authoritative keep-set for external distributed cleanup, and it builds that set by inserting this helper's output directly — one entry per referenced data file, per deletion file, and per index prefix. Those inputs are join-derived too, so the fallback is no more reachable there than anywhere else, but the blast radius differs in kind: an absolute path in a keep-set means the real root-relative path is absent from it, and a driver computing the difference against a listing then deletes a file the manifest still references. A helper whose output authorizes deletion should not carry a silent fallback at all.The two copies are now one
strip_prefixreturningResult, kept infiles.rsbecausefiles/scan.rsis its child and shares it.Error::internalrather thaninvalid_input: the helper is private and no user-supplied path reaches it, so a mismatch is a broken invariant rather than bad input.Seven of the eight production call sites propagate with
?. Four of those build their input by joining onto the base and so cannot reach the error at all; the other three pass a location the object store reported from a listing, and there dropping an entry is what causes damage — an omission from a keep-set gets a live file deleted, and an omission from a file inventory gets one deleted by whatever consumes that inventory. The eighth,CleanupTask::cleanup_file_if_not_referenced, is the one site that runs while the pass is already deleting, and there the safe answer is the opposite: it skips the object and returnsOk(None), the value that function already returns for every other shape it cannot classify. Propagating would strand a pass that has already removed files, leaving old manifests behind while some of the data they reference is gone, and it would repeat on every later run. Because a store that misreports one path misreports every path of that kind, the per-file detail stays atdebug!and the pass warns once with a count, so an operator learns at the default log level that files are being left behind without getting one line per object.One user-visible regression to flag rather than bury. On stores Lance reaches through the OpenDAL bridge (
hf://andgoosefs://always,s3:///gs:///az://withuse_opendal=true), listings return percent-encoded locations whileDataset::baseholds the raw string, so for a dataset path containing a character inobject_store's encode set the two never match.all_filesandtracked_filesgo from emitting a wrong-but-present path to failing outright. The root cause is inlance-io's bridge, not in this change, and is filed separately as #8652; this change makes the existing breakage loud instead of silent. Cleanup is unaffected either way, since the absolute path already matched none of its expected prefixes.Deliberately out of scope: two byte-wise prefix strips with the same silent-fallback shape remain in
mem_wal/memtable/flush.rsandlance-table/src/format/index.rs, both unreachable today, andDataFile.path's single-segment invariant is neither documented nor validated at the boundaries that accept it. Both belong with the follow-up that moves the helper somewherelance-tablecan reach it.Test plan
Four
strip_prefixcases cover the relative result, the equal-to-base result, a path under a different dataset, and a near-miss sibling (bucket/dataset2against basebucket/dataset) that pins matching as per path segment rather than per byte.build_all_files_batch_rejects_a_path_outside_the_baseconstructs anObjectMetadirectly and asserts the batch fails naming the offending path, rather than emitting an absolute one.cleanup_skips_listed_files_outside_the_dataset_baseinstalls a store wrapper that reports a rewritten location for the data files and asserts the pass leaves them alone and still completes. The rewrite target isdata/outside-base/{filename}, chosen so the test discriminates in both directions: it is outside the base, so the guard skips it, and it still reads as a collectable data path, so the removed fallback would have handed it to the delete stream. Verified by reverting each half — restoring the fallback fails the test, and replacing the skip with?fails it too. The rewritten entry is stamped a day before the epoch so it clears any cutoff cleanup computes rather than sitting on the boundary, and two count assertions stop the test decaying into a vacuous pass.cargo fmt --all --check,cargo clippy --all --tests --benches -- -D warnings, andRUSTDOCFLAGS="-D warnings" cargo doc -p lance --no-depsare clean.cargo test -p lance --lib dataset::passes, 2232 tests. The full suite is left to CI.