mirror of
https://github.com/lancedb/lancedb.git
synced 2026-05-26 16:30:41 +00:00
fix(rust): map lance-namespace errors to TableNotFound / TableAlreadyExists (#3385)
## Summary `LanceNamespaceDatabase::open_table` and `create_table` were squashing `NamespaceError::TableNotFound` and `TableAlreadyExists` into generic `Error::Runtime`, so callers couldn't distinguish a missing-table or duplicate-table error from any other internal failure. Downstream this surfaced to geneva-style code as HTTP 500 / "internal server error" on operations that should have been 400/404 — see [ENT-1235](https://linear.app/lancedb/issue/ENT-1235/fix-ns-errors-for-create-tableopen-table). This PR walks the boxed-error chain from `lance::Error::Namespace` down to the inner `NamespaceError` and maps its `ErrorCode` onto the proper `lancedb::Error` variant: - `NamespaceError::TableNotFound` → `Error::TableNotFound { name, source }` - `NamespaceError::TableAlreadyExists` → `Error::TableAlreadyExists { name }` - everything else → `Error::Runtime` (unchanged behavior for the long tail) It also replaces the existing `e.to_string().contains("already exists")` string match in `LanceNamespaceDatabase::create_table` with a downcast on the `NamespaceError` code. That string-match happened to work for the `dir` backend but isn't guaranteed to match the REST namespace backend's error format; the downcast works for both. The chain-walk is needed because `DatasetBuilder::from_namespace` re-wraps the inner namespace error in a fresh `lance::Error::Namespace`, so a single top-level downcast misses it. ## How this helps geneva Geneva's workaround (linked in the parent issue) currently has to use `except Exception:` with a `# todo: this is too broad` comment, plus `str(e).lower().contains("already exists")` string matching, because the namespace-impl path raised a generic `RuntimeError`. After this PR: - `db.open_table("missing")` raises `ValueError("Table 'missing' was not found")` (via the existing Python binding mapping of `TableNotFound` → `PyValueError`) — geneva can catch `ValueError` cleanly. - `db.create_table("dup")` raises `ValueError("Table 'dup' already exists")` reliably across both `dir` and REST backends, so the existing string match becomes deterministic. In phalanx (the sophon REST server), `LanceDBError::TableNotFound` and `LanceDBError::TableAlreadyExists` already map directly to HTTP 404 and HTTP 400 respectively — see [phalanx/src/error.rs:77-94](https://github.com/lancedb/sophon/blob/main/src/rust/phalanx/src/error.rs#L77). No phalanx code change is needed for the bug fix; the previous 500 came from phalanx's string-match fallback not finding `"namespace"` AND `"not found"` in the `Runtime` error's debug-formatted message. ## Follow-up [ENT-1246](https://linear.app/lancedb/issue/ENT-1246/remove-dead-namespace-error-string-matching-in-phalanx) — after this lands and phalanx picks up the new lancedb, the string-matching fallback for table errors in `src/rust/phalanx/src/error.rs` (lines 99-168, 236-256, 502-514) and `src/rust/phalanx/src/rest/table/create_table.rs` (lines 224-241) becomes dead code and can be removed. The `// TODO: Refactor for better namespace error handling` comment at phalanx/src/error.rs:96-98 is exactly what this PR addresses on the lancedb side; ENT-1246 finishes the cleanup on the sophon side. ## Test plan - [x] `cargo test --quiet --features remote -p lancedb --lib` — all 495 lib tests pass, including 4 new tests in `database::namespace::tests`: - `test_namespace_table_not_found` — extended to assert `Error::TableNotFound` (was just `is_err()`) - `test_namespace_open_table_not_found_at_root` — covers the root-namespace path - `test_namespace_create_table_already_exists` — covers child namespace - `test_namespace_create_table_already_exists_at_root` — covers root namespace - [x] `cargo clippy --quiet --features remote --tests` — clean - [x] `cargo fmt --all` — clean - [x] Manually confirmed (via test failures before the fix) that the two `open_table` tests were returning `Error::Runtime { message: "Failed to get table info from namespace: Namespace { source: TableNotFound { ... } }" }` prior to this change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Justin Miller
GitHub
@@ -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