laskoviymishka commented on code in PR #3309:
URL: https://github.com/apache/iceberg-rust/pull/3309#discussion_r4153168760
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -1168,6 +1208,52 @@ impl Catalog for SqlCatalog {
Ok(builder.build()?)
}
+ async fn unregister_table(&self, table_ident: &TableIdent) ->
Result<Table> {
+ let rows = self
+ .fetch_rows(
+ &format!(
+ "SELECT {CATALOG_FIELD_METADATA_LOCATION_PROP}
+ FROM {CATALOG_TABLE_NAME}
+ WHERE {CATALOG_FIELD_CATALOG_NAME} = ?
+ AND {CATALOG_FIELD_TABLE_NAME} = ?
+ AND {CATALOG_FIELD_TABLE_NAMESPACE} = ?
+ {}",
+ self.schema_version.record_type_filter()
+ ),
+ vec![
+ Some(&self.name),
+ Some(table_ident.name()),
+ Some(&table_ident.namespace().join(".")),
+ ],
+ )
+ .await?;
+ let row = rows.first().ok_or_else(|| {
+ Error::new(
+ ErrorKind::TableNotFound,
+ format!("No such table: {table_ident}"),
+ )
+ })?;
+ let metadata_location = row
+ .try_get::<String, _>(CATALOG_FIELD_METADATA_LOCATION_PROP)
+ .map_err(from_sqlx_error)?;
+
+ self.remove_table_at_location(table_ident, &metadata_location)
Review Comment:
This removes the catalog entry before reading the metadata that populates
the return value, so a transient read failure here leaves the table permanently
unregistered with no recovery — the caller gets an error and never sees the
`metadata_location` it would need to re-register. That's the opposite of the
safety unregister is meant to buy over `drop_table`.
I'd read and validate the metadata first, then call
`remove_table_at_location`. `register_table` already reads-before-inserts for
exactly this reason, and the 0-rows conflict path stays intact:
```rust
let metadata = TableMetadata::read_from(&self.fileio,
&metadata_location).await?;
self.remove_table_at_location(table_ident, &metadata_location).await?;
```
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -121,6 +121,12 @@ pub trait Catalog: Debug + Sync + Send {
/// Register an existing table to the catalog.
async fn register_table(&self, table: &TableIdent, metadata_location:
String) -> Result<Table>;
+ /// Unregister a table without deleting its data or metadata files.
+ ///
+ /// The returned table contains the last committed metadata and its
location,
+ /// which can be passed to [`Catalog::register_table`] to register it
again.
+ async fn unregister_table(&self, table: &TableIdent) -> Result<Table>;
Review Comment:
Adding this as a required method with no default is a breaking change for
every downstream crate implementing `Catalog` (and `SessionCatalog` in
session.rs) — they all stop compiling. Since three of the six in-tree backends
only return `FeatureUnsupported`, a default `Err(FeatureUnsupported,
"unregister_table is not supported by this catalog")` is the natural baseline:
the glue/hms/s3tables stubs collapse into it, and external impls keep building.
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -399,6 +399,26 @@ impl Catalog for MemoryCatalog {
builder.build()
}
+ async fn unregister_table(&self, table_ident: &TableIdent) ->
Result<Table> {
+ let metadata_location = {
+ let mut root_namespace_state =
self.root_namespace_state.lock().await;
+ root_namespace_state.remove_existing_table(table_ident)?
Review Comment:
Same inversion as the SQL path: `remove_existing_table` drops the entry and
releases the lock before `read_from` runs, so a failed read leaves the table
irrecoverably unregistered. I'd get the metadata_location under the lock, read
the metadata while still holding it (as `load_table_from_locked_state` does),
then remove — the entry only disappears once the file is confirmed readable.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -1168,6 +1208,52 @@ impl Catalog for SqlCatalog {
Ok(builder.build()?)
}
+ async fn unregister_table(&self, table_ident: &TableIdent) ->
Result<Table> {
+ let rows = self
+ .fetch_rows(
+ &format!(
+ "SELECT {CATALOG_FIELD_METADATA_LOCATION_PROP}
+ FROM {CATALOG_TABLE_NAME}
+ WHERE {CATALOG_FIELD_CATALOG_NAME} = ?
+ AND {CATALOG_FIELD_TABLE_NAME} = ?
+ AND {CATALOG_FIELD_TABLE_NAMESPACE} = ?
+ {}",
+ self.schema_version.record_type_filter()
+ ),
+ vec![
+ Some(&self.name),
+ Some(table_ident.name()),
+ Some(&table_ident.namespace().join(".")),
+ ],
+ )
+ .await?;
+ let row = rows.first().ok_or_else(|| {
+ Error::new(
+ ErrorKind::TableNotFound,
+ format!("No such table: {table_ident}"),
Review Comment:
Every other `TableNotFound` in this file goes through
`no_such_table_err(table_ident)`, which formats with Debug — this one uses
Display, so we end up with two different message strings for the same semantic
error. Use the helper (it's already imported).
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -1944,6 +1964,35 @@ pub(crate) mod tests {
);
}
+ #[tokio::test]
+ async fn test_unregister_table() {
+ let catalog = new_memory_catalog().await;
+ let namespace = NamespaceIdent::new("unregister_namespace".into());
+ create_namespace(&catalog, &namespace).await;
+ let table_ident = TableIdent::new(namespace,
"unregister_table".into());
+ create_table(&catalog, &table_ident).await;
+
+ let before = catalog.load_table(&table_ident).await.unwrap();
+ let unregistered =
catalog.unregister_table(&table_ident).await.unwrap();
+ assert_eq!(unregistered.identifier(), &table_ident);
+ assert_eq!(unregistered.metadata_location(),
before.metadata_location());
+ assert_eq!(unregistered.metadata(), before.metadata());
+ assert!(unregistered.readonly());
+ assert!(!catalog.table_exists(&table_ident).await.unwrap());
+
+ let err = catalog.unregister_table(&table_ident).await.unwrap_err();
+ assert_eq!(err.kind(), ErrorKind::TableNotFound);
+
+ catalog
+ .register_table(
+ &table_ident,
+ unregistered.metadata_location().unwrap().to_string(),
+ )
+ .await
+ .unwrap();
+ assert!(catalog.table_exists(&table_ident).await.unwrap());
Review Comment:
The re-registration here proves the catalog entry can come back, but not
that the metadata file was left untouched — which is the actual contract of
unregister. I'd add a `file_io.exists(metadata_location)` assertion after the
unregister (or a `drop_table` + read-back), so a future refactor that
accidentally wires up file deletion gets caught.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -1168,6 +1208,52 @@ impl Catalog for SqlCatalog {
Ok(builder.build()?)
}
+ async fn unregister_table(&self, table_ident: &TableIdent) ->
Result<Table> {
Review Comment:
`register_table` guards non-V1 schemas with an early `FeatureUnsupported`;
this path has no equivalent. If `record_type_filter()` behaves differently on
V2, the SELECT can miss an existing row and return `TableNotFound` for a table
that's actually there. I'd add the same guard, or add a V2 test that proves the
round-trip.
##########
crates/catalog/glue/src/catalog.rs:
##########
@@ -897,6 +897,13 @@ impl Catalog for GlueCatalog {
Ok(builder.build()?)
}
+ async fn unregister_table(&self, _table_ident: &TableIdent) ->
Result<Table> {
Review Comment:
Glue can do this natively — `delete_table` drops the catalog pointer without
touching files, so unregister here is `load_table` + `delete_table` returning
the read-only table, the same shape as the SQL path. Worth implementing rather
than stubbing. For hms/s3tables the stub is fine, but I'd drop "yet" from the
message so we're not implying a roadmap commitment we haven't made.
##########
crates/catalog/rest/src/types.rs:
##########
@@ -231,6 +231,16 @@ pub struct LoadTableResult {
pub storage_credentials: Option<Vec<StorageCredential>>,
}
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
+#[serde(rename_all = "kebab-case")]
+/// Result returned when a table is successfully unregistered from a catalog.
+pub struct UnregisterTableResult {
Review Comment:
Callers of `unregister_table` only ever get a `Table` back — they never
construct or inspect `UnregisterTableResult`. I'd make it `pub(crate)` so we're
not permanently committing an internal HTTP-parsing DTO to the stable API.
(`CommitTableResponse` being public is the existing precedent, but I'm not
convinced that one's right either.)
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -1465,6 +1551,78 @@ mod tests {
new_sql_catalog(warehouse_loc.clone(), Some("iceberg")).await;
}
+ #[tokio::test]
+ async fn test_unregister_table() {
+ let catalog = new_sql_catalog(temp_path(), Some("iceberg")).await;
+ let namespace = NamespaceIdent::new("unregister_namespace".into());
+ create_namespace(&catalog, &namespace).await;
+ let table_ident = TableIdent::new(namespace,
"unregister_table".into());
+ create_table(&catalog, &table_ident).await;
+
+ let before = catalog.load_table(&table_ident).await.unwrap();
+ let unregistered =
catalog.unregister_table(&table_ident).await.unwrap();
+ assert_eq!(unregistered.identifier(), &table_ident);
+ assert_eq!(unregistered.metadata_location(),
before.metadata_location());
+ assert_eq!(unregistered.metadata(), before.metadata());
+ assert!(unregistered.readonly());
+ assert!(!catalog.table_exists(&table_ident).await.unwrap());
+
+ let err = catalog.unregister_table(&table_ident).await.unwrap_err();
+ assert_eq!(err.kind(), ErrorKind::TableNotFound);
+ }
+
+ #[tokio::test]
+ async fn test_unregister_table_rejects_changed_metadata_location() {
+ let db_path = temp_path();
+ let uri = format!("sqlite:{db_path}");
+ sqlx::Sqlite::create_database(&uri).await.unwrap();
+ let catalog = SqlCatalogBuilder::default()
+ .with_storage_factory(Arc::new(LocalFsStorageFactory))
+ .load(
+ "iceberg",
+ HashMap::from([
+ (SQL_CATALOG_PROP_URI.to_string(), uri),
+ (SQL_CATALOG_PROP_WAREHOUSE.to_string(), temp_path()),
+ ]),
+ )
+ .await
+ .unwrap();
+ let namespace = NamespaceIdent::new("unregister_conflict".into());
+ create_namespace(&catalog, &namespace).await;
+ let table_ident = TableIdent::new(namespace, "table".into());
+ create_table(&catalog, &table_ident).await;
+
+ let stale_location = catalog
+ .load_table(&table_ident)
+ .await
+ .unwrap()
+ .metadata_location()
+ .unwrap()
+ .to_string();
+ catalog
+ .execute(
+ "UPDATE iceberg_tables SET metadata_location = ?
+ WHERE catalog_name = ? AND table_namespace = ? AND table_name
= ?",
+ vec![
+ Some("new-location"),
+ Some("iceberg"),
+ Some("unregister_conflict"),
+ Some("table"),
+ ],
+ None,
+ )
+ .await
+ .unwrap();
+
+ let err = catalog
+ .remove_table_at_location(&table_ident, &stale_location)
Review Comment:
This exercises the conflict path by calling the private
`remove_table_at_location` directly, so the real end-to-end race (SELECT →
concurrent update → CAS DELETE) never runs through `unregister_table`. Once the
read-before-remove swap lands, I'd point this at the public `unregister_table`
so it covers the actual sequence a caller hits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
