geruh commented on code in PR #2232:
URL: https://github.com/apache/iceberg-rust/pull/2232#discussion_r2984242753
##########
crates/catalog/s3tables/src/catalog.rs:
##########
@@ -562,15 +562,18 @@ impl Catalog for S3TablesCatalog {
Ok(self.load_table_with_version_token(table_ident).await?.0)
}
- /// Drops an existing table from the s3tables catalog.
+ /// Not supported for S3Tables. Use `purge_table` instead.
///
- /// Validates the table identifier and then deletes the corresponding
- /// table from the s3tables catalog.
- ///
- /// This function can return an error in the following situations:
- /// - Errors from the underlying database deletion process, converted using
- /// `from_aws_sdk_error`.
- async fn drop_table(&self, table: &TableIdent) -> Result<()> {
+ /// S3 Tables doesn't support soft delete, so dropping a table will
permanently remove it from the catalog.
+ async fn drop_table(&self, _table: &TableIdent) -> Result<()> {
+ Err(Error::new(
Review Comment:
Nice this aligns with:
https://github.com/awslabs/s3-tables-catalog/blob/main/src/software/amazon/s3tables/iceberg/S3TablesCatalog.java#L459
##########
crates/catalog/loader/tests/table_suite.rs:
##########
@@ -274,3 +274,80 @@ async fn test_catalog_drop_table_missing_errors(#[case]
kind: CatalogKind) -> Re
assert!(catalog.drop_table(&table_ident).await.is_err());
Ok(())
}
+
+// Common behavior: purge_table removes the table from the catalog.
+#[rstest]
+#[case::rest_catalog(CatalogKind::Rest)]
+#[case::glue_catalog(CatalogKind::Glue)]
+#[case::hms_catalog(CatalogKind::Hms)]
+#[case::sql_catalog(CatalogKind::Sql)]
+#[case::s3tables_catalog(CatalogKind::S3Tables)]
+#[case::memory_catalog(CatalogKind::Memory)]
+#[tokio::test]
+async fn test_catalog_purge_table(#[case] kind: CatalogKind) -> Result<()> {
+ let Some(harness) = load_catalog(kind).await else {
+ return Ok(());
+ };
+ let catalog = harness.catalog;
+ let namespace = NamespaceIdent::new(normalize_test_name_with_parts!(
+ "catalog_purge_table",
+ harness.label
+ ));
+
+ cleanup_namespace_dyn(catalog.as_ref(), &namespace).await;
+ catalog.create_namespace(&namespace, HashMap::new()).await?;
+
+ let table_name = normalize_test_name_with_parts!("catalog_purge_table",
harness.label, "table");
+ let table = catalog
+ .create_table(&namespace, table_creation(table_name))
+ .await?;
+ let ident = table.identifier().clone();
+
+ assert!(catalog.table_exists(&ident).await?);
+
+ // Capture metadata location and file_io before purge so we can verify
+ // that the underlying files are actually deleted.
+ let metadata_location = table.metadata_location().map(|s| s.to_string());
+ let file_io = table.file_io().clone();
+
+ catalog.purge_table(&ident).await?;
+ assert!(!catalog.table_exists(&ident).await?);
+
+ if let Some(location) = &metadata_location {
+ assert!(
+ !file_io.exists(location).await?,
+ "Metadata file should have been deleted after purge"
+ );
+ }
+
+ catalog.drop_namespace(&namespace).await?;
+
+ Ok(())
+}
+
+// Common behavior: purging a missing table should error.
+#[rstest]
+#[case::rest_catalog(CatalogKind::Rest)]
+#[case::glue_catalog(CatalogKind::Glue)]
+#[case::hms_catalog(CatalogKind::Hms)]
+#[case::sql_catalog(CatalogKind::Sql)]
+#[case::s3tables_catalog(CatalogKind::S3Tables)]
+#[case::memory_catalog(CatalogKind::Memory)]
+#[tokio::test]
+async fn test_catalog_purge_table_missing_errors(#[case] kind: CatalogKind) ->
Result<()> {
+ let Some(harness) = load_catalog(kind).await else {
+ return Ok(());
+ };
+ let catalog = harness.catalog;
+ let namespace = NamespaceIdent::new(normalize_test_name_with_parts!(
+ "catalog_purge_table_missing_errors",
+ harness.label
+ ));
+
+ cleanup_namespace_dyn(catalog.as_ref(), &namespace).await;
Review Comment:
Just a heads up but, looks like
[`cleanup_namespace_dyn`](https://github.com/CTTY/iceberg-rust/blob/main/crates/catalog/loader/tests/common/mod.rs#L338)
calls `drop_table`, and with the new logic from S3Tables we would get the
unsupported exception. The error gets swallowed, but worth running with the
S3Tables creds to make sure. Might also be worth switching cleanup to use purge
instead.
--
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]
