mirror of
https://github.com/lancedb/lancedb.git
synced 2026-05-14 02:20:40 +00:00
fix(rust): map lance-namespace errors to TableNotFound / TableAlreadyExists
`LanceNamespaceDatabase::open_table` and `create_table` were squashing
`NamespaceError::TableNotFound` and `TableAlreadyExists` into generic
`Error::Runtime`, forcing callers (and downstream services like phalanx
in sophon) to fall back on fragile string matching to detect these
cases. This surfaced to users as HTTP 500 / "internal server error" on
operations that should have been 400/404 (see ENT-1235).
Walks the boxed-error chain from `lance::Error::Namespace` down to the
inner `NamespaceError` and maps its `ErrorCode` to the corresponding
`lancedb::Error` variant. Also replaces the brittle
`e.to_string().contains("already exists")` check in
`LanceNamespaceDatabase::create_table` with a downcast on the
`NamespaceError` code, so the create-after-declare-conflict path works
the same way across `dir` and REST namespace backends.
Tests pin the desired behavior at both the root namespace and inside a
child namespace, for both open_table and create_table.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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::<NamespaceError>() {
|
||||
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<dyn LanceNamespace>,
|
||||
@@ -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]
|
||||
|
||||
@@ -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::<NamespaceError>() {
|
||||
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)
|
||||
|
||||
Reference in New Issue
Block a user