fix: refuse to drop a path that is not a Lance dataset - #8665
Open
xiaguanglei wants to merge 1 commit into
Open
Conversation
xiaguanglei
force-pushed
the
feat/drop-validate-lance-root
branch
from
August 20, 2026 13:13
7611da8 to
39961e0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Dataset.drop recursively deletes whatever path it is handed. It is exposed as a public API in both Python (LanceDataset.drop) and Java (Dataset.drop), and is also used by catalog/table operations such as Spark DROP TABLE / CREATE OR REPLACE TABLE, so a single misconfigured value destroys unrelated data with no way back.
xiaguanglei
force-pushed
the
feat/drop-validate-lance-root
branch
from
August 21, 2026 01:39
39961e0 to
5acb0ed
Compare
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The rebased revision preserves the positive-identity guard: recursive drop is authorized only by a readable committed or detached manifest, or an explicit namespace marker. The new rooted local/UNC object-store handling keeps validation, manifest reads, and deletion scoped to the same store root, so the prior safety conclusion remains supported.
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.
Summary
Dataset.droprecursively deletes whatever path it is handed viaremove_dir_all, with no check that the target is a Lance dataset. The path comes from a public Python/Java API and from catalog-driven callers (e.g. SparkDROP TABLE/CREATE OR REPLACE TABLE), so a mistyped warehouse root or bucket prefix can destroy unrelated data with no recovery.base_uricould wipe out the entire bucket root, affecting all tables and data stored in the same location, with no recovery path.For example, consider:
This currently deletes everything under s3://my-bucket/ — all files, all directories, and all other tables or data stored in that bucket — with no way to recover.The same risk applies to S3, GCS, and other object stores.
This PR adds a fail-closed check immediately before deletion: the path must hold positive Lance evidence — a file under
_versions/that both parses as a manifest location and deserializes as a manifest (committed or detached), or a namespace declare/deregister marker (.lance-reserved/.lance-deregistered). Anything else is rejected withInvalidInput/ValueError/IllegalArgumentException. Missing or empty paths keep today's not-found /ignore_not_foundbehavior.This is intentionally minimal: only
Dataset.drop(Python/Java); namespacedrop_tableis unchanged; create semantics are unchanged; a successful drop still deletes everything under the path.Policy
A path may be dropped when either:
_versions/holds a file that parses as a manifest location (ManifestLocation::try_from/ detached-version naming) andread_manifestsucceeds on it, or.lance-reserved/.lance-deregisteredmarker is present (declared or deregistered, not yet materialized).Nothing weaker qualifies. In particular:
_versions/is not identity (e.g._versions/READMEnext toreports/q1.csvis refused);data/(ordata/+tree/) is refused.Those leftovers look identical to a storage root whose only top-level prefix is
data/— a common layout on object stores — and deleting the wrong one is unrecoverable. Corrupt or uncommitted cleanup is left to an explicit storage-level delete rather than a weaker default guard. Failing closed is cheap: such leftovers do not block re-create (create only refuses a path that already has a manifest), andcleanup_old_versionsremoves data files no manifest references.Unmanaged files next to a committed dataset do not block drop (matching cleanup's allowance for them). Note that the recursive delete still removes those files once validation passes.
Probes list
_versions/, parse candidate names with the existing manifest naming logic, and stop at the first successfulread_manifest, then fall back to markerexistschecks. Dropping a real dataset therefore costs one listing plus one manifest read, and does not list an entire bucket root.Compatibility
remove_dir_all+ existing not-found /ignore_not_foundhandling (OSErrorin Python), not a new validation error.DirectoryNamespace::drop_table/ManifestNamespace::drop_table: untouched (catalog-derived paths; different threat model).Boundaries / non-goals
remove_dir_alldeletes the whole tree, including unmanaged siblings (images/,notes.txt). Making drop selective (only Lance-managed prefixes) would align better with cleanup but is a separate semantic change._versions/under a child table name does not make a parent warehouse look like a dataset (warehouse roots are rejected).Test plan
cargo test -p lance --lib validate_dataset_root_for_drop(committed + unmanaged siblings, detached manifest, markers, warehouse / home / unrelated / data-only / layout-without-manifest, nonempty_versionswithout a readable manifest, unreadable / empty / staging-named manifests, missing path)uv run pytest python/tests/test_dataset.py -k drop(including rejects-paths-without-readable-manifest and create-over-uncommitted-leftovers)./mvnw test -Dtest='DatasetTest#testDropPath+testDropRejectsNonDatasetPath'(2 passed)cargo fmt --all,cargo clippy -p lance --tests -- -D warnings,uv run make lint,./mvnw spotless:check,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps