mkroll-db commented on code in PR #3309:
URL: https://github.com/apache/iceberg-rust/pull/3309#discussion_r4530829939


##########
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 is really hard to test, without introducing internal hooks and would be 
a first.
   Is this really needed?



-- 
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]

Reply via email to