laskoviymishka commented on code in PR #3309:
URL: https://github.com/apache/iceberg-rust/pull/3309#discussion_r4536250404
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -1168,6 +1209,60 @@ impl Catalog for SqlCatalog {
Ok(builder.build()?)
}
+ async fn unregister_table(&self, table_ident: &TableIdent) ->
Result<Table> {
+ if self.schema_version != SchemaVersion::V1 {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ format!(
+ "Table unregistration is not supported for SQL catalog
schema version {}",
+ self.schema_version
+ ),
+ ));
+ }
+
+ 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 Some(row) = rows.first() else {
+ return no_such_table_err(table_ident);
+ };
+ let metadata_location = row
+ .try_get::<String, _>(CATALOG_FIELD_METADATA_LOCATION_PROP)
+ .map_err(from_sqlx_error)?;
+
+ // read before (re-)moving, this way a failed read keeps the table
registered
+ let metadata = TableMetadata::read_from(&self.fileio,
&metadata_location).await?;
Review Comment:
Could we add a test that pins this order? Point the row at a path with no
metadata file (the `UPDATE ... SET metadata_location = 'new-location'` setup in
`test_unregister_table_rejects_changed_metadata_location` already does that),
call `unregister_table`, and assert it errors and `table_exists` stays true. As
I read it, moving
[`remove_table_at_location`](https://github.com/apache/iceberg-rust/blob/0700dfe51e2a5ea4405830f5fc6887df6659ec5d/crates/catalog/sql/src/catalog.rs#L1247-L1251)
back above `read_from` would pass every unregister test today. A
memory-catalog variant would cover the other backend.
##########
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:
agreed, `rename_table` and `purge_table` already use the same
load-then-`DeleteTable` shape, and
[`DeleteTableInput`](https://docs.rs/aws-sdk-glue/1.153.0/aws_sdk_glue/operation/delete_table/struct.DeleteTableInput.html)
has no version field to make the delete conditional, so fine to leave as is.
##########
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:
Could we make `unregister_table` optional on both `Catalog` and
`SessionCatalog`, with a default body that returns `FeatureUnsupported`? I know
`purge_table` and `register_table` went in as required, but not every catalog
will support unregister:
[hms](https://github.com/apache/iceberg-rust/blob/0700dfe51e2a5ea4405830f5fc6887df6659ec5d/crates/catalog/hms/src/catalog.rs#L745-L750)
and
[s3tables](https://github.com/apache/iceberg-rust/blob/0700dfe51e2a5ea4405830f5fc6887df6659ec5d/crates/catalog/s3tables/src/catalog.rs#L702-L707)
already only stub it, and every out-of-tree impl would need the same stub just
to keep compiling.
--
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]
