harshm-dev commented on a change in pull request #3326:
URL: https://github.com/apache/iceberg/pull/3326#discussion_r740242281



##########
File path: site/docs/spark-configuration.md
##########
@@ -54,7 +54,7 @@ Both catalogs are configured using properties nested under 
the catalog name. Com
 
 | Property                                           | Values                  
      | Description                                                          |
 | -------------------------------------------------- | 
----------------------------- | 
-------------------------------------------------------------------- |
-| spark.sql.catalog._catalog-name_.type              | `hive` or `hadoop`      
      | The underlying Iceberg catalog implementation, `HiveCatalog`, 
`HadoopCatalog` or left unset if using a custom catalog |
+| spark.sql.catalog._catalog-name_.type              | `hive`, `hadoop`, 
`nessie`, `jdbc`, `glue` or `dynamodb`| Represents the underlying Iceberg 
catalog implementation. For example - `HiveCatalog` for `hive` and 
`HadoopCatalog` for `hadoop`.<br>Leave unset if using a custom catalog via the 
conf `catalog-impl`. |

Review comment:
       The hive.md is a good reference. I'll switch to that template.

##########
File path: core/src/main/java/org/apache/iceberg/CatalogType.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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;
+
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.stream.Collectors;
+
+public enum CatalogType {
+  HIVE("org.apache.iceberg.hive.HiveCatalog"),
+  HADOOP("org.apache.iceberg.hadoop.HadoopCatalog"),
+  GLUE("org.apache.iceberg.aws.glue.GlueCatalog"),
+  NESSIE("org.apache.iceberg.nessie.NessieCatalog"),
+  DYNAMODB("org.apache.iceberg.aws.dynamodb.DynamoDbCatalog"),
+  JDBC("org.apache.iceberg.jdbc.JdbcCatalog");
+
+  private final String catalogImpl;
+
+  CatalogType(String catalogImpl) {
+    this.catalogImpl = catalogImpl;
+  }
+
+  public String getTypeName() {
+    return name().toLowerCase(Locale.ENGLISH);
+  }
+
+  public String getCatalogImpl() {
+    return catalogImpl;
+  }
+
+  /**
+   * Returns catalog-impl class name for the input type name.
+   *
+   * @param inputType Non-null input type.
+   * @return catalog-impl class name
+   */
+  public static String getCatalogImpl(String inputType) {

Review comment:
       I'll refactor the static method to CatalogType CatalogType.from(String 
inputType). Since all the consumers were interested only in the mapping (type 
to impl class), my initial thought was to give a direct method to do that.
   But I see your point, and I think it makes sense.

##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestTables.java
##########
@@ -414,14 +415,15 @@ public Table loadTable(TableIdentifier identifier) {
   static class HiveTestTables extends TestTables {
 
     HiveTestTables(Configuration conf, TemporaryFolder temp, String 
catalogName) {
-      super(CatalogUtil.loadCatalog(HiveCatalog.class.getName(), 
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE,
+      super(CatalogUtil.loadCatalog(HiveCatalog.class.getName(), 
CatalogType.HIVE.getTypeName(),
               ImmutableMap.of(), conf), temp, catalogName);
     }
 
     @Override
     public Map<String, String> properties() {
-      return 
ImmutableMap.of(InputFormatConfig.catalogPropertyConfigKey(catalog, 
CatalogUtil.ICEBERG_CATALOG_TYPE),
-          CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
+      return ImmutableMap
+          .of(InputFormatConfig.catalogPropertyConfigKey(catalog, 
CatalogProperties.CATALOG_TYPE),

Review comment:
       Not sure if I got the concern. Is it about the indentation?

##########
File path: core/src/main/java/org/apache/iceberg/CatalogType.java
##########
@@ -0,0 +1,64 @@
+/*
+ * 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;
+
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.stream.Collectors;
+
+public enum CatalogType {
+  HIVE("org.apache.iceberg.hive.HiveCatalog"),
+  HADOOP("org.apache.iceberg.hadoop.HadoopCatalog"),
+  GLUE("org.apache.iceberg.aws.glue.GlueCatalog"),
+  NESSIE("org.apache.iceberg.nessie.NessieCatalog"),
+  DYNAMODB("org.apache.iceberg.aws.dynamodb.DynamoDbCatalog"),
+  JDBC("org.apache.iceberg.jdbc.JdbcCatalog");
+
+  private final String catalogImpl;
+
+  CatalogType(String catalogImpl) {
+    this.catalogImpl = catalogImpl;
+  }
+
+  public String getTypeName() {
+    return name().toLowerCase(Locale.ENGLISH);
+  }
+
+  public String getCatalogImpl() {
+    return catalogImpl;

Review comment:
       I'll refactor the static method to `CatalogType CatalogType.from(String 
inputType)`. Since all the consumers were interested only in the mapping (type 
to impl class), my initial thought was to give a direct method to do that. 
   But I see your point, and I think it makes sense.




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