From add90e0283515ca9850c9bdc8687f201e545ed4b Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Sun, 30 Jan 2022 16:48:09 -0800 Subject: [PATCH 1/9] Allow table defaults to be configured and/ or enforced at catalog level using catalog properties. --- .../apache/iceberg/BaseMetastoreCatalog.java | 86 ++++++++++++++++++- .../org/apache/iceberg/CatalogProperties.java | 2 + .../apache/iceberg/hadoop/HadoopCatalog.java | 2 + .../iceberg/hadoop/HadoopTableTestBase.java | 9 +- .../iceberg/hadoop/TestHadoopCatalog.java | 57 ++++++++++++ .../org/apache/iceberg/hive/HiveCatalog.java | 2 + .../apache/iceberg/hive/TestHiveCatalog.java | 81 +++++++++++++++++ 7 files changed, 235 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java index 9cea03d2f1ae..50692d445134 100644 --- a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java +++ b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java @@ -19,7 +19,11 @@ package org.apache.iceberg; +import java.util.Collections; +import java.util.Locale; import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.apache.iceberg.catalog.Catalog; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.AlreadyExistsException; @@ -33,6 +37,13 @@ public abstract class BaseMetastoreCatalog implements Catalog { private static final Logger LOG = LoggerFactory.getLogger(BaseMetastoreCatalog.class); + @SuppressWarnings("checkstyle:VisibilityModifier") + protected Map catalogProps = Collections.emptyMap(); + + @Override + public void initialize(String name, Map properties) { + catalogProps = properties; + } @Override public Table loadTable(TableIdentifier identifier) { @@ -158,7 +169,7 @@ public Table create() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - Map properties = propertiesBuilder.build(); + Map properties = overrideTableProperties(propertiesBuilder.build()); TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); try { @@ -178,7 +189,7 @@ public Transaction createTransaction() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - Map properties = propertiesBuilder.build(); + Map properties = overrideTableProperties(propertiesBuilder.build()); TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); return Transactions.createTableTransaction(identifier.toString(), ops, metadata); } @@ -205,7 +216,8 @@ private Transaction newReplaceTableTransaction(boolean orCreate) { metadata = ops.current().buildReplacement(schema, spec, sortOrder, baseLocation, propertiesBuilder.build()); } else { String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder.build()); + Map properties = overrideTableProperties(propertiesBuilder.build()); + metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); } if (orCreate) { @@ -238,4 +250,72 @@ protected static String fullTableName(String catalogName, TableIdentifier identi return sb.toString(); } + + @Override + public Table createTable( + TableIdentifier identifier, + Schema schema, + PartitionSpec spec, + String location, + Map properties) { + return buildTable(identifier, schema) + .withPartitionSpec(spec) + .withLocation(location) + .withProperties(overrideTableProperties(properties)) + .create(); + } + + /** + * Get catalog properties. + * + * @return catalog properties + */ + Map properties() { + return catalogProps; + } + + /** + * Get default table properties set at Catalog level through catalog properties. + * + * @return default table properties specified in catalog properties + */ + Map tableCreateDefaultProperties() { + Map props = properties() == null ? Collections.emptyMap() : properties(); + + return props.entrySet().stream() + .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(CatalogProperties.TABLE_DEFAULT_PREFIX)) + .collect(Collectors.toMap(e -> e.getKey().replace(CatalogProperties.TABLE_DEFAULT_PREFIX, ""), + Map.Entry::getValue)); + } + + /** + * Get table properties that are enforced at Catalog level through catalog properties. + * + * @return default table properties enforced through catalog properties + */ + Map tableCreateEnforcedProperties() { + Map props = properties() == null ? Collections.emptyMap() : properties(); + + return props.entrySet().stream() + .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(CatalogProperties.TABLE_OVERRIDE_PREFIX)) + .collect(Collectors.toMap(e -> e.getKey().replace(CatalogProperties.TABLE_OVERRIDE_PREFIX, ""), + Map.Entry::getValue)); + } + + /** + * Return updated table properties with table properties defaults and enforcements set at Catalog level through + * catalog properties. + * + * @return updated table properties with defaults and enforcements set at Catalog level + */ + Map overrideTableProperties(Map tableProperties) { + Map props = tableProperties == null ? Collections.emptyMap() : tableProperties; + + return Stream.concat( + Stream.concat( + tableCreateDefaultProperties().entrySet().stream(), + props.entrySet().stream()), + tableCreateEnforcedProperties().entrySet().stream()) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (e1, e2) -> e2)); + } } diff --git a/core/src/main/java/org/apache/iceberg/CatalogProperties.java b/core/src/main/java/org/apache/iceberg/CatalogProperties.java index d5daedee812b..447970cf643d 100644 --- a/core/src/main/java/org/apache/iceberg/CatalogProperties.java +++ b/core/src/main/java/org/apache/iceberg/CatalogProperties.java @@ -29,6 +29,8 @@ private CatalogProperties() { public static final String CATALOG_IMPL = "catalog-impl"; public static final String FILE_IO_IMPL = "io-impl"; public static final String WAREHOUSE_LOCATION = "warehouse"; + public static final String TABLE_DEFAULT_PREFIX = "table-default."; + public static final String TABLE_OVERRIDE_PREFIX = "table-override."; /** * Controls whether the catalog will cache table entries upon load. diff --git a/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java b/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java index 78224e27bb0a..6f2c76267df6 100644 --- a/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java +++ b/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java @@ -98,6 +98,8 @@ public HadoopCatalog() { @Override public void initialize(String name, Map properties) { + super.initialize(name, properties); + String inputWarehouseLocation = properties.get(CatalogProperties.WAREHOUSE_LOCATION); Preconditions.checkArgument(inputWarehouseLocation != null && !inputWarehouseLocation.equals(""), "Cannot instantiate hadoop catalog. No location provided for warehouse (Set warehouse config)"); diff --git a/core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java b/core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java index 0c2114808ae8..3fd5fa72c92e 100644 --- a/core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java +++ b/core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java @@ -24,7 +24,9 @@ import java.io.FileOutputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.zip.GZIPOutputStream; import org.apache.hadoop.conf.Configuration; import org.apache.iceberg.CatalogProperties; @@ -170,10 +172,15 @@ void rewriteMetadataAsGzipWithOldExtension() throws IOException { } protected HadoopCatalog hadoopCatalog() throws IOException { + return hadoopCatalog(Collections.emptyMap()); + } + + protected HadoopCatalog hadoopCatalog(Map catalogProperties) throws IOException { HadoopCatalog hadoopCatalog = new HadoopCatalog(); hadoopCatalog.setConf(new Configuration()); hadoopCatalog.initialize("hadoop", - ImmutableMap.of(CatalogProperties.WAREHOUSE_LOCATION, temp.newFolder().getAbsolutePath())); + ImmutableMap.builder().putAll(catalogProperties).put(CatalogProperties.WAREHOUSE_LOCATION, + temp.newFolder().getAbsolutePath()).build()); return hadoopCatalog; } } diff --git a/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java b/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java index 244f01aaa285..89e7794b22f0 100644 --- a/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java +++ b/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.List; +import java.util.Map; import java.util.Set; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; @@ -547,4 +548,60 @@ private static void addVersionsToTable(Table table) { table.newAppend().appendFile(dataFile1).commit(); table.newAppend().appendFile(dataFile2).commit(); } + + @Test + public void testTablePropsDefaultsWithoutConflict() throws IOException { + TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl"); + Map catalogProps = ImmutableMap.of("table-default.key3", "value3", + "table-override.key4", "value4"); + + Table table = hadoopCatalog(catalogProps).buildTable(tableIdent, SCHEMA) + .withPartitionSpec(SPEC) + .withProperties(null) + .withProperty("key1", "value1") + .withProperty("key2", "value2") + .create(); + + Assert.assertEquals("value1", table.properties().get("key1")); + Assert.assertEquals("value2", table.properties().get("key2")); + Assert.assertEquals("value3", table.properties().get("key3")); + Assert.assertEquals("value4", table.properties().get("key4")); + } + + + @Test + public void testTablePropsOverrideCatalogDefaultProps() throws IOException { + TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl"); + Map catalogProps = ImmutableMap.of("table-default.key3", "value3"); + + Table table = hadoopCatalog(catalogProps).buildTable(tableIdent, SCHEMA) + .withPartitionSpec(SPEC) + .withProperties(null) + .withProperty("key1", "value1") + .withProperty("key2", "value2") + .withProperty("key3", "value31") + .create(); + + Assert.assertEquals("value1", table.properties().get("key1")); + Assert.assertEquals("value2", table.properties().get("key2")); + Assert.assertEquals("value31", table.properties().get("key3")); + } + + @Test + public void testEnforcedCatalogPropsOverrideTableDefaults() throws IOException { + TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl"); + Map catalogProps = ImmutableMap.of("table-override.key3", "value3"); + + Table table = hadoopCatalog(catalogProps).buildTable(tableIdent, SCHEMA) + .withPartitionSpec(SPEC) + .withProperties(null) + .withProperty("key1", "value1") + .withProperty("key2", "value2") + .withProperty("key3", "value31") + .create(); + + Assert.assertEquals("value1", table.properties().get("key1")); + Assert.assertEquals("value2", table.properties().get("key2")); + Assert.assertEquals("value3", table.properties().get("key3")); + } } diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java index 0d1ba8e34bb1..078832dcc6bf 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java @@ -78,6 +78,8 @@ public HiveCatalog() { @Override public void initialize(String inputName, Map properties) { + super.initialize(name, properties); + this.name = inputName; if (conf == null) { LOG.warn("No Hadoop Configuration was set, using the default environment Configuration"); diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 74716b771cb0..2f0c7b505346 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -24,6 +24,7 @@ import org.apache.hadoop.hive.metastore.api.Database; import org.apache.iceberg.AssertHelpers; import org.apache.iceberg.CachingCatalog; +import org.apache.iceberg.CatalogUtil; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; import org.apache.iceberg.SortOrder; @@ -468,4 +469,84 @@ public void testUUIDinTableProperties() throws Exception { catalog.dropTable(tableIdentifier); } } + + @Test + public void testTablePropsDefaultsWithoutConflict() { + Schema schema = new Schema( + required(1, "id", Types.IntegerType.get(), "unique ID") + ); + TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl"); + + ImmutableMap catalogProps = ImmutableMap.of("table-default.key3", "value3", + "table-override.key4", "value4"); + HiveCatalog hiveCatalog = (HiveCatalog) CatalogUtil.loadCatalog(HiveCatalog.class.getName(), + CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE, catalogProps, hiveConf); + + try { + Table table = hiveCatalog.buildTable(tableIdent, schema) + .withProperty("key1", "value1") + .withProperty("key2", "value2") + .create(); + + Assert.assertEquals("value1", table.properties().get("key1")); + Assert.assertEquals("value2", table.properties().get("key2")); + Assert.assertEquals("value3", table.properties().get("key3")); + Assert.assertEquals("value4", table.properties().get("key4")); + } finally { + hiveCatalog.dropTable(tableIdent); + } + } + + @Test + public void testTablePropsOverrideCatalogDefaultProps() { + Schema schema = new Schema( + required(1, "id", Types.IntegerType.get(), "unique ID") + ); + TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl"); + + ImmutableMap catalogProps = ImmutableMap.of("table-default.key3", "value3"); + HiveCatalog hiveCatalog = (HiveCatalog) CatalogUtil.loadCatalog(HiveCatalog.class.getName(), + CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE, catalogProps, hiveConf); + + try { + Table table = hiveCatalog.buildTable(tableIdent, schema) + .withProperty("key1", "value1") + .withProperty("key2", "value2") + .withProperty("key3", "value31") + .create(); + + Assert.assertEquals("value1", table.properties().get("key1")); + Assert.assertEquals("value2", table.properties().get("key2")); + Assert.assertEquals("value31", table.properties().get("key3")); + } finally { + hiveCatalog.dropTable(tableIdent); + } + } + + @Test + public void testEnforcedCatalogPropsOverrideTableDefaults() { + Schema schema = new Schema( + required(1, "id", Types.IntegerType.get(), "unique ID") + ); + TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl"); + + ImmutableMap catalogProps = ImmutableMap.of( + "table-override.key3", "value3"); + HiveCatalog hiveCatalog = (HiveCatalog) CatalogUtil.loadCatalog(HiveCatalog.class.getName(), + CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE, catalogProps, hiveConf); + + try { + Table table = hiveCatalog.buildTable(tableIdent, schema) + .withProperty("key1", "value1") + .withProperty("key2", "value2") + .withProperty("key3", "value31") + .create(); + + Assert.assertEquals("value1", table.properties().get("key1")); + Assert.assertEquals("value2", table.properties().get("key2")); + Assert.assertEquals("value3", table.properties().get("key3")); + } finally { + hiveCatalog.dropTable(tableIdent); + } + } } From 4d26bb3e17206b687f3721cfe8d30eb343ff0583 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Sun, 30 Jan 2022 16:49:47 -0800 Subject: [PATCH 2/9] Make catalogProps field private --- .../apache/iceberg/BaseMetastoreCatalog.java | 34 +++++++------------ 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java index 50692d445134..1e697052dd1d 100644 --- a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java +++ b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java @@ -37,8 +37,7 @@ public abstract class BaseMetastoreCatalog implements Catalog { private static final Logger LOG = LoggerFactory.getLogger(BaseMetastoreCatalog.class); - @SuppressWarnings("checkstyle:VisibilityModifier") - protected Map catalogProps = Collections.emptyMap(); + private Map catalogProps = Collections.emptyMap(); @Override public void initialize(String name, Map properties) { @@ -169,7 +168,7 @@ public Table create() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - Map properties = overrideTableProperties(propertiesBuilder.build()); + Map properties = updateTableProperties(propertiesBuilder.build()); TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); try { @@ -189,7 +188,7 @@ public Transaction createTransaction() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - Map properties = overrideTableProperties(propertiesBuilder.build()); + Map properties = updateTableProperties(propertiesBuilder.build()); TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); return Transactions.createTableTransaction(identifier.toString(), ops, metadata); } @@ -216,7 +215,7 @@ private Transaction newReplaceTableTransaction(boolean orCreate) { metadata = ops.current().buildReplacement(schema, spec, sortOrder, baseLocation, propertiesBuilder.build()); } else { String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - Map properties = overrideTableProperties(propertiesBuilder.build()); + Map properties = updateTableProperties(propertiesBuilder.build()); metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); } @@ -261,26 +260,17 @@ public Table createTable( return buildTable(identifier, schema) .withPartitionSpec(spec) .withLocation(location) - .withProperties(overrideTableProperties(properties)) + .withProperties(updateTableProperties(properties)) .create(); } - /** - * Get catalog properties. - * - * @return catalog properties - */ - Map properties() { - return catalogProps; - } - /** * Get default table properties set at Catalog level through catalog properties. * * @return default table properties specified in catalog properties */ - Map tableCreateDefaultProperties() { - Map props = properties() == null ? Collections.emptyMap() : properties(); + private Map tableDefaultProperties() { + Map props = catalogProps == null ? Collections.emptyMap() : catalogProps; return props.entrySet().stream() .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(CatalogProperties.TABLE_DEFAULT_PREFIX)) @@ -293,8 +283,8 @@ Map tableCreateDefaultProperties() { * * @return default table properties enforced through catalog properties */ - Map tableCreateEnforcedProperties() { - Map props = properties() == null ? Collections.emptyMap() : properties(); + private Map tableOverrideProperties() { + Map props = catalogProps == null ? Collections.emptyMap() : catalogProps; return props.entrySet().stream() .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(CatalogProperties.TABLE_OVERRIDE_PREFIX)) @@ -308,14 +298,14 @@ Map tableCreateEnforcedProperties() { * * @return updated table properties with defaults and enforcements set at Catalog level */ - Map overrideTableProperties(Map tableProperties) { + private Map updateTableProperties(Map tableProperties) { Map props = tableProperties == null ? Collections.emptyMap() : tableProperties; return Stream.concat( Stream.concat( - tableCreateDefaultProperties().entrySet().stream(), + tableDefaultProperties().entrySet().stream(), props.entrySet().stream()), - tableCreateEnforcedProperties().entrySet().stream()) + tableOverrideProperties().entrySet().stream()) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (e1, e2) -> e2)); } } From 7eb19c5902f6b3acb4d72bc42ee1376f6903ee53 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Wed, 9 Feb 2022 11:08:57 -0800 Subject: [PATCH 3/9] Updates --- .../apache/iceberg/BaseMetastoreCatalog.java | 110 ++++++------------ .../org/apache/iceberg/util/PropertyUtil.java | 14 +++ .../iceberg/hadoop/TestHadoopCatalog.java | 2 +- .../apache/iceberg/hive/TestHiveCatalog.java | 2 +- 4 files changed, 54 insertions(+), 74 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java index 1e697052dd1d..fed68c1165ea 100644 --- a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java +++ b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java @@ -20,10 +20,7 @@ package org.apache.iceberg; import java.util.Collections; -import java.util.Locale; import java.util.Map; -import java.util.stream.Collectors; -import java.util.stream.Stream; import org.apache.iceberg.catalog.Catalog; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.AlreadyExistsException; @@ -31,7 +28,8 @@ import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; -import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.util.PropertyUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -116,7 +114,7 @@ public String toString() { protected class BaseMetastoreCatalogTableBuilder implements TableBuilder { private final TableIdentifier identifier; private final Schema schema; - private final ImmutableMap.Builder propertiesBuilder = ImmutableMap.builder(); + private final Map propertiesBuilder = Maps.newHashMap(); private PartitionSpec spec = PartitionSpec.unpartitioned(); private SortOrder sortOrder = SortOrder.unsorted(); private String location = null; @@ -126,6 +124,7 @@ public BaseMetastoreCatalogTableBuilder(TableIdentifier identifier, Schema schem this.identifier = identifier; this.schema = schema; + this.propertiesBuilder.putAll(tableDefaultProperties()); } @Override @@ -149,7 +148,7 @@ public TableBuilder withLocation(String newLocation) { @Override public TableBuilder withProperties(Map properties) { if (properties != null) { - propertiesBuilder.putAll(properties); + this.propertiesBuilder.putAll(properties); } return this; } @@ -168,8 +167,8 @@ public Table create() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - Map properties = updateTableProperties(propertiesBuilder.build()); - TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); + this.propertiesBuilder.putAll(tableOverrideProperties()); + TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder); try { ops.commit(null, metadata); @@ -188,8 +187,8 @@ public Transaction createTransaction() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - Map properties = updateTableProperties(propertiesBuilder.build()); - TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); + this.propertiesBuilder.putAll(tableOverrideProperties()); + TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder); return Transactions.createTableTransaction(identifier.toString(), ops, metadata); } @@ -210,13 +209,13 @@ private Transaction newReplaceTableTransaction(boolean orCreate) { } TableMetadata metadata; + this.propertiesBuilder.putAll(tableOverrideProperties()); if (ops.current() != null) { String baseLocation = location != null ? location : ops.current().location(); - metadata = ops.current().buildReplacement(schema, spec, sortOrder, baseLocation, propertiesBuilder.build()); + metadata = ops.current().buildReplacement(schema, spec, sortOrder, baseLocation, propertiesBuilder); } else { String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - Map properties = updateTableProperties(propertiesBuilder.build()); - metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties); + metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder); } if (orCreate) { @@ -225,6 +224,32 @@ private Transaction newReplaceTableTransaction(boolean orCreate) { return Transactions.replaceTableTransaction(identifier.toString(), ops, metadata); } } + + /** + * Get default table properties set at Catalog level through catalog properties. + * + * @return default table properties specified in catalog properties + */ + private Map tableDefaultProperties() { + if (catalogProps == null || catalogProps.isEmpty()) { + return Collections.emptyMap(); + } + + return PropertyUtil.propertiesWithPrefix(catalogProps, CatalogProperties.TABLE_DEFAULT_PREFIX); + } + + /** + * Get table properties that are enforced at Catalog level through catalog properties. + * + * @return default table properties enforced through catalog properties + */ + private Map tableOverrideProperties() { + if (catalogProps == null || catalogProps.isEmpty()) { + return Collections.emptyMap(); + } + + return PropertyUtil.propertiesWithPrefix(catalogProps, CatalogProperties.TABLE_OVERRIDE_PREFIX); + } } protected static String fullTableName(String catalogName, TableIdentifier identifier) { @@ -249,63 +274,4 @@ protected static String fullTableName(String catalogName, TableIdentifier identi return sb.toString(); } - - @Override - public Table createTable( - TableIdentifier identifier, - Schema schema, - PartitionSpec spec, - String location, - Map properties) { - return buildTable(identifier, schema) - .withPartitionSpec(spec) - .withLocation(location) - .withProperties(updateTableProperties(properties)) - .create(); - } - - /** - * Get default table properties set at Catalog level through catalog properties. - * - * @return default table properties specified in catalog properties - */ - private Map tableDefaultProperties() { - Map props = catalogProps == null ? Collections.emptyMap() : catalogProps; - - return props.entrySet().stream() - .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(CatalogProperties.TABLE_DEFAULT_PREFIX)) - .collect(Collectors.toMap(e -> e.getKey().replace(CatalogProperties.TABLE_DEFAULT_PREFIX, ""), - Map.Entry::getValue)); - } - - /** - * Get table properties that are enforced at Catalog level through catalog properties. - * - * @return default table properties enforced through catalog properties - */ - private Map tableOverrideProperties() { - Map props = catalogProps == null ? Collections.emptyMap() : catalogProps; - - return props.entrySet().stream() - .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(CatalogProperties.TABLE_OVERRIDE_PREFIX)) - .collect(Collectors.toMap(e -> e.getKey().replace(CatalogProperties.TABLE_OVERRIDE_PREFIX, ""), - Map.Entry::getValue)); - } - - /** - * Return updated table properties with table properties defaults and enforcements set at Catalog level through - * catalog properties. - * - * @return updated table properties with defaults and enforcements set at Catalog level - */ - private Map updateTableProperties(Map tableProperties) { - Map props = tableProperties == null ? Collections.emptyMap() : tableProperties; - - return Stream.concat( - Stream.concat( - tableDefaultProperties().entrySet().stream(), - props.entrySet().stream()), - tableOverrideProperties().entrySet().stream()) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (e1, e2) -> e2)); - } } diff --git a/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java b/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java index ae66b67592ec..a578bde8ddf7 100644 --- a/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java @@ -19,7 +19,9 @@ package org.apache.iceberg.util; +import java.util.Collections; import java.util.Map; +import java.util.stream.Collectors; public class PropertyUtil { @@ -70,4 +72,16 @@ public static String propertyAsString(Map properties, } return defaultValue; } + + public static Map propertiesWithPrefix(Map properties, + String prefix) { + if (properties == null || properties.isEmpty()) { + return Collections.emptyMap(); + } + + return properties.entrySet().stream() + .filter(e -> e.getKey().startsWith(prefix)) + .collect(Collectors.toMap( + e -> e.getKey().replace(prefix, ""), Map.Entry::getValue)); + } } diff --git a/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java b/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java index 89e7794b22f0..6a57da46c828 100644 --- a/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java +++ b/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java @@ -588,7 +588,7 @@ public void testTablePropsOverrideCatalogDefaultProps() throws IOException { } @Test - public void testEnforcedCatalogPropsOverrideTableDefaults() throws IOException { + public void testCatalogOverridePropsOverrideTableDefaults() throws IOException { TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl"); Map catalogProps = ImmutableMap.of("table-override.key3", "value3"); diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 2f0c7b505346..207669fc30bc 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -524,7 +524,7 @@ public void testTablePropsOverrideCatalogDefaultProps() { } @Test - public void testEnforcedCatalogPropsOverrideTableDefaults() { + public void testCatalogOverridePropsOverrideTableDefaults() { Schema schema = new Schema( required(1, "id", Types.IntegerType.get(), "unique ID") ); From 46711e074c80e8e4c7c31e5a869f4393135def88 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Sun, 13 Feb 2022 10:28:18 -0800 Subject: [PATCH 4/9] Minor cleanup --- .../java/org/apache/iceberg/BaseMetastoreCatalog.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java index fed68c1165ea..4dbde1ad8bc5 100644 --- a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java +++ b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java @@ -148,7 +148,7 @@ public TableBuilder withLocation(String newLocation) { @Override public TableBuilder withProperties(Map properties) { if (properties != null) { - this.propertiesBuilder.putAll(properties); + propertiesBuilder.putAll(properties); } return this; } @@ -167,7 +167,7 @@ public Table create() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - this.propertiesBuilder.putAll(tableOverrideProperties()); + propertiesBuilder.putAll(tableOverrideProperties()); TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder); try { @@ -187,7 +187,7 @@ public Transaction createTransaction() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - this.propertiesBuilder.putAll(tableOverrideProperties()); + propertiesBuilder.putAll(tableOverrideProperties()); TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder); return Transactions.createTableTransaction(identifier.toString(), ops, metadata); } @@ -209,7 +209,7 @@ private Transaction newReplaceTableTransaction(boolean orCreate) { } TableMetadata metadata; - this.propertiesBuilder.putAll(tableOverrideProperties()); + propertiesBuilder.putAll(tableOverrideProperties()); if (ops.current() != null) { String baseLocation = location != null ? location : ops.current().location(); metadata = ops.current().buildReplacement(schema, spec, sortOrder, baseLocation, propertiesBuilder); From 5c70c4d86df72106a223a269fdb0d23d1eba647e Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Mon, 28 Feb 2022 12:23:47 -0800 Subject: [PATCH 5/9] Update --- .../apache/iceberg/aws/glue/GlueCatalog.java | 8 ++ .../apache/iceberg/BaseMetastoreCatalog.java | 44 ++++----- .../apache/iceberg/hadoop/HadoopCatalog.java | 9 +- .../org/apache/iceberg/util/PropertyUtil.java | 17 +++- .../iceberg/hadoop/HadoopTableTestBase.java | 9 +- .../iceberg/hadoop/TestHadoopCatalog.java | 79 +++++++--------- .../org/apache/iceberg/hive/HiveCatalog.java | 9 +- .../apache/iceberg/hive/TestHiveCatalog.java | 94 +++++++------------ 8 files changed, 125 insertions(+), 144 deletions(-) diff --git a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java index f878ecf2412b..81b6018fad11 100644 --- a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java +++ b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java @@ -21,6 +21,7 @@ import java.io.Closeable; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -86,6 +87,7 @@ public class GlueCatalog extends BaseMetastoreCatalog private FileIO fileIO; private LockManager lockManager; private CloseableGroup closeableGroup; + private Map catalogProps = Collections.emptyMap(); /** * No-arg constructor to load the catalog dynamically. @@ -97,6 +99,7 @@ public GlueCatalog() { @Override public void initialize(String name, Map properties) { + this.catalogProps = properties; initialize( name, properties.get(CatalogProperties.WAREHOUSE_LOCATION), @@ -445,4 +448,9 @@ public void close() throws IOException { public void setConf(Configuration conf) { this.hadoopConf = conf; } + + @Override + protected Map properties() { + return this.catalogProps; + } } diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java index 4dbde1ad8bc5..765be7fd1217 100644 --- a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java +++ b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java @@ -35,12 +35,6 @@ public abstract class BaseMetastoreCatalog implements Catalog { private static final Logger LOG = LoggerFactory.getLogger(BaseMetastoreCatalog.class); - private Map catalogProps = Collections.emptyMap(); - - @Override - public void initialize(String name, Map properties) { - catalogProps = properties; - } @Override public Table loadTable(TableIdentifier identifier) { @@ -114,7 +108,7 @@ public String toString() { protected class BaseMetastoreCatalogTableBuilder implements TableBuilder { private final TableIdentifier identifier; private final Schema schema; - private final Map propertiesBuilder = Maps.newHashMap(); + private final Map tableProperties = Maps.newHashMap(); private PartitionSpec spec = PartitionSpec.unpartitioned(); private SortOrder sortOrder = SortOrder.unsorted(); private String location = null; @@ -124,7 +118,7 @@ public BaseMetastoreCatalogTableBuilder(TableIdentifier identifier, Schema schem this.identifier = identifier; this.schema = schema; - this.propertiesBuilder.putAll(tableDefaultProperties()); + this.tableProperties.putAll(tableDefaultProperties()); } @Override @@ -148,14 +142,14 @@ public TableBuilder withLocation(String newLocation) { @Override public TableBuilder withProperties(Map properties) { if (properties != null) { - propertiesBuilder.putAll(properties); + tableProperties.putAll(properties); } return this; } @Override public TableBuilder withProperty(String key, String value) { - propertiesBuilder.put(key, value); + tableProperties.put(key, value); return this; } @@ -167,8 +161,8 @@ public Table create() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - propertiesBuilder.putAll(tableOverrideProperties()); - TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder); + tableProperties.putAll(tableOverrideProperties()); + TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, tableProperties); try { ops.commit(null, metadata); @@ -187,8 +181,8 @@ public Transaction createTransaction() { } String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - propertiesBuilder.putAll(tableOverrideProperties()); - TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder); + tableProperties.putAll(tableOverrideProperties()); + TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, tableProperties); return Transactions.createTableTransaction(identifier.toString(), ops, metadata); } @@ -209,13 +203,13 @@ private Transaction newReplaceTableTransaction(boolean orCreate) { } TableMetadata metadata; - propertiesBuilder.putAll(tableOverrideProperties()); + tableProperties.putAll(tableOverrideProperties()); if (ops.current() != null) { String baseLocation = location != null ? location : ops.current().location(); - metadata = ops.current().buildReplacement(schema, spec, sortOrder, baseLocation, propertiesBuilder); + metadata = ops.current().buildReplacement(schema, spec, sortOrder, baseLocation, tableProperties); } else { String baseLocation = location != null ? location : defaultWarehouseLocation(identifier); - metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder); + metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, tableProperties); } if (orCreate) { @@ -231,11 +225,7 @@ private Transaction newReplaceTableTransaction(boolean orCreate) { * @return default table properties specified in catalog properties */ private Map tableDefaultProperties() { - if (catalogProps == null || catalogProps.isEmpty()) { - return Collections.emptyMap(); - } - - return PropertyUtil.propertiesWithPrefix(catalogProps, CatalogProperties.TABLE_DEFAULT_PREFIX); + return PropertyUtil.propertiesWithPrefix(properties(), CatalogProperties.TABLE_DEFAULT_PREFIX); } /** @@ -244,11 +234,7 @@ private Map tableDefaultProperties() { * @return default table properties enforced through catalog properties */ private Map tableOverrideProperties() { - if (catalogProps == null || catalogProps.isEmpty()) { - return Collections.emptyMap(); - } - - return PropertyUtil.propertiesWithPrefix(catalogProps, CatalogProperties.TABLE_OVERRIDE_PREFIX); + return PropertyUtil.propertiesWithPrefix(properties(), CatalogProperties.TABLE_OVERRIDE_PREFIX); } } @@ -274,4 +260,8 @@ protected static String fullTableName(String catalogName, TableIdentifier identi return sb.toString(); } + + protected Map properties() { + return Collections.emptyMap(); + } } diff --git a/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java b/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java index 6f2c76267df6..db8efa71d7b8 100644 --- a/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java +++ b/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java @@ -25,6 +25,7 @@ import java.io.UncheckedIOException; import java.nio.file.AccessDeniedException; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -92,13 +93,14 @@ public class HadoopCatalog extends BaseMetastoreCatalog implements Closeable, Su private FileIO fileIO; private LockManager lockManager; private boolean suppressPermissionError = false; + private Map catalogProps = Collections.emptyMap(); public HadoopCatalog() { } @Override public void initialize(String name, Map properties) { - super.initialize(name, properties); + this.catalogProps = properties; String inputWarehouseLocation = properties.get(CatalogProperties.WAREHOUSE_LOCATION); Preconditions.checkArgument(inputWarehouseLocation != null && !inputWarehouseLocation.equals(""), @@ -393,6 +395,11 @@ public Configuration getConf() { return conf; } + @Override + protected Map properties() { + return this.catalogProps; + } + private class HadoopCatalogTableBuilder extends BaseMetastoreCatalogTableBuilder { private final String defaultLocation; diff --git a/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java b/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java index a578bde8ddf7..7cb8dd2b15ae 100644 --- a/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java @@ -19,9 +19,10 @@ package org.apache.iceberg.util; -import java.util.Collections; import java.util.Map; import java.util.stream.Collectors; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; public class PropertyUtil { @@ -73,15 +74,25 @@ public static String propertyAsString(Map properties, return defaultValue; } + /** + * Returns subset of provided map with keys matching the provided prefix. Matching is case-sensitive and the matching + * prefix is removed from the keys in returned map. + * + * @param properties input map + * @param prefix prefix to choose keys from input map + * @return subset of input map with keys starting with provided prefix and prefix trimmed out + */ public static Map propertiesWithPrefix(Map properties, String prefix) { if (properties == null || properties.isEmpty()) { - return Collections.emptyMap(); + return ImmutableMap.of(); } + Preconditions.checkState(prefix != null, "prefix can't be null."); + return properties.entrySet().stream() .filter(e -> e.getKey().startsWith(prefix)) .collect(Collectors.toMap( - e -> e.getKey().replace(prefix, ""), Map.Entry::getValue)); + e -> e.getKey().replaceFirst(prefix, ""), Map.Entry::getValue)); } } diff --git a/core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java b/core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java index 3fd5fa72c92e..012aa82bb447 100644 --- a/core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java +++ b/core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java @@ -178,9 +178,12 @@ protected HadoopCatalog hadoopCatalog() throws IOException { protected HadoopCatalog hadoopCatalog(Map catalogProperties) throws IOException { HadoopCatalog hadoopCatalog = new HadoopCatalog(); hadoopCatalog.setConf(new Configuration()); - hadoopCatalog.initialize("hadoop", - ImmutableMap.builder().putAll(catalogProperties).put(CatalogProperties.WAREHOUSE_LOCATION, - temp.newFolder().getAbsolutePath()).build()); + hadoopCatalog.initialize( + "hadoop", + ImmutableMap.builder() + .putAll(catalogProperties) + .put(CatalogProperties.WAREHOUSE_LOCATION, temp.newFolder().getAbsolutePath()) + .build()); return hadoopCatalog; } } diff --git a/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java b/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java index 6a57da46c828..889cf2b2b449 100644 --- a/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java +++ b/core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java @@ -22,7 +22,6 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.List; -import java.util.Map; import java.util.Set; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; @@ -550,58 +549,44 @@ private static void addVersionsToTable(Table table) { } @Test - public void testTablePropsDefaultsWithoutConflict() throws IOException { + public void testTablePropsDefinedAtCatalogLevel() throws IOException { TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl"); - Map catalogProps = ImmutableMap.of("table-default.key3", "value3", - "table-override.key4", "value4"); + ImmutableMap catalogProps = ImmutableMap.of( + "table-default.key1", "catalog-default-key1", + "table-default.key2", "catalog-default-key2", + "table-default.key3", "catalog-default-key3", + "table-override.key3", "catalog-override-key3", + "table-override.key4", "catalog-override-key4"); Table table = hadoopCatalog(catalogProps).buildTable(tableIdent, SCHEMA) .withPartitionSpec(SPEC) .withProperties(null) - .withProperty("key1", "value1") - .withProperty("key2", "value2") - .create(); - - Assert.assertEquals("value1", table.properties().get("key1")); - Assert.assertEquals("value2", table.properties().get("key2")); - Assert.assertEquals("value3", table.properties().get("key3")); - Assert.assertEquals("value4", table.properties().get("key4")); - } - - - @Test - public void testTablePropsOverrideCatalogDefaultProps() throws IOException { - TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl"); - Map catalogProps = ImmutableMap.of("table-default.key3", "value3"); - - Table table = hadoopCatalog(catalogProps).buildTable(tableIdent, SCHEMA) - .withPartitionSpec(SPEC) - .withProperties(null) - .withProperty("key1", "value1") - .withProperty("key2", "value2") - .withProperty("key3", "value31") + .withProperty("key2", "table-key2") + .withProperty("key3", "table-key3") + .withProperty("key5", "table-key5") .create(); - Assert.assertEquals("value1", table.properties().get("key1")); - Assert.assertEquals("value2", table.properties().get("key2")); - Assert.assertEquals("value31", table.properties().get("key3")); - } - - @Test - public void testCatalogOverridePropsOverrideTableDefaults() throws IOException { - TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl"); - Map catalogProps = ImmutableMap.of("table-override.key3", "value3"); - - Table table = hadoopCatalog(catalogProps).buildTable(tableIdent, SCHEMA) - .withPartitionSpec(SPEC) - .withProperties(null) - .withProperty("key1", "value1") - .withProperty("key2", "value2") - .withProperty("key3", "value31") - .create(); - - Assert.assertEquals("value1", table.properties().get("key1")); - Assert.assertEquals("value2", table.properties().get("key2")); - Assert.assertEquals("value3", table.properties().get("key3")); + Assert.assertEquals( + "Table defaults set for the catalog must be added to the table properties.", + "catalog-default-key1", + table.properties().get("key1")); + Assert.assertEquals( + "Table property must override table default properties set at catalog level.", + "table-key2", + table.properties().get("key2")); + Assert.assertEquals( + "Table property override set at catalog level must override table default" + + " properties set at catalog level and table property specified.", + "catalog-override-key3", + table.properties().get("key3")); + Assert.assertEquals( + "Table override not in table props or defaults should be added to table properties", + "catalog-override-key4", + table.properties().get("key4")); + Assert.assertEquals( + "Table properties without any catalog level default or override should be added to table" + + " properties.", + "table-key5", + table.properties().get("key5")); } } diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java index 078832dcc6bf..84ba3640266b 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java @@ -19,6 +19,7 @@ package org.apache.iceberg.hive; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -72,13 +73,14 @@ public class HiveCatalog extends BaseMetastoreCatalog implements SupportsNamespa private FileIO fileIO; private ClientPool clients; private boolean listAllTables = false; + private Map catalogProps = Collections.emptyMap(); public HiveCatalog() { } @Override public void initialize(String inputName, Map properties) { - super.initialize(name, properties); + this.catalogProps = properties; this.name = inputName; if (conf == null) { @@ -539,6 +541,11 @@ public Configuration getConf() { return conf; } + @Override + protected Map properties() { + return this.catalogProps; + } + @VisibleForTesting void setListAllTables(boolean listAllTables) { this.listAllTables = listAllTables; diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 207669fc30bc..31989be36f58 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -471,80 +471,50 @@ public void testUUIDinTableProperties() throws Exception { } @Test - public void testTablePropsDefaultsWithoutConflict() { - Schema schema = new Schema( - required(1, "id", Types.IntegerType.get(), "unique ID") - ); - TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl"); - - ImmutableMap catalogProps = ImmutableMap.of("table-default.key3", "value3", - "table-override.key4", "value4"); - HiveCatalog hiveCatalog = (HiveCatalog) CatalogUtil.loadCatalog(HiveCatalog.class.getName(), - CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE, catalogProps, hiveConf); - - try { - Table table = hiveCatalog.buildTable(tableIdent, schema) - .withProperty("key1", "value1") - .withProperty("key2", "value2") - .create(); - - Assert.assertEquals("value1", table.properties().get("key1")); - Assert.assertEquals("value2", table.properties().get("key2")); - Assert.assertEquals("value3", table.properties().get("key3")); - Assert.assertEquals("value4", table.properties().get("key4")); - } finally { - hiveCatalog.dropTable(tableIdent); - } - } - - @Test - public void testTablePropsOverrideCatalogDefaultProps() { - Schema schema = new Schema( - required(1, "id", Types.IntegerType.get(), "unique ID") - ); - TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl"); - - ImmutableMap catalogProps = ImmutableMap.of("table-default.key3", "value3"); - HiveCatalog hiveCatalog = (HiveCatalog) CatalogUtil.loadCatalog(HiveCatalog.class.getName(), - CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE, catalogProps, hiveConf); - - try { - Table table = hiveCatalog.buildTable(tableIdent, schema) - .withProperty("key1", "value1") - .withProperty("key2", "value2") - .withProperty("key3", "value31") - .create(); - - Assert.assertEquals("value1", table.properties().get("key1")); - Assert.assertEquals("value2", table.properties().get("key2")); - Assert.assertEquals("value31", table.properties().get("key3")); - } finally { - hiveCatalog.dropTable(tableIdent); - } - } - - @Test - public void testCatalogOverridePropsOverrideTableDefaults() { + public void testTablePropsDefinedAtCatalogLevel() { Schema schema = new Schema( required(1, "id", Types.IntegerType.get(), "unique ID") ); TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl"); ImmutableMap catalogProps = ImmutableMap.of( - "table-override.key3", "value3"); - HiveCatalog hiveCatalog = (HiveCatalog) CatalogUtil.loadCatalog(HiveCatalog.class.getName(), + "table-default.key1", "catalog-default-key1", + "table-default.key2", "catalog-default-key2", + "table-default.key3", "catalog-default-key3", + "table-override.key3", "catalog-override-key3", + "table-override.key4", "catalog-override-key4"); + Catalog hiveCatalog = CatalogUtil.loadCatalog(HiveCatalog.class.getName(), CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE, catalogProps, hiveConf); try { Table table = hiveCatalog.buildTable(tableIdent, schema) - .withProperty("key1", "value1") - .withProperty("key2", "value2") - .withProperty("key3", "value31") + .withProperty("key2", "table-key2") + .withProperty("key3", "table-key3") + .withProperty("key5", "table-key5") .create(); - Assert.assertEquals("value1", table.properties().get("key1")); - Assert.assertEquals("value2", table.properties().get("key2")); - Assert.assertEquals("value3", table.properties().get("key3")); + Assert.assertEquals( + "Table defaults set for the catalog must be added to the table properties.", + "catalog-default-key1", + table.properties().get("key1")); + Assert.assertEquals( + "Table property must override table default properties set at catalog level.", + "table-key2", + table.properties().get("key2")); + Assert.assertEquals( + "Table property override set at catalog level must override table default" + + " properties set at catalog level and table property specified.", + "catalog-override-key3", + table.properties().get("key3")); + Assert.assertEquals( + "Table override not in table props or defaults should be added to table properties", + "catalog-override-key4", + table.properties().get("key4")); + Assert.assertEquals( + "Table properties without any catalog level default or override should be added to table" + + " properties.", + "table-key5", + table.properties().get("key5")); } finally { hiveCatalog.dropTable(tableIdent); } From ed1a3a662ea882326f7760a5adfee5494833af5f Mon Sep 17 00:00:00 2001 From: Rajarshi Sarkar Date: Wed, 11 May 2022 15:09:20 +0530 Subject: [PATCH 6/9] Fix build --- .../main/java/org/apache/iceberg/aws/glue/GlueCatalog.java | 2 ++ .../test/java/org/apache/iceberg/hive/TestHiveCatalog.java | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java index 1071a570593f..1b1cc8d32148 100644 --- a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java +++ b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java @@ -94,6 +94,7 @@ public class GlueCatalog extends BaseMetastoreCatalog private LockManager lockManager; private CloseableGroup closeableGroup; private Map catalogProperties; + private Map catalogProps = Collections.emptyMap(); // Attempt to set versionId if available on the path private static final DynMethods.UnboundMethod SET_VERSION_ID = DynMethods.builder("versionId") @@ -111,6 +112,7 @@ public GlueCatalog() { @Override public void initialize(String name, Map properties) { + this.catalogProps = properties; AwsClientFactory awsClientFactory; FileIO catalogFileIO; if (PropertyUtil.propertyAsBoolean( diff --git a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java index 22a26266c368..46ccb571a553 100644 --- a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java +++ b/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java @@ -27,7 +27,11 @@ import org.apache.hadoop.hive.metastore.api.Database; import org.apache.iceberg.AssertHelpers; import org.apache.iceberg.CachingCatalog; +import org.apache.iceberg.CatalogProperties; import org.apache.iceberg.CatalogUtil; +import org.apache.iceberg.DataFile; +import org.apache.iceberg.DataFiles; +import org.apache.iceberg.FileFormat; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.PartitionSpecParser; import org.apache.iceberg.Schema; @@ -656,7 +660,7 @@ public void testConstructorWarehousePathWithEndSlash() { Assert.assertEquals("Should have trailing slash stripped", wareHousePath, catalogWithSlash.getConf().get( HiveConf.ConfVars.METASTOREWAREHOUSE.varname)); } - + @Test public void testTablePropsDefinedAtCatalogLevel() { Schema schema = new Schema( From 6af5b510b6de3c37a8760259e5ead5842133ffaf Mon Sep 17 00:00:00 2001 From: Rajarshi Sarkar Date: Wed, 11 May 2022 17:08:41 +0530 Subject: [PATCH 7/9] Resolve review comments --- .../aws/glue/TestGlueCatalogTable.java | 51 +++++++++++++++++++ .../apache/iceberg/aws/glue/GlueCatalog.java | 7 ++- .../apache/iceberg/BaseMetastoreCatalog.java | 8 +-- .../apache/iceberg/hadoop/HadoopCatalog.java | 7 ++- .../org/apache/iceberg/hive/HiveCatalog.java | 7 ++- 5 files changed, 64 insertions(+), 16 deletions(-) diff --git a/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java b/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java index 8429e0f6953a..f493ccd73d77 100644 --- a/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java +++ b/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java @@ -41,6 +41,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.types.Types; +import org.apache.iceberg.types.Types.NestedField; import org.junit.Assert; import org.junit.Test; import software.amazon.awssdk.services.glue.model.Column; @@ -368,4 +369,54 @@ public void testColumnCommentsAndParameters() { ); Assert.assertEquals("Columns do not match", expectedColumns, actualColumns); } + + @Test + public void testTablePropsDefinedAtCatalogLevel() { + String namespace = createNamespace(); + String tableName = getRandomName(); + TableIdentifier tableIdent = TableIdentifier.of(namespace, tableName); + ImmutableMap catalogProps = ImmutableMap.of( + "table-default.key1", "catalog-default-key1", + "table-default.key2", "catalog-default-key2", + "table-default.key3", "catalog-default-key3", + "table-override.key3", "catalog-override-key3", + "table-override.key4", "catalog-override-key4", + "warehouse", "s3://" + testBucketName + "/" + testPathPrefix); + + glueCatalog.initialize("glue", catalogProps); + + Schema schema = new Schema( + NestedField.required(3, "id", Types.IntegerType.get(), "unique ID"), + NestedField.required(4, "data", Types.StringType.get()) + ); + + org.apache.iceberg.Table table = glueCatalog.buildTable(tableIdent, schema) + .withProperty("key2", "table-key2") + .withProperty("key3", "table-key3") + .withProperty("key5", "table-key5") + .create(); + + Assert.assertEquals( + "Table defaults set for the catalog must be added to the table properties.", + "catalog-default-key1", + table.properties().get("key1")); + Assert.assertEquals( + "Table property must override table default properties set at catalog level.", + "table-key2", + table.properties().get("key2")); + Assert.assertEquals( + "Table property override set at catalog level must override table default" + + " properties set at catalog level and table property specified.", + "catalog-override-key3", + table.properties().get("key3")); + Assert.assertEquals( + "Table override not in table props or defaults should be added to table properties", + "catalog-override-key4", + table.properties().get("key4")); + Assert.assertEquals( + "Table properties without any catalog level default or override should be added to table" + + " properties.", + "table-key5", + table.properties().get("key5")); + } } diff --git a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java index 1b1cc8d32148..6b5e52911a5b 100644 --- a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java +++ b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java @@ -93,8 +93,7 @@ public class GlueCatalog extends BaseMetastoreCatalog private FileIO fileIO; private LockManager lockManager; private CloseableGroup closeableGroup; - private Map catalogProperties; - private Map catalogProps = Collections.emptyMap(); + private Map catalogProperties = Collections.emptyMap(); // Attempt to set versionId if available on the path private static final DynMethods.UnboundMethod SET_VERSION_ID = DynMethods.builder("versionId") @@ -112,7 +111,7 @@ public GlueCatalog() { @Override public void initialize(String name, Map properties) { - this.catalogProps = properties; + this.catalogProperties = properties; AwsClientFactory awsClientFactory; FileIO catalogFileIO; if (PropertyUtil.propertyAsBoolean( @@ -501,6 +500,6 @@ public void setConf(Configuration conf) { @Override protected Map properties() { - return this.catalogProps; + return catalogProperties; } } diff --git a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java index 8c71df383681..e4ad6a7a2941 100644 --- a/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java +++ b/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java @@ -96,6 +96,10 @@ protected boolean isValidIdentifier(TableIdentifier tableIdentifier) { return true; } + protected Map properties() { + return Collections.emptyMap(); + } + @Override public String toString() { return MoreObjects.toStringHelper(this).toString(); @@ -260,8 +264,4 @@ protected static String fullTableName(String catalogName, TableIdentifier identi return sb.toString(); } - - protected Map properties() { - return Collections.emptyMap(); - } } diff --git a/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java b/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java index 88a8b46fd926..b574b5c78055 100644 --- a/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java +++ b/core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java @@ -94,15 +94,14 @@ public class HadoopCatalog extends BaseMetastoreCatalog implements Closeable, Su private FileIO fileIO; private LockManager lockManager; private boolean suppressPermissionError = false; - private Map catalogProps = Collections.emptyMap(); + private Map catalogProperties = Collections.emptyMap(); public HadoopCatalog() { } @Override public void initialize(String name, Map properties) { - this.catalogProps = properties; - + this.catalogProperties = properties; String inputWarehouseLocation = properties.get(CatalogProperties.WAREHOUSE_LOCATION); Preconditions.checkArgument(inputWarehouseLocation != null && inputWarehouseLocation.length() > 0, "Cannot initialize HadoopCatalog because warehousePath must not be null or empty"); @@ -399,7 +398,7 @@ public Configuration getConf() { @Override protected Map properties() { - return this.catalogProps; + return catalogProperties; } private class HadoopCatalogTableBuilder extends BaseMetastoreCatalogTableBuilder { diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java index 975e73ddf152..7d1f84d91027 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java @@ -74,15 +74,14 @@ public class HiveCatalog extends BaseMetastoreCatalog implements SupportsNamespa private FileIO fileIO; private ClientPool clients; private boolean listAllTables = false; - private Map catalogProps = Collections.emptyMap(); + private Map catalogProperties = Collections.emptyMap(); public HiveCatalog() { } @Override public void initialize(String inputName, Map properties) { - this.catalogProps = properties; - + this.catalogProperties = properties; this.name = inputName; if (conf == null) { LOG.warn("No Hadoop Configuration was set, using the default environment Configuration"); @@ -545,7 +544,7 @@ public Configuration getConf() { @Override protected Map properties() { - return this.catalogProps; + return catalogProperties; } @VisibleForTesting From a2fbf5920035daf0868c27d25945025220ab712a Mon Sep 17 00:00:00 2001 From: Rajarshi Sarkar Date: Wed, 11 May 2022 17:14:57 +0530 Subject: [PATCH 8/9] Resolve review comments --- core/src/main/java/org/apache/iceberg/util/PropertyUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java b/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java index 601f88fc0745..1028bc5e05c1 100644 --- a/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java @@ -88,7 +88,7 @@ public static Map propertiesWithPrefix( return ImmutableMap.of(); } - Preconditions.checkArgument(prefix != null, "prefix can't be null."); + Preconditions.checkArgument(prefix != null, "Invalid prefix: null"); return properties.entrySet().stream() .filter(e -> e.getKey().startsWith(prefix)) From 9fe9215368f576cf4431c01c81ab8efb096f5ab6 Mon Sep 17 00:00:00 2001 From: Rajarshi Sarkar Date: Tue, 17 May 2022 14:40:42 +0530 Subject: [PATCH 9/9] Resolve review comments --- .../apache/iceberg/aws/glue/GlueCatalog.java | 7 ++++++ .../iceberg/aws/glue/TestGlueCatalog.java | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java index 6b5e52911a5b..3c04a1bfbff3 100644 --- a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java +++ b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java @@ -164,6 +164,13 @@ private FileIO initializeFileIO(Map properties) { } } + @VisibleForTesting + void initialize(String name, String path, AwsProperties properties, GlueClient client, + LockManager lock, FileIO io, Map catalogProps) { + this.catalogProperties = catalogProps; + initialize(name, path, properties, client, lock, io); + } + @VisibleForTesting void initialize(String name, String path, AwsProperties properties, GlueClient client, LockManager lock, FileIO io) { Preconditions.checkArgument(path != null && path.length() > 0, diff --git a/aws/src/test/java/org/apache/iceberg/aws/glue/TestGlueCatalog.java b/aws/src/test/java/org/apache/iceberg/aws/glue/TestGlueCatalog.java index b5db606622af..b81d1cf0cb65 100644 --- a/aws/src/test/java/org/apache/iceberg/aws/glue/TestGlueCatalog.java +++ b/aws/src/test/java/org/apache/iceberg/aws/glue/TestGlueCatalog.java @@ -455,4 +455,28 @@ public void testRemoveProperties() { .when(glue).updateDatabase(Mockito.any(UpdateDatabaseRequest.class)); glueCatalog.removeProperties(Namespace.of("db1"), Sets.newHashSet("key")); } + + @Test + public void testTablePropsDefinedAtCatalogLevel() { + ImmutableMap catalogProps = ImmutableMap.of( + "table-default.key1", "catalog-default-key1", + "table-default.key2", "catalog-default-key2", + "table-default.key3", "catalog-default-key3", + "table-override.key3", "catalog-override-key3", + "table-override.key4", "catalog-override-key4"); + glueCatalog.initialize(CATALOG_NAME, WAREHOUSE_PATH, new AwsProperties(), glue, + LockManagers.defaultLockManager(), null, catalogProps); + Map properties = glueCatalog.properties(); + Assert.assertFalse(properties.isEmpty()); + Assert.assertTrue(properties.containsKey("table-default.key1")); + Assert.assertEquals("catalog-default-key1", properties.get("table-default.key1")); + Assert.assertTrue(properties.containsKey("table-default.key2")); + Assert.assertEquals("catalog-default-key2", properties.get("table-default.key2")); + Assert.assertTrue(properties.containsKey("table-default.key3")); + Assert.assertEquals("catalog-default-key3", properties.get("table-default.key3")); + Assert.assertTrue(properties.containsKey("table-override.key3")); + Assert.assertEquals("catalog-override-key3", properties.get("table-override.key3")); + Assert.assertTrue(properties.containsKey("table-override.key4")); + Assert.assertEquals("catalog-override-key4", properties.get("table-override.key4")); + } }