From 82bfe7d38e38285424e843f2491b9680c59c30d2 Mon Sep 17 00:00:00 2001 From: Joaquin Hui Gomez <132194176+joaquinhuigomez@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:25:43 +0100 Subject: [PATCH] fix: table_names should not list loose .lance files as tables (#4203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- rust/lancedb/src/database/listing.rs | 120 +++++++++++++++++++++------ 1 file changed, 94 insertions(+), 26 deletions(-) diff --git a/rust/lancedb/src/database/listing.rs b/rust/lancedb/src/database/listing.rs index 71e9f0ef1..700c855dd 100644 --- a/rust/lancedb/src/database/listing.rs +++ b/rust/lancedb/src/database/listing.rs @@ -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::>(); 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]