diff --git a/rust/lancedb/src/database/namespace.rs b/rust/lancedb/src/database/namespace.rs index de18f8db8..4a0b09e05 100644 --- a/rust/lancedb/src/database/namespace.rs +++ b/rust/lancedb/src/database/namespace.rs @@ -11,6 +11,7 @@ use lance::io::commit::namespace_manifest::LanceNamespaceExternalManifestStore; use lance_io::object_store::{ObjectStoreParams, StorageOptionsAccessor}; use lance_namespace::{ LanceNamespace, + error::{ErrorCode, NamespaceError}, models::{ CreateNamespaceRequest, CreateNamespaceResponse, DeclareTableRequest, DescribeNamespaceRequest, DescribeNamespaceResponse, DescribeTableRequest, @@ -29,7 +30,7 @@ use crate::database::listing::{ OPT_NEW_TABLE_V2_MANIFEST_PATHS, }; use crate::error::{Error, Result}; -use crate::table::NativeTable; +use crate::table::{NativeTable, map_namespace_lance_error}; use lance::dataset::WriteMode; use super::{ @@ -37,6 +38,19 @@ use super::{ Database, OpenTableRequest, TableNamesRequest, }; +/// Returns true if the given `lance::Error` (anywhere in its source chain) is a +/// `NamespaceError::TableAlreadyExists`. +fn is_table_already_exists_namespace_error(err: &lance::Error) -> bool { + let mut current: Option<&(dyn std::error::Error + 'static)> = Some(err); + while let Some(e) = current { + if let Some(ns_err) = e.downcast_ref::() { + return ns_err.code() == ErrorCode::TableAlreadyExists; + } + current = e.source(); + } + false +} + /// A database implementation that uses lance-namespace for table management pub struct LanceNamespaceDatabase { namespace: Arc, @@ -356,13 +370,15 @@ impl Database for LanceNamespaceDatabase { (loc, opts, response.managed_versioning) } Err(e) - if matches!(request.mode, CreateTableMode::Create) && { - let err_str = e.to_string(); - err_str.contains("already exists") - || err_str.contains("TableAlreadyExists") - || err_str.contains("table already exists") - } => + if matches!(request.mode, CreateTableMode::Create) + && is_table_already_exists_namespace_error(&e) => { + // A declare conflict can either mean (a) the table was previously + // *declared* but never written (in which case we should proceed and + // create it), or (b) the table is fully realized (in which case the + // user is creating something that already exists and we should + // surface TableAlreadyExists). Disambiguate by describing the table + // and checking whether it has both a version and a schema. let response = self .namespace .describe_table(DescribeTableRequest { @@ -370,11 +386,8 @@ impl Database for LanceNamespaceDatabase { ..Default::default() }) .await - .map_err(|describe_err| Error::Runtime { - message: format!( - "Failed to describe existing declared table after declare conflict: {}", - describe_err - ), + .map_err(|describe_err| { + map_namespace_lance_error(describe_err, &request.name) })?; if response.version.is_some() && response.schema.is_some() { @@ -394,9 +407,7 @@ impl Database for LanceNamespaceDatabase { (loc, opts, response.managed_versioning) } Err(e) => { - return Err(Error::Runtime { - message: format!("Failed to declare table: {}", e), - }); + return Err(map_namespace_lance_error(e, &request.name)); } } } @@ -1086,8 +1097,120 @@ mod tests { .execute() .await; - // Verify: Should return an error - assert!(result.is_err()); + // Verify: Should return TableNotFound — not a generic Runtime/internal error + // (regression test for ENT-1235: open_table on missing table previously surfaced as + // a generic 500/Runtime error rather than TableNotFound). + match result { + Err(Error::TableNotFound { name, .. }) => { + assert_eq!(name, "non_existent_table"); + } + Err(other) => panic!("Expected TableNotFound, got: {:?}", other), + Ok(_) => panic!("Expected open_table to fail, but it succeeded"), + } + } + + #[tokio::test] + async fn test_namespace_open_table_not_found_at_root() { + // Same as above, but at the root namespace (no parent namespace creation). + // Covers the common code path used by `db.open_table("foo")` without a namespace. + let tmp_dir = tempdir().unwrap(); + let root_path = tmp_dir.path().to_str().unwrap().to_string(); + + let mut properties = HashMap::new(); + properties.insert("root".to_string(), root_path); + + let conn = connect_namespace("dir", properties) + .execute() + .await + .expect("Failed to connect to namespace"); + + let result = conn.open_table("missing_at_root").execute().await; + + match result { + Err(Error::TableNotFound { name, .. }) => { + assert_eq!(name, "missing_at_root"); + } + Err(other) => panic!("Expected TableNotFound, got: {:?}", other), + Ok(_) => panic!("Expected open_table to fail, but it succeeded"), + } + } + + #[tokio::test] + async fn test_namespace_create_table_already_exists() { + // Regression test for ENT-1235: create_table on an existing table (in default + // Create mode) should return TableAlreadyExists, not a generic Runtime/500 error. + let tmp_dir = tempdir().unwrap(); + let root_path = tmp_dir.path().to_str().unwrap().to_string(); + + let mut properties = HashMap::new(); + properties.insert("root".to_string(), root_path); + + let conn = connect_namespace("dir", properties) + .execute() + .await + .expect("Failed to connect to namespace"); + + conn.create_namespace(CreateNamespaceRequest { + id: Some(vec!["test_ns".into()]), + ..Default::default() + }) + .await + .expect("Failed to create namespace"); + + // Create the table once. + conn.create_table("dup_table", create_test_data()) + .namespace(vec!["test_ns".into()]) + .execute() + .await + .expect("Failed to create table the first time"); + + // Try to create it again with the default Create mode. + let result = conn + .create_table("dup_table", create_test_data()) + .namespace(vec!["test_ns".into()]) + .execute() + .await; + + match result { + Err(Error::TableAlreadyExists { name }) => { + assert_eq!(name, "dup_table"); + } + Err(other) => panic!("Expected TableAlreadyExists, got: {:?}", other), + Ok(_) => panic!("Expected create_table to fail, but it succeeded"), + } + } + + #[tokio::test] + async fn test_namespace_create_table_already_exists_at_root() { + // Same as above, but at the root namespace. + let tmp_dir = tempdir().unwrap(); + let root_path = tmp_dir.path().to_str().unwrap().to_string(); + + let mut properties = HashMap::new(); + properties.insert("root".to_string(), root_path); + + let conn = connect_namespace("dir", properties) + .execute() + .await + .expect("Failed to connect to namespace"); + + conn.create_table("dup_root", create_test_data()) + .execute() + .await + .expect("Failed to create table the first time"); + + let result = conn + .create_table("dup_root", create_test_data()) + .execute() + .await; + + match result { + Err(Error::TableAlreadyExists { name }) => { + assert_eq!(name, "dup_root"); + } + Err(other) => panic!("Expected TableAlreadyExists, got: {:?}", other), + Ok(_) => panic!("Expected create_table to fail, but it succeeded"), + } } #[tokio::test] diff --git a/rust/lancedb/src/table.rs b/rust/lancedb/src/table.rs index 29bcaea26..2f340436e 100644 --- a/rust/lancedb/src/table.rs +++ b/rust/lancedb/src/table.rs @@ -36,6 +36,7 @@ pub use query::AnyQuery; use lance::io::commit::namespace_manifest::LanceNamespaceExternalManifestStore; use lance_namespace::LanceNamespace; +use lance_namespace::error::NamespaceError; use lance_namespace::models::DescribeTableRequest; use lance_table::format::Manifest; use lance_table::io::commit::CommitHandler; @@ -94,6 +95,53 @@ pub use schema_evolution::{AddColumnsResult, AlterColumnsResult, DropColumnsResu use serde_with::skip_serializing_none; pub use update::{UpdateBuilder, UpdateResult}; +/// Walk a boxed error chain to find the innermost `NamespaceError`. +/// +/// Callers like `DatasetBuilder::from_namespace` re-wrap their inner namespace error +/// inside a fresh `lance::Error::Namespace`, so a single downcast at the top level +/// won't find it. This walks `.source()` to unwrap arbitrarily nested layers. +fn find_namespace_error<'a>( + err: &'a (dyn std::error::Error + 'static), +) -> Option<&'a NamespaceError> { + let mut current: Option<&(dyn std::error::Error + 'static)> = Some(err); + while let Some(e) = current { + if let Some(ns_err) = e.downcast_ref::() { + return Some(ns_err); + } + current = e.source(); + } + None +} + +/// Map a `lance::Error` coming from a `lance-namespace` call into a `lancedb::Error`, +/// preserving the fine-grained namespace error code (e.g. `TableNotFound`, +/// `TableAlreadyExists`). Errors that aren't recognized namespace error variants fall +/// through to a generic runtime error rather than `TableNotFound`/`TableAlreadyExists`. +pub(crate) fn map_namespace_lance_error(err: lance::Error, table_name: &str) -> Error { + if let Some(code) = find_namespace_error(&err).map(NamespaceError::code) { + match code { + lance_namespace::error::ErrorCode::TableNotFound => { + return Error::TableNotFound { + name: table_name.to_string(), + source: Box::new(err), + }; + } + lance_namespace::error::ErrorCode::TableAlreadyExists => { + return Error::TableAlreadyExists { + name: table_name.to_string(), + }; + } + _ => {} + } + } + match err { + lance::Error::Namespace { source, .. } => Error::Runtime { + message: format!("Namespace error: {}", source), + }, + other => other.into(), + } +} + /// Defines the type of column #[derive(Debug, Clone, Serialize, Deserialize)] pub enum ColumnKind { @@ -1494,12 +1542,7 @@ impl NativeTable { // and storage options from the namespace let builder = DatasetBuilder::from_namespace(namespace_client.clone(), table_id) .await - .map_err(|e| match e { - lance::Error::Namespace { source, .. } => Error::Runtime { - message: format!("Failed to get table info from namespace: {:?}", source), - }, - e => e.into(), - })?; + .map_err(|e| map_namespace_lance_error(e, name))?; let dataset = builder .with_read_params(params)