[GitHub] [iceberg] dennishuo commented on a diff in pull request #6428: Add new SnowflakeCatalog implementation to enable directly using Snowflake-managed Iceberg tables

[GitBox <gi...@apache.org>]

dennishuo commented on code in PR #6428:
URL: https://github.com/apache/iceberg/pull/6428#discussion_r1050257006


##########
snowflake/src/test/java/org/apache/iceberg/snowflake/SnowflakeCatalogTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * 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.snowflake;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.TableMetadata;
+import org.apache.iceberg.TableMetadataParser;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.snowflake.entities.SnowflakeTableMetadata;
+import org.apache.iceberg.types.Types;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class SnowflakeCatalogTest {

Review Comment:
   Good suggestion! I recall skimming over it when looking at the JdbcCatalog's unittest, but saw most of the test cases were for (currently) unsupported functionality. Just for completeness though I went ahead and plugged it in (which helped identify the need to specify `test { useJUnitPlatform() }` in build.gradle to successfully inherit the tests) and verified that they all fail due to depending on at least `buildTable/createNamespace` to set up test data.
   
   One possibility if we want to split out read-only variations even for temporary scenarios, would be to abstract out the mutations into helper methods in the `CatalogTests` base class, and only test cases that explicitly test mutations would call the catalog directly, letting read-only stuff do the mutations in their fake in-memory data model.
   
   It seems like the `supports*` test methods are also pointing towards possibly wanting to extract a more formalized way to define differences in supported functionality between catalogs, perhaps more fine-grained than what things like e.g. `SupportsNamespaces` java interfaces provide.
   
   Is there any work in progress to do something for Catalog interfaces analogous to the [HCFS efforts for HadoopFileSystem](https://cwiki.apache.org/confluence/display/HADOOP2/HCFS) ? 
   
   Seems like CatalogTests may have already been written with the analogous [AbstractFSContractTestBase](https://github.com/apache/hadoop/blob/03cfc852791c14fad39db4e5b14104a276c08e59/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractFSContractTestBase.java) in mind, where we could move towards analogous ["contract" definitions](https://github.com/apache/hadoop/blob/03cfc852791c14fad39db4e5b14104a276c08e59/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/localfs/LocalFSContract.java) and [contract config specs](https://github.com/apache/hadoop/blob/03cfc852791c14fad39db4e5b14104a276c08e59/hadoop-common-project/hadoop-common/src/test/resources/contract/localfs.xml)



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org