jackye1995 commented on a change in pull request #2565:
URL: https://github.com/apache/iceberg/pull/2565#discussion_r633144895
##########
File path: mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java
##########
@@ -191,91 +191,103 @@ public void testCreateDropTableToCatalog() throws
IOException {
}
@Test
- public void testLoadCatalog() throws IOException {
- conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
- Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ public void testLoadCatalog_legacy_default() {
+ Optional<Catalog> defaultCatalog = Catalogs.loadCatalog(conf, null);
+ Assert.assertTrue(defaultCatalog.isPresent());
+ Assert.assertTrue(defaultCatalog.get() instanceof HiveCatalog);
+ }
- String nonExistentCatalogType = "fooType";
+ @Test
+ public void testLoadCatalog_legacy_hive() {
+ conf.set(InputFormatConfig.CATALOG, CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
+ Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, null);
+ Assert.assertTrue(hiveCatalog.isPresent());
+ Assert.assertTrue(hiveCatalog.get() instanceof HiveCatalog);
+ }
- conf.set(InputFormatConfig.CATALOG, nonExistentCatalogType);
- AssertHelpers.assertThrows(
- "should complain about catalog not supported",
UnsupportedOperationException.class,
- "Unknown catalog type", () -> Catalogs.loadCatalog(conf, null));
+ @Test
+ public void testLoadCatalog_legacy_location() {
+ conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
+ Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ }
+ @Test
+ public void testLoadCatalog_legacy_hadoop() {
conf.set(InputFormatConfig.CATALOG,
CatalogUtil.ICEBERG_CATALOG_TYPE_HADOOP);
conf.set(InputFormatConfig.HADOOP_CATALOG_WAREHOUSE_LOCATION,
"/tmp/mylocation");
Optional<Catalog> hadoopCatalog = Catalogs.loadCatalog(conf, null);
-
Assert.assertTrue(hadoopCatalog.isPresent());
Assert.assertTrue(hadoopCatalog.get() instanceof HadoopCatalog);
+ }
- conf.set(InputFormatConfig.CATALOG, CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
- Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, null);
-
- Assert.assertTrue(hiveCatalog.isPresent());
- Assert.assertTrue(hiveCatalog.get() instanceof HiveCatalog);
-
- conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
- Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ @Test
+ public void testLoadCatalog_legacy_unknown() {
+ conf.set(InputFormatConfig.CATALOG, "fooType");
+ AssertHelpers.assertThrows(
+ "should complain about catalog not supported",
UnsupportedOperationException.class,
+ "Unknown catalog type", () -> Catalogs.loadCatalog(conf, null));
+ }
- // arbitrary catalog name with non existent catalog type
+ @Test
+ public void testLoadCatalog_unknown() {
String catalogName = "barCatalog";
- conf.unset(InputFormatConfig.CATALOG);
- conf.set(String.format(InputFormatConfig.CATALOG_TYPE_TEMPLATE,
catalogName), nonExistentCatalogType);
+ conf.set(InputFormatConfig.catalogTypeConfigKey(catalogName), "fooType");
AssertHelpers.assertThrows(
"should complain about catalog not supported",
UnsupportedOperationException.class,
"Unknown catalog type:", () -> Catalogs.loadCatalog(conf,
catalogName));
+ }
- // arbitrary catalog name with hadoop catalog type and default warehouse
location
- conf.set(String.format(InputFormatConfig.CATALOG_TYPE_TEMPLATE,
catalogName),
- CatalogUtil.ICEBERG_CATALOG_TYPE_HADOOP);
- hadoopCatalog = Catalogs.loadCatalog(conf, catalogName);
-
+ @Test
+ public void testLoadCatalog_hadoop_warehouseLocation() {
+ String catalogName = "barCatalog";
+ conf.set(InputFormatConfig.catalogTypeConfigKey(catalogName),
CatalogUtil.ICEBERG_CATALOG_TYPE_HADOOP);
+ conf.set(InputFormatConfig.catalogWarehouseConfigKey(catalogName),
"/tmp/mylocation");
+ Optional<Catalog> hadoopCatalog = Catalogs.loadCatalog(conf, catalogName);
Assert.assertTrue(hadoopCatalog.isPresent());
Assert.assertTrue(hadoopCatalog.get() instanceof HadoopCatalog);
+ Assert.assertEquals("HadoopCatalog{name=barCatalog,
location=/tmp/mylocation}", hadoopCatalog.get().toString());
Review comment:
This was in the original test, so sorry I did not look too much into it.
I agree that it's better to test with those methods, but unfortunately
`HadoopCatalog.warehouseLocation()` does not exist, so we cannot really use
that, so I suppose that is why this test is written in this way to get the
warehouse location.
##########
File path: mr/src/main/java/org/apache/iceberg/mr/Catalogs.java
##########
@@ -267,8 +265,8 @@ public static boolean hiveCatalog(Configuration conf,
Properties props) {
*/
private static String getCatalogType(Configuration conf, String catalogName)
{
if (catalogName != null) {
- String catalogType =
conf.get(String.format(InputFormatConfig.CATALOG_TYPE_TEMPLATE, catalogName));
- if (catalogName.equals(ICEBERG_HADOOP_TABLE_NAME) || catalogType ==
null) {
+ String catalogType =
conf.get(InputFormatConfig.catalogTypeConfigKey(catalogName));
+ if (catalogName.equals(ICEBERG_HADOOP_TABLE_NAME)) {
Review comment:
updated
##########
File path: mr/src/main/java/org/apache/iceberg/mr/InputFormatConfig.java
##########
@@ -203,6 +202,44 @@ public static Schema readSchema(Configuration conf) {
return readColumns != null && readColumns.length > 0 ? readColumns : null;
}
+ /**
+ * Get Hadoop config key of catalog type based on catalog name and {@link
#CATALOG_TYPE_TEMPLATE}
+ * @param catalogName catalog name
+ * @return Hadoop config key of catalog type for the catalog name
+ */
+ public static String catalogTypeConfigKey(String catalogName) {
+ return String.format(InputFormatConfig.CATALOG_TYPE_TEMPLATE, catalogName);
Review comment:
So while Ryan and Anton have not replied, I will keep it like this, we
can always add `catalog-type` as a new catalog properties in a new PR.
##########
File path: mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java
##########
@@ -191,91 +191,103 @@ public void testCreateDropTableToCatalog() throws
IOException {
}
@Test
- public void testLoadCatalog() throws IOException {
- conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
- Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ public void testLoadCatalog_legacy_default() {
+ Optional<Catalog> defaultCatalog = Catalogs.loadCatalog(conf, null);
+ Assert.assertTrue(defaultCatalog.isPresent());
+ Assert.assertTrue(defaultCatalog.get() instanceof HiveCatalog);
+ }
- String nonExistentCatalogType = "fooType";
+ @Test
+ public void testLoadCatalog_legacy_hive() {
+ conf.set(InputFormatConfig.CATALOG, CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
+ Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, null);
+ Assert.assertTrue(hiveCatalog.isPresent());
+ Assert.assertTrue(hiveCatalog.get() instanceof HiveCatalog);
+ }
- conf.set(InputFormatConfig.CATALOG, nonExistentCatalogType);
- AssertHelpers.assertThrows(
- "should complain about catalog not supported",
UnsupportedOperationException.class,
- "Unknown catalog type", () -> Catalogs.loadCatalog(conf, null));
+ @Test
+ public void testLoadCatalog_legacy_location() {
+ conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
+ Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ }
+ @Test
+ public void testLoadCatalog_legacy_hadoop() {
conf.set(InputFormatConfig.CATALOG,
CatalogUtil.ICEBERG_CATALOG_TYPE_HADOOP);
conf.set(InputFormatConfig.HADOOP_CATALOG_WAREHOUSE_LOCATION,
"/tmp/mylocation");
Optional<Catalog> hadoopCatalog = Catalogs.loadCatalog(conf, null);
-
Assert.assertTrue(hadoopCatalog.isPresent());
Assert.assertTrue(hadoopCatalog.get() instanceof HadoopCatalog);
+ }
- conf.set(InputFormatConfig.CATALOG, CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
- Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, null);
-
- Assert.assertTrue(hiveCatalog.isPresent());
- Assert.assertTrue(hiveCatalog.get() instanceof HiveCatalog);
-
- conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
- Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ @Test
+ public void testLoadCatalog_legacy_unknown() {
+ conf.set(InputFormatConfig.CATALOG, "fooType");
+ AssertHelpers.assertThrows(
+ "should complain about catalog not supported",
UnsupportedOperationException.class,
+ "Unknown catalog type", () -> Catalogs.loadCatalog(conf, null));
+ }
Review comment:
Actually, because the `CATALOG_LOADER_CLASS`
(`iceberg.mr.catalog.loader.class`) is no longer used, we cannot load a custom
catalog in the legacy mode, so there is no test for it.
Looking at
https://github.com/apache/iceberg/pull/2129/files#diff-c183ea3aa154c2a5012f87d7a06dba3cff3f27975384e9fb4040fe6850a98bd6L192-L193,
this seems like a backwards incompatible change. @lcspinter is this
intentional, or should we add that part of the logic back?
##########
File path: mr/src/main/java/org/apache/iceberg/mr/InputFormatConfig.java
##########
@@ -203,6 +202,44 @@ public static Schema readSchema(Configuration conf) {
return readColumns != null && readColumns.length > 0 ? readColumns : null;
}
+ /**
+ * Get Hadoop config key of catalog type based on catalog name and {@link
#CATALOG_TYPE_TEMPLATE}
+ * @param catalogName catalog name
+ * @return Hadoop config key of catalog type for the catalog name
+ */
+ public static String catalogTypeConfigKey(String catalogName) {
+ return String.format(InputFormatConfig.CATALOG_TYPE_TEMPLATE, catalogName);
+ }
+
+ /**
+ * Get Hadoop config key of catalog warehouse location based on catalog name
+ * @param catalogName catalog name
+ * @return Hadoop config key of catalog warehouse location for the catalog
name
+ */
+ public static String catalogWarehouseConfigKey(String catalogName) {
Review comment:
@pvary it is used in all catalogs except `HiveCatalog`, because Hive
uses `HiveConf.ConfVars.METASTOREWAREHOUSE` instead.
##########
File path: mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java
##########
@@ -191,91 +191,103 @@ public void testCreateDropTableToCatalog() throws
IOException {
}
@Test
- public void testLoadCatalog() throws IOException {
- conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
- Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ public void testLoadCatalog_legacy_default() {
+ Optional<Catalog> defaultCatalog = Catalogs.loadCatalog(conf, null);
+ Assert.assertTrue(defaultCatalog.isPresent());
+ Assert.assertTrue(defaultCatalog.get() instanceof HiveCatalog);
+ }
- String nonExistentCatalogType = "fooType";
+ @Test
+ public void testLoadCatalog_legacy_hive() {
+ conf.set(InputFormatConfig.CATALOG, CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
+ Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, null);
+ Assert.assertTrue(hiveCatalog.isPresent());
+ Assert.assertTrue(hiveCatalog.get() instanceof HiveCatalog);
+ }
- conf.set(InputFormatConfig.CATALOG, nonExistentCatalogType);
- AssertHelpers.assertThrows(
- "should complain about catalog not supported",
UnsupportedOperationException.class,
- "Unknown catalog type", () -> Catalogs.loadCatalog(conf, null));
+ @Test
+ public void testLoadCatalog_legacy_location() {
+ conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
+ Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ }
+ @Test
+ public void testLoadCatalog_legacy_hadoop() {
conf.set(InputFormatConfig.CATALOG,
CatalogUtil.ICEBERG_CATALOG_TYPE_HADOOP);
conf.set(InputFormatConfig.HADOOP_CATALOG_WAREHOUSE_LOCATION,
"/tmp/mylocation");
Optional<Catalog> hadoopCatalog = Catalogs.loadCatalog(conf, null);
-
Assert.assertTrue(hadoopCatalog.isPresent());
Assert.assertTrue(hadoopCatalog.get() instanceof HadoopCatalog);
+ }
- conf.set(InputFormatConfig.CATALOG, CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
- Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, null);
-
- Assert.assertTrue(hiveCatalog.isPresent());
- Assert.assertTrue(hiveCatalog.get() instanceof HiveCatalog);
-
- conf.set(InputFormatConfig.CATALOG, Catalogs.LOCATION);
- Assert.assertFalse(Catalogs.loadCatalog(conf, null).isPresent());
+ @Test
+ public void testLoadCatalog_legacy_unknown() {
+ conf.set(InputFormatConfig.CATALOG, "fooType");
+ AssertHelpers.assertThrows(
+ "should complain about catalog not supported",
UnsupportedOperationException.class,
+ "Unknown catalog type", () -> Catalogs.loadCatalog(conf, null));
+ }
Review comment:
for now, I have updated the code to allow this legacy behavior, and
added the missing test.
##########
File path: mr/src/main/java/org/apache/iceberg/mr/InputFormatConfig.java
##########
@@ -203,6 +202,44 @@ public static Schema readSchema(Configuration conf) {
return readColumns != null && readColumns.length > 0 ? readColumns : null;
}
+ /**
+ * Get Hadoop config key of catalog type based on catalog name and {@link
#CATALOG_TYPE_TEMPLATE}
+ * @param catalogName catalog name
+ * @return Hadoop config key of catalog type for the catalog name
+ */
+ public static String catalogTypeConfigKey(String catalogName) {
+ return String.format(InputFormatConfig.CATALOG_TYPE_TEMPLATE, catalogName);
Review comment:
In addition, I am treating `CatalogUtil.ICEBERG_CATALOG_TYPE` as the
catalog property to use, and removed reference to
`InputFormatConfig.CATALOG_TYPE_TEMPLATE`. I also added documentation inside
`CatalogUtil` to make things clear.
I don't personally like the fact that `CatalogUtil.ICEBERG_CATALOG_TYPE` is
in `CatalogUtil` instead of `CatalogProperties`, but we already released it in
0.11 as public variables so I decide to just keep it this way.
##########
File path: mr/src/main/java/org/apache/iceberg/mr/Catalogs.java
##########
@@ -267,8 +265,8 @@ public static boolean hiveCatalog(Configuration conf,
Properties props) {
*/
private static String getCatalogType(Configuration conf, String catalogName)
{
if (catalogName != null) {
- String catalogType =
conf.get(String.format(InputFormatConfig.CATALOG_TYPE_TEMPLATE, catalogName));
- if (catalogName.equals(ICEBERG_HADOOP_TABLE_NAME) || catalogType ==
null) {
+ String catalogType =
conf.get(InputFormatConfig.catalogTypeConfigKey(catalogName));
+ if (catalogName.equals(ICEBERG_HADOOP_TABLE_NAME)) {
Review comment:
I moved the doc of this method to `Catalogs` class level, because
private method docs are not generated in html.
##########
File path: mr/src/main/java/org/apache/iceberg/mr/InputFormatConfig.java
##########
@@ -203,6 +202,44 @@ public static Schema readSchema(Configuration conf) {
return readColumns != null && readColumns.length > 0 ? readColumns : null;
}
+ /**
+ * Get Hadoop config key of catalog type based on catalog name and {@link
#CATALOG_TYPE_TEMPLATE}
+ * @param catalogName catalog name
+ * @return Hadoop config key of catalog type for the catalog name
+ */
+ public static String catalogTypeConfigKey(String catalogName) {
+ return String.format(InputFormatConfig.CATALOG_TYPE_TEMPLATE, catalogName);
+ }
+
+ /**
+ * Get Hadoop config key of catalog warehouse location based on catalog name
+ * @param catalogName catalog name
+ * @return Hadoop config key of catalog warehouse location for the catalog
name
+ */
+ public static String catalogWarehouseConfigKey(String catalogName) {
Review comment:
Sure, I can remove that method
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
