mirror of
https://github.com/lancedb/lancedb.git
synced 2026-08-18 12:08:35 +00:00
fix: disable manifest for commit-engine URIs on default connect
Connecting to an external commit-engine URI (e.g. `s3+ddb://...`) went through the directory namespace with manifest listing enabled by default. That opens a `__manifest` table under the root, whose URI has no `ddbTableName` query, so building its commit handler fails with "`s3+ddb://` scheme and expects exactly one query `ddbTableName`". Lance v8 silently swallowed that error and fell back to directory listing, so the connect succeeded. Lance v9's directory namespace only falls back on not-found errors and propagates everything else, turning the pre-existing incompatibility into a hard failure (surfaced by `test_concurrent_dynamodb_commit`). Disable the manifest namespace when a commit engine is in use, matching the explicit `manifest_enabled(true)` path which already rejects these URIs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1472,6 +1472,33 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
// A default connection to a commit-engine URI must disable the manifest
|
||||
// namespace: the `__manifest` table under the root has no `ddbTableName`
|
||||
// query, so building its commit handler would fail. See the DynamoDB
|
||||
// regression fixed alongside the Lance v9 bump.
|
||||
#[cfg(feature = "dynamodb")]
|
||||
#[tokio::test]
|
||||
async fn test_default_connect_disables_manifest_for_commit_engine() {
|
||||
for uri in [
|
||||
"s3+ddb://bucket/db?ddbTableName=manifest",
|
||||
"s3://bucket/db?engine=ddb&ddbTableName=manifest",
|
||||
] {
|
||||
let db = connect(uri)
|
||||
.storage_option("region", "us-east-1")
|
||||
.execute()
|
||||
.await
|
||||
.unwrap_or_else(|e| panic!("expected {uri} to connect, got {e:?}"));
|
||||
|
||||
let (ns_impl, properties) = db.namespace_client_config().await.unwrap();
|
||||
assert_eq!(ns_impl, "dir");
|
||||
assert_eq!(
|
||||
properties.get("manifest_enabled"),
|
||||
Some(&"false".to_string()),
|
||||
"manifest should be disabled for commit-engine URI {uri}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_manifest_enabled_connection_migrates_root_listing_table() {
|
||||
let tmp_dir = tempdir().unwrap();
|
||||
|
||||
@@ -323,12 +323,21 @@ impl ListingDatabase {
|
||||
namespace_client_properties: HashMap<String, String>,
|
||||
read_consistency_interval: Option<std::time::Duration>,
|
||||
session: Arc<lance::session::Session>,
|
||||
uses_commit_engine: bool,
|
||||
) -> Result<Arc<LanceNamespaceDatabase>> {
|
||||
let ns_properties = Self::build_namespace_client_properties(
|
||||
let mut ns_properties = Self::build_namespace_client_properties(
|
||||
uri,
|
||||
&storage_options,
|
||||
namespace_client_properties,
|
||||
);
|
||||
// The directory namespace enables manifest-based listing by default, which
|
||||
// opens a `__manifest` table under the root. That is incompatible with an
|
||||
// external commit engine (e.g. `s3+ddb://`): the manifest table URI has no
|
||||
// `ddbTableName` query, so building its commit handler fails. Disable the
|
||||
// manifest so these connections fall back to directory listing.
|
||||
if uses_commit_engine {
|
||||
ns_properties.insert("manifest_enabled".to_string(), "false".to_string());
|
||||
}
|
||||
Ok(Arc::new(
|
||||
LanceNamespaceDatabase::connect(
|
||||
"dir",
|
||||
@@ -503,6 +512,12 @@ impl ListingDatabase {
|
||||
}
|
||||
}
|
||||
|
||||
// A commit engine is in use either via an explicit `engine=` query
|
||||
// param or a `<scheme>+<engine>` URI scheme (e.g. `s3+ddb`). The
|
||||
// manifest namespace is incompatible with these, so track it to
|
||||
// disable the manifest below.
|
||||
let uses_commit_engine = engine.is_some() || url.scheme().contains('+');
|
||||
|
||||
// Filter out the commit store query param -- it's a lancedb param
|
||||
url.query_pairs_mut().clear();
|
||||
url.query_pairs_mut().extend_pairs(filtered_querys);
|
||||
@@ -575,6 +590,7 @@ impl ListingDatabase {
|
||||
request.namespace_client_properties.clone(),
|
||||
request.read_consistency_interval,
|
||||
session.clone(),
|
||||
uses_commit_engine,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -629,6 +645,7 @@ impl ListingDatabase {
|
||||
namespace_client_properties,
|
||||
read_consistency_interval,
|
||||
session.clone(),
|
||||
false,
|
||||
)
|
||||
.await?;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user