RussellSpitzer commented on code in PR #15614: URL: https://github.com/apache/iceberg/pull/15614#discussion_r2957167507
########## spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestRestDropPurgeTable.java: ########## @@ -0,0 +1,172 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.spark.sql; + +import static org.apache.iceberg.CatalogProperties.CATALOG_IMPL; +import static org.apache.iceberg.CatalogProperties.REST_CATALOG_PURGE; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Map; +import org.apache.iceberg.Parameter; +import org.apache.iceberg.ParameterizedTestExtension; +import org.apache.iceberg.Parameters; +import org.apache.iceberg.Schema; +import org.apache.iceberg.Table; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.inmemory.InMemoryCatalog; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.rest.RESTCatalog; +import org.apache.iceberg.spark.SparkCatalog; +import org.apache.iceberg.spark.TestBase; +import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException; +import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException; +import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException; +import org.apache.spark.sql.connector.catalog.Column; +import org.apache.spark.sql.connector.catalog.Identifier; +import org.apache.spark.sql.types.DataTypes; +import org.apache.spark.sql.util.CaseInsensitiveStringMap; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.TestTemplate; +import org.junit.jupiter.api.extension.ExtendWith; + +/** + * Tests for the REST catalog purge delegation feature. + * + * <p>This test verifies that when {@link org.apache.iceberg.CatalogProperties#REST_CATALOG_PURGE} + * is enabled, Spark delegates DROP TABLE PURGE operations to the REST catalog instead of performing + * client-side file deletion. This allows REST catalogs to implement features like UNDROP by + * preserving table metadata. + * + * <p>The test uses a mock REST catalog that verifies the purge flag is correctly passed through + * based on the configuration. + */ +@ExtendWith(ParameterizedTestExtension.class) +public class TestRestDropPurgeTable extends TestBase { Review Comment: Two comments on the test 1. I'm not sure this is the right place, it probably should be in o.a.i.spark (the parent package) because it's not really sql related 2. We need coverage for SparkSessionCatalog as well (i'm not sure we actually covered it's behavior in the above changes, Although you could argue SparkSessionCatalog shouldn't be used with a Rest Catalog which I think is also fair. If that's the case you should be sure to have an error or something if delegation is set and SparkSessionCatalog is used. It probably makes sense to always have that as an assertion in Spark Catalog too. If the property is true, you must be using a rest catalog. -- 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]
