kbendick commented on a change in pull request #3326: URL: https://github.com/apache/iceberg/pull/3326#discussion_r733265003
########## File path: core/src/main/java/org/apache/iceberg/CatalogType.java ########## @@ -0,0 +1,61 @@ +/* + * 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; + } + + public static String getCatalogImpl(String inputType) { + if (inputType == null) { + return null; + } Review comment: Question: Is this an error state? When would it be valid to pass `null` in here and then just get `null` back? In my opinion, it's easier for the caller to check for null before passing things in than it is for callers to check for null on every API call. If this is an error state, could you use a `Preconditions.checkArgument` call instead? ########## File path: mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java ########## @@ -312,12 +313,18 @@ public void testLoadCatalogLocation() { @Test public void testLoadCatalogUnknown() { String catalogName = "barCatalog"; - conf.set(InputFormatConfig.catalogPropertyConfigKey(catalogName, CatalogUtil.ICEBERG_CATALOG_TYPE), "fooType"); + conf.set(InputFormatConfig.catalogPropertyConfigKey(catalogName, CatalogProperties.CATALOG_TYPE), "fooType"); AssertHelpers.assertThrows( "should complain about catalog not supported", UnsupportedOperationException.class, "Unknown catalog type:", () -> Catalogs.loadCatalog(conf, catalogName)); } + @Test + public void testCatalogTypeImpl() { + Assertions.assertThat( + CatalogType.getCatalogImpl(CatalogType.JDBC.getTypeName())).isEqualTo(JdbcCatalog.class.getName()); Review comment: Nit: This is over-indented. It should be 4 spaces from the start of `Assertions`. So ```java Assertions.assertThat( CatalogType.getCatalogImpl(.....))); ``` ########## File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java ########## @@ -208,23 +201,14 @@ public static Catalog loadCatalog( */ public static Catalog buildIcebergCatalog(String name, Map<String, String> options, Configuration conf) { String catalogImpl = options.get(CatalogProperties.CATALOG_IMPL); + String catalogType = options.get(CatalogProperties.CATALOG_TYPE); + Preconditions.checkArgument(catalogImpl == null || catalogType == null, + "Cannot create catalog %s, both type and catalog-impl are set: type=%s, catalog-impl=%s", Review comment: In Apache Flink, I believe that `type` is an existing catalog configuration requirement. I think people need to put `type = iceberg` on their Flink catalogs. Is this going to cause confusion at all? Open to discussion on it. ########## 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: I don't think you need to use `<br>` tags in the site docs. I believe that `mkdocs` will wrap `Description` and whatever is in it in a way that fits the screen (likely using a flexbox styling). What happens if you leave the `<br>` out? Keep in mind we also have a documentation refactor taking place, so keeping things somewhat consistent is likely better (unless there are other places that make use of `<br>`, but I doubt it and if there are we should definitely consider whether or not they're necessary). ########## File path: core/src/main/java/org/apache/iceberg/CatalogType.java ########## @@ -0,0 +1,61 @@ +/* + * 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); Review comment: Is there a specific reason you chose to use `Locale.ENGLISH`? I know that there are some usages of `Locale.ENGLISH` in the code base, but far more often we use `Locale.ROOT`. If there's no specific reason to not use `Locale.ROOT`, would you mind doing that for consistency's sake? I eventually plan on changing all of the instances of `Locale.ENGLISH` to `Locale.ROOT` unless there's a reason not to in the few places that do (maybe at most 10% of the time based on my past research). -- 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]
