fix: table_names should not list loose .lance files as tables (#4203)

`table_names()` enumerates with `read_dir`, which returns files as well
as directories, filtered only on the `.lance` extension — so a loose
`foo.lance` *file* at the database prefix is reported as a table.
`list_tables()` already applies the correct rule ("Only child
directories can be tables"), and
`test_listing_ignores_non_table_children` documents it, so the two
listing APIs currently disagree.

The practical damage is in `drop_all_tables()`, which is built on
`table_names`: with a loose `.lance` file present it deletes some real
tables, fails on the file with `os error 20 (NotADirectory)`, and then
fails the same way on every retry — the database can never be emptied
through the API, and the partial drop is not recoverable.

`table_names` now lists via `list_with_delimiter` and filters
`common_prefixes` through the same `table_name` helper `list_tables`
uses; the sort/`start_after`/`limit` handling is unchanged.
`drop_tables` additionally attempts every name before returning, still
reporting the first error unchanged (kept rather than an aggregate error
so the mapped exception types in the bindings stay exactly as they are).
Extended the non-table-children test to cover `table_names`, plus tests
for the drop behavior — all three fail without the fix. Full `cargo test
-p lancedb --lib --features remote` 1320 passed on the repo-pinned 1.97
toolchain; clippy/fmt clean.
This commit is contained in:
Joaquin Hui Gomez
2026-10-23 01:25:43 +08:00
committed by GitHub
parent 9a59091376
commit 82bfe7d38e
+94 -26
View File
@@ -750,26 +750,39 @@ impl ListingDatabase {
uri.push_str(&format!("?{}", query_string));
}
let commit_handler = commit_handler_from_url(&uri, &Some(object_store_params)).await?;
// One name that cannot be dropped must not strand the names after it: a caller
// dropping every table would otherwise be left with a partly dropped database that
// the same call cannot finish. Every name is attempted and the first failure is
// still what the call reports, so a caller matching on the error sees what it saw
// before.
let mut first_error = None;
for name in names {
let dir_name = format!("{}.{}", name, LANCE_EXTENSION);
let full_path = self.base_path.clone().join(dir_name.clone());
commit_handler.delete(&full_path).await?;
let dropped: Result<()> = async {
commit_handler.delete(&full_path).await?;
self.object_store
.remove_dir_all(full_path.clone())
.await
.map_err(|err| match err {
// this error is not lance::Error::DatasetNotFound, as the method
// `remove_dir_all` may be used to remove something not be a dataset
lance::Error::NotFound { .. } => Error::TableNotFound {
name: name.clone(),
source: Box::new(err),
},
_ => Error::from(err),
})?;
self.object_store
.remove_dir_all(full_path.clone())
.await
.map_err(|err| match err {
// this error is not lance::Error::DatasetNotFound, as the method
// `remove_dir_all` may be used to remove something not be a dataset
lance::Error::NotFound { .. } => Error::TableNotFound {
name: name.clone(),
source: Box::new(err),
},
_ => Error::from(err),
})
}
.await;
if let Err(err) = dropped {
first_error.get_or_insert(err);
}
}
Ok(())
first_error.map_or(Ok(()), Err)
}
/// Inherit storage options from the connection into the target map
@@ -930,20 +943,18 @@ impl Database for ListingDatabase {
if !request.namespace_path.is_empty() {
return self.namespace_database().table_names(request).await;
}
let mut f = self
// Only child directories can be tables, the same rule `list_tables` lists by: a
// loose object named like a table is not one, and a name handed back for it is a
// name the caller can neither open nor drop.
let dir_suffix = format!(".{LANCE_EXTENSION}");
let listing = self
.object_store
.read_dir(self.base_path.clone())
.await?
.list_with_delimiter(Some(&self.base_path))
.await?;
let mut f = listing
.common_prefixes
.iter()
.map(Path::new)
.filter(|path| {
let is_lance = path
.extension()
.and_then(|e| e.to_str())
.map(|e| e == LANCE_EXTENSION);
is_lance.unwrap_or(false)
})
.filter_map(|p| p.file_stem().and_then(|s| s.to_str().map(String::from)))
.filter_map(|location| table_name(location, &dir_suffix))
.collect::<Vec<String>>();
f.sort();
if let Some(start_after) = request.start_after {
@@ -1697,6 +1708,63 @@ mod tests {
.unwrap();
assert_eq!(page.tables, vec!["real"]);
// The deprecated name listing answers the same question and so follows the same
// rule: a name it hands back has to be one a caller can open and drop.
#[allow(deprecated)]
let names = db.table_names(TableNamesRequest::default()).await.unwrap();
assert_eq!(names, vec!["real"]);
}
/// Dropping the database drops its tables and leaves everything else alone. A loose
/// `*.lance` object under the database prefix is not a table, so it is neither dropped
/// nor allowed to strand the tables that are.
#[tokio::test]
async fn test_drop_all_tables_leaves_loose_lance_files() {
let (tempdir, db) = setup_database().await;
create_tables(&db, &["a", "b"]).await;
// Sorts between the two tables, so a drop that stops at the first failure would
// leave `b` behind.
let loose = tempdir.path().join("aaa-loose.lance");
std::fs::write(&loose, b"not a table").unwrap();
db.drop_all_tables(&[]).await.unwrap();
#[allow(deprecated)]
let names = db.table_names(TableNamesRequest::default()).await.unwrap();
assert!(names.is_empty(), "a table survived the drop: {names:?}");
assert!(!tempdir.path().join("a.lance").exists());
assert!(!tempdir.path().join("b.lance").exists());
assert!(
loose.exists(),
"a loose object was dropped as if it were a table"
);
}
/// A name that cannot be dropped does not strand the names after it: every name is
/// attempted, and the failure is still what the call reports.
#[tokio::test]
async fn test_drop_tables_attempts_every_name() {
let (tempdir, db) = setup_database().await;
create_tables(&db, &["a", "b"]).await;
let result = db
.drop_tables(vec![
"a".to_string(),
"missing".to_string(),
"b".to_string(),
])
.await;
assert!(
matches!(result, Err(Error::TableNotFound { .. })),
"dropping a table that is not there did not report it: {result:?}"
);
assert!(!tempdir.path().join("a.lance").exists());
assert!(
!tempdir.path().join("b.lance").exists(),
"the table after the failing name was left behind"
);
}
#[tokio::test]