Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 45 additions & 9 deletions core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.apache.iceberg;

import java.util.Collections;
import java.util.Map;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.TableIdentifier;
Expand All @@ -27,12 +28,19 @@
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;

public abstract class BaseMetastoreCatalog implements Catalog {
private static final Logger LOG = LoggerFactory.getLogger(BaseMetastoreCatalog.class);
private Map<String, String> catalogProps = Collections.emptyMap();

@Override
public void initialize(String name, Map<String, String> properties) {
catalogProps = properties;
}
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated

@Override
public Table loadTable(TableIdentifier identifier) {
Expand Down Expand Up @@ -106,7 +114,7 @@ public String toString() {
protected class BaseMetastoreCatalogTableBuilder implements TableBuilder {
private final TableIdentifier identifier;
private final Schema schema;
private final ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
private final Map<String, String> propertiesBuilder = Maps.newHashMap();
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated
private PartitionSpec spec = PartitionSpec.unpartitioned();
private SortOrder sortOrder = SortOrder.unsorted();
private String location = null;
Expand All @@ -116,6 +124,7 @@ public BaseMetastoreCatalogTableBuilder(TableIdentifier identifier, Schema schem

this.identifier = identifier;
this.schema = schema;
this.propertiesBuilder.putAll(tableDefaultProperties());
}

@Override
Expand All @@ -139,7 +148,7 @@ public TableBuilder withLocation(String newLocation) {
@Override
public TableBuilder withProperties(Map<String, String> properties) {
if (properties != null) {
propertiesBuilder.putAll(properties);
this.propertiesBuilder.putAll(properties);
}
return this;
}
Expand All @@ -158,8 +167,8 @@ public Table create() {
}

String baseLocation = location != null ? location : defaultWarehouseLocation(identifier);
Map<String, String> properties = 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);
Expand All @@ -178,8 +187,8 @@ public Transaction createTransaction() {
}

String baseLocation = location != null ? location : defaultWarehouseLocation(identifier);
Map<String, String> properties = propertiesBuilder.build();
TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, properties);
this.propertiesBuilder.putAll(tableOverrideProperties());
TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it now okay to pass Map instead of ImmutableMap while creating TableMetadata object?

return Transactions.createTableTransaction(identifier.toString(), ops, metadata);
}

Expand All @@ -200,12 +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);
metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder.build());
metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder);
}

if (orCreate) {
Expand All @@ -214,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<String, String> 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<String, String> tableOverrideProperties() {
if (catalogProps == null || catalogProps.isEmpty()) {
return Collections.emptyMap();
}
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated

return PropertyUtil.propertiesWithPrefix(catalogProps, CatalogProperties.TABLE_OVERRIDE_PREFIX);
}
Comment thread
SinghAsDev marked this conversation as resolved.
}

protected static String fullTableName(String catalogName, TableIdentifier identifier) {
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/org/apache/iceberg/CatalogProperties.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.";
Comment on lines +32 to +33

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit / open-question: How do people feel about these names? I'd kind of like them to have properties or something in them, but this might be overkill.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this.


/**
* Controls whether the catalog will cache table entries upon load.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ public HadoopCatalog() {

@Override
public void initialize(String name, Map<String, String> properties) {
super.initialize(name, properties);
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated

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)");
Expand Down
14 changes: 14 additions & 0 deletions core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}

public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
Comment thread
rdblue marked this conversation as resolved.
Outdated
String prefix) {
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated
if (properties == null || properties.isEmpty()) {
return Collections.emptyMap();
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated
}

return properties.entrySet().stream()
.filter(e -> e.getKey().startsWith(prefix))
.collect(Collectors.toMap(
e -> e.getKey().replace(prefix, ""), Map.Entry::getValue));
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated
}
Comment on lines +77 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We recently took this method in master to extract a map having s3 write tags prefix (#4259).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this can be removed. Thanks, @rajarshisarkar!

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -170,10 +172,15 @@ void rewriteMetadataAsGzipWithOldExtension() throws IOException {
}

protected HadoopCatalog hadoopCatalog() throws IOException {
return hadoopCatalog(Collections.emptyMap());
}

protected HadoopCatalog hadoopCatalog(Map<String, String> catalogProperties) throws IOException {
HadoopCatalog hadoopCatalog = new HadoopCatalog();
hadoopCatalog.setConf(new Configuration());
hadoopCatalog.initialize("hadoop",
ImmutableMap.of(CatalogProperties.WAREHOUSE_LOCATION, temp.newFolder().getAbsolutePath()));
ImmutableMap.<String, String>builder().putAll(catalogProperties).put(CatalogProperties.WAREHOUSE_LOCATION,
temp.newFolder().getAbsolutePath()).build());
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated
return hadoopCatalog;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -547,4 +548,60 @@ private static void addVersionsToTable(Table table) {
table.newAppend().appendFile(dataFile1).commit();
table.newAppend().appendFile(dataFile2).commit();
}

Comment thread
SinghAsDev marked this conversation as resolved.
@Test
public void testTablePropsDefaultsWithoutConflict() throws IOException {
TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
Map<String, String> 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<String, String> 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 testCatalogOverridePropsOverrideTableDefaults() throws IOException {
TableIdentifier tableIdent = TableIdentifier.of("db", "ns1", "ns2", "tbl");
Map<String, String> 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"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ public HiveCatalog() {

@Override
public void initialize(String inputName, Map<String, String> properties) {
super.initialize(name, properties);

this.name = inputName;
if (conf == null) {
LOG.warn("No Hadoop Configuration was set, using the default environment Configuration");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String, String> catalogProps = ImmutableMap.of("table-default.key3", "value3",
"table-override.key4", "value4");
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated
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<String, String> catalogProps = ImmutableMap.of("table-default.key3", "value3");
HiveCatalog hiveCatalog = (HiveCatalog) CatalogUtil.loadCatalog(HiveCatalog.class.getName(),
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE, catalogProps, hiveConf);
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated

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() {
Comment thread
SinghAsDev marked this conversation as resolved.
Outdated
Schema schema = new Schema(
required(1, "id", Types.IntegerType.get(), "unique ID")
);
TableIdentifier tableIdent = TableIdentifier.of(DB_NAME, "tbl");

ImmutableMap<String, String> 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);
}
}
}