You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/01/30 20:30:26 UTC
[GitHub] [iceberg] SinghAsDev opened a new pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
SinghAsDev opened a new pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011
Allow table defaults to be configured and/ or enforced at catalog level using catalog properties. This resolves https://github.com/apache/iceberg/issues/3994
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#issuecomment-1038324853
@rdblue this should be ready for another pass when you get a chance. thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r800238442
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> 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));
Review comment:
Rather than copying this, can you add a util method in `PropertyUtil`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#issuecomment-1038324853
@rdblue this should be ready for another pass when you get a chance. thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540323
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
Review comment:
I agree, but I cant think of a method name that is much better. I think we should just cover this in Javadoc.
@SinghAsDev can you add Javadoc that notes this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540056
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
Review comment:
I agree. It's worth the rename to `properties`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r835953132
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -445,4 +448,9 @@ public void close() throws IOException {
public void setConf(Configuration conf) {
this.hadoopConf = conf;
}
+
+ @Override
+ protected Map<String, String> properties() {
+ return this.catalogProps;
Review comment:
Minor: `this.` is only used when assigning to a field.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540716
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -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");
Review comment:
For ImmutableMap.of, how about putting each key on a new line?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r807620069
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
+ String prefix) {
Review comment:
[question] since this is a public should we check `prefix != null` as replace requires the target CharSequence to be NonNull ?
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
+ }
+
+ return PropertyUtil.propertiesWithPrefix(catalogProps, CatalogProperties.TABLE_OVERRIDE_PREFIX);
+ }
Review comment:
[minor] how about making a single function and taking Prefix as argument, as the code skeleton is almost same
some thing like :
```
private Map<String, String> extendedTableProperties(String prefix) {
if (catalogProps == null || catalogProps.isEmpty()) {
return Collections.emptyMap();
}
return PropertyUtil.propertiesWithPrefix(catalogProps, prefix);
}
```
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java
##########
@@ -98,6 +98,8 @@ public HadoopCatalog() {
@Override
public void initialize(String name, Map<String, String> properties) {
+ super.initialize(name, properties);
Review comment:
[question] Any reason we didn't do it for glue catalog
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java
##########
@@ -547,4 +548,60 @@ private static void addVersionsToTable(Table table) {
table.newAppend().appendFile(dataFile1).commit();
table.newAppend().appendFile(dataFile2).commit();
}
+
Review comment:
how about adding a test case when override / default are on the same key
same key is also passed with property
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r807712217
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
Review comment:
How about renaming it as `properties`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r795267568
##########
File path: core/src/main/java/org/apache/iceberg/CachingCatalog.java
##########
@@ -65,6 +66,8 @@ public static Catalog wrap(Catalog catalog, boolean caseSensitive, long expirati
protected final long expirationIntervalMillis;
@SuppressWarnings("checkstyle:VisibilityModifier")
protected final Cache<TableIdentifier, Table> tableCache;
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ protected Map<String, String> catalogProps = Collections.emptyMap();
Review comment:
I don't think that CachingCatalog should need to change at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r795286751
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -388,6 +392,59 @@ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
default void initialize(String name, Map<String, String> properties) {
}
+ /**
+ * Get catalog properties.
+ *
+ * @return catalog properties
+ */
+ Map<String, String> properties();
+
+ /**
+ * Get default table properties set at Catalog level through catalog properties.
+ *
+ * @return default table properties specified in catalog properties
+ */
+ default Map<String, String> tableCreateDefaultProperties() {
+ String tableCreateDefaultPrefix = "table.create.default.";
+ Map<String, String> props = properties() == null ? Collections.emptyMap() : properties();
+
+ return props.entrySet().stream()
+ .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(tableCreateDefaultPrefix))
+ .collect(Collectors.toMap(e -> e.getKey().replace(tableCreateDefaultPrefix, ""),
+ Map.Entry::getValue));
+ }
+
+ /**
+ * Get table properties that are enforced at Catalog level through catalog properties.
+ *
+ * @return default table properties enforced through catalog properties
+ */
+ default Map<String, String> tableCreateEnforcedProperties() {
+ String tableCreateEnforcedPrefix = "table.create.enforced.";
+ Map<String, String> props = properties() == null ? Collections.emptyMap() : properties();
+
+ return props.entrySet().stream()
+ .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(tableCreateEnforcedPrefix))
+ .collect(Collectors.toMap(e -> e.getKey().replace(tableCreateEnforcedPrefix, ""),
+ 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
+ */
+ default Map<String, String> overrideTableProperties(Map<String, String> tableProperties) {
+ Map<String, String> props = tableProperties == null ? Collections.emptyMap() : tableProperties;
+
+ return Stream.concat(Stream.concat(
+ tableCreateDefaultProperties().entrySet().stream(),
+ props.entrySet().stream()),
Review comment:
Thanks for the pointer, updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540646
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java
##########
@@ -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());
Review comment:
Style: In Iceberg, try to wrap lines at the highest semantic level possible, rather than trying to use the whole line. In this case, it is awkward that `temp.newFolder()...` is on its own line because it's inside a method call.
I'd change it to this:
```java
hadoopCatalog.initialize(
"hadoop",
ImmutableMap.<String, String>builder()
.putAll(catalogProperties)
.put(CatalogProperties.WAREHOUSE_LOCATION, temp.newFolder().getAbsolutePath())
.build());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r800238719
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> 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, ""),
Review comment:
I think it would be more clear if both key and value handlers were wrapped at the same level. That is, move `e -> ...` to a newline next to `Map.Entry::getValue`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540777
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -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");
+ 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);
Review comment:
Why is casting needed here? This test doesn't need to rely on this being a HiveCatalog, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r820394678
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -445,4 +448,9 @@ public void close() throws IOException {
public void setConf(Configuration conf) {
this.hadoopConf = conf;
}
+
+ @Override
+ protected Map<String, String> properties() {
Review comment:
Agreed, we should have test cases for Glue catalog.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -445,4 +448,9 @@ public void close() throws IOException {
public void setConf(Configuration conf) {
this.hadoopConf = conf;
}
+
+ @Override
+ protected Map<String, String> properties() {
Review comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r795267343
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -388,6 +392,59 @@ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
default void initialize(String name, Map<String, String> properties) {
}
+ /**
+ * Get catalog properties.
+ *
+ * @return catalog properties
+ */
+ Map<String, String> properties();
+
+ /**
+ * Get default table properties set at Catalog level through catalog properties.
+ *
+ * @return default table properties specified in catalog properties
+ */
+ default Map<String, String> tableCreateDefaultProperties() {
+ String tableCreateDefaultPrefix = "table.create.default.";
+ Map<String, String> props = properties() == null ? Collections.emptyMap() : properties();
+
+ return props.entrySet().stream()
+ .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(tableCreateDefaultPrefix))
+ .collect(Collectors.toMap(e -> e.getKey().replace(tableCreateDefaultPrefix, ""),
+ Map.Entry::getValue));
+ }
+
+ /**
+ * Get table properties that are enforced at Catalog level through catalog properties.
+ *
+ * @return default table properties enforced through catalog properties
+ */
+ default Map<String, String> tableCreateEnforcedProperties() {
+ String tableCreateEnforcedPrefix = "table.create.enforced.";
+ Map<String, String> props = properties() == null ? Collections.emptyMap() : properties();
+
+ return props.entrySet().stream()
+ .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(tableCreateEnforcedPrefix))
+ .collect(Collectors.toMap(e -> e.getKey().replace(tableCreateEnforcedPrefix, ""),
+ 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
+ */
+ default Map<String, String> overrideTableProperties(Map<String, String> tableProperties) {
+ Map<String, String> props = tableProperties == null ? Collections.emptyMap() : tableProperties;
+
+ return Stream.concat(Stream.concat(
+ tableCreateDefaultProperties().entrySet().stream(),
+ props.entrySet().stream()),
Review comment:
This mixes indentation for semantic levels. See https://github.com/apache/iceberg/blob/master/CONTRIBUTING.md#line-breaks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r835956856
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +260,8 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ protected Map<String, String> properties() {
Review comment:
This should be located with the other instance methods, not at the end after the builder and static methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#issuecomment-1030075657
Hi @rdblue just wanted to bump this and see if you would have cycles to review this. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r803006106
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
Review comment:
The variable name here is misleading now, but leaving it as is to avoid conflicts. Let me know if we should change this instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r807720897
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
Review comment:
The method name doesn't suggest that we replace the keys which starts with the prefix.
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
Review comment:
How about `properties`?
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> 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));
Review comment:
We can use `replaceFirst` if we are interested only in replacing the prefix.
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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);
+ propertiesBuilder.putAll(tableOverrideProperties());
+ TableMetadata metadata = TableMetadata.newTableMetadata(schema, spec, sortOrder, baseLocation, propertiesBuilder);
Review comment:
Is it now okay to pass `Map` instead of `ImmutableMap` while creating `TableMetadata` object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r799872564
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> 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<String, String> tableOverrideProperties() {
+ Map<String, String> 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<String, String> updateTableProperties(Map<String, String> tableProperties) {
Review comment:
Nit: Would it be possible to name this something that makes it a bit more obvious what this function does instead of the verb `update`?
Maybe `combineCatalogDerivedTablePropertiesWith`? Kind of long, but the current usages like the following are now that clear to me when read.
```java
return buildTable(identifier, schema)
.withPartitionSpec(spec)
.withLocation(location)
.withProperties(updateTableProperties(properties))
.create();
```
If it were the following, the long name might be justified to as it is more readable (to me at least).
```
return buildTable(identifier, schema)
.withPartitionSpec(spec)
.withLocation(location)
.withProperties(combinCatalogTablePropertiesWith(properties))
.create();
```
I'm open to other names and would love to hear other people's opinion. This is by no means a blocker (especially as the method is `private` anyway).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#issuecomment-1025260143
I took a quick look. I think the idea is reasonable, but the implementation needs to be changed:
* I think `table-default` and `table-override` are better property prefixes
* This should not modify the `Catalog` interface or `CachingCatalog`
* The best place to add this is in `BaseMetastoreCatalog` or other catalogs you need to update
* Subclasses should pass properties using `super(...)` in the constructor, not set parent fields
* Please reduce the footprint of this change by not modifying classes unnecessarily; add defaults to avoid touching so many files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#issuecomment-1025961297
Thanks for the quick turn-around @SinghAsDev! I'll take another look, hopefully today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r835956050
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +73,26 @@ public static String propertyAsString(Map<String, String> 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<String, String> propertiesWithPrefix(Map<String, String> properties,
+ String prefix) {
+ if (properties == null || properties.isEmpty()) {
+ return ImmutableMap.of();
+ }
+
+ Preconditions.checkState(prefix != null, "prefix can't be null.");
Review comment:
Error messages should begin with capital letters. They also do not end with punctuation because it isn't helpful and takes unnecessary space.. Last, we don't use contractions and we try to stick to a common structure. This should be `Invalid prefix: null`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r816263640
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
+ }
Review comment:
Probably forgot after moving this check down, removed. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r835956091
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
Review comment:
I think `propertiesFromPrefix` is a better name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r795267497
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -33,6 +34,8 @@
public abstract class BaseMetastoreCatalog implements Catalog {
private static final Logger LOG = LoggerFactory.getLogger(BaseMetastoreCatalog.class);
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ protected Map<String, String> catalogProps = Collections.emptyMap();
Review comment:
Why is this protected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540226
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> 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));
Review comment:
Yes, this seems like a good suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810539835
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
+ String prefix) {
+ if (properties == null || properties.isEmpty()) {
+ return Collections.emptyMap();
Review comment:
We generally prefer `ImmutableMap.of()`, but this is minor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r826607645
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +73,26 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ /**
+ * Returns subset of provided map with keys matching the provided prefix. Matching is case-sensitive and the matching
Review comment:
We recently took this method in `master` to extract a map having s3 write tags prefix (https://github.com/apache/iceberg/pull/4259).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r802958326
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> 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));
Review comment:
Good point
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r800238123
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> props = catalogProps == null ? Collections.emptyMap() : catalogProps;
Review comment:
If catalog props are null or empty, this should just return `ImmutableMap.empty`. Same with the override case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r814339181
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java
##########
@@ -98,6 +98,8 @@ public HadoopCatalog() {
@Override
public void initialize(String name, Map<String, String> properties) {
+ super.initialize(name, properties);
Review comment:
Added
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
+ String prefix) {
Review comment:
Thanks, that's a good point.
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java
##########
@@ -547,4 +548,60 @@ private static void addVersionsToTable(Table table) {
table.newAppend().appendFile(dataFile1).commit();
table.newAppend().appendFile(dataFile2).commit();
}
+
Review comment:
thanks, added
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
+ }
+
+ return PropertyUtil.propertiesWithPrefix(catalogProps, CatalogProperties.TABLE_OVERRIDE_PREFIX);
+ }
Review comment:
The null and empty checks were pushed down to `PropertyUtil.propertiesWithPrefix`, so these are not needed. Removed. These functions can be inlined too, however, keeping them here with java doc makes it a bit more readable. If there are strong opinion on this, I can inline these functions too.
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -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");
+ 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);
+
+ 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() {
Review comment:
I initially kept separate tests to make test failures communicate which specific test failed. But, we can do that with assert messages as well, so adopted this suggestion and added assert messages to explain failures.
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
Review comment:
Added java doc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r820392408
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -445,4 +448,9 @@ public void close() throws IOException {
public void setConf(Configuration conf) {
this.hadoopConf = conf;
}
+
+ @Override
+ protected Map<String, String> properties() {
Review comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] singhpk234 commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r820378331
##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
##########
@@ -445,4 +448,9 @@ public void close() throws IOException {
public void setConf(Configuration conf) {
this.hadoopConf = conf;
}
+
+ @Override
+ protected Map<String, String> properties() {
Review comment:
[question] Looks like we are missing test cases for Glue catalog ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#issuecomment-1025235929
@jackye1995 @rdblue @anuragmantri can you help review this, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r800238563
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> props = catalogProps == null ? Collections.emptyMap() : catalogProps;
+
+ return props.entrySet().stream()
+ .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(CatalogProperties.TABLE_DEFAULT_PREFIX))
Review comment:
Why make this case insensitive? I don't think there are other examples of table properties being case insensitive.
This is also broken because although this finds properties using a case insensitive prefix lookup, removing the prefix is not case insensitive.
I think this should just use `startsWith` and not worry about case sensitivity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rajarshisarkar commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rajarshisarkar commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r826608670
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +73,26 @@ public static String propertyAsString(Map<String, String> 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<String, String> propertiesWithPrefix(Map<String, String> properties,
+ String prefix) {
+ if (properties == null || properties.isEmpty()) {
+ 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().replaceFirst(prefix, ""), Map.Entry::getValue));
+ }
Review comment:
We recently took this method in `master` to extract a map having s3 write tags prefix (https://github.com/apache/iceberg/pull/4259).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r800239506
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> 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<String, String> tableOverrideProperties() {
+ Map<String, String> 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<String, String> updateTableProperties(Map<String, String> tableProperties) {
Review comment:
I find it awkward how this is done outside the existing builder. Why not change how properties are built there instead? It would be pretty clean to apply the defaults in the builder's constructor, and to add the overrides in the `create`/`createTransaction`/`newReplaceTableTransaction` methods.
That way you only have one way of accumulating properties for a table, rather than building the property map and then rebuilding it immediately afterward. You'll need to change the `ImmutableMap` builder to a regular map (`Maps.newHashMap`) and then use `ImmutableMap.copyOf` in place of the build step.
Then you can also use `putAll(Map<String, String>)`, which is a bit simpler than these streams.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r800237975
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
Review comment:
Why is this necessary? Won't this be handled in `BaseMetastoreCatalogTableBuilder.create`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r802935212
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> 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<String, String> tableOverrideProperties() {
+ Map<String, String> 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<String, String> updateTableProperties(Map<String, String> tableProperties) {
Review comment:
Hey @kbendick , thanks, I was having trouble coming up with a good name 😀. maybe `updateWithCatalogDerivedTableProps`? `combine` makes me think we are only merging (and not overriding), but we are.
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> 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<String, String> tableOverrideProperties() {
+ Map<String, String> 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<String, String> updateTableProperties(Map<String, String> tableProperties) {
Review comment:
Thanks, this makes sense.
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
Review comment:
You are right, this is not needed. Removing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540078
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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;
+ }
Review comment:
Rather than keeping track of properties in two places and requiring subclasses to call `super.initialize`, I think it would be better to add a new method to override, `properties`, that returns the properties map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810539882
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
+ }
Review comment:
This check is done in `propertiesWithPrefix` so there is no need to do it here or in the method above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810539882
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -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();
+ }
Review comment:
This check is done in `propertiesWithPrefix` so there is no need to do it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] SinghAsDev commented on pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
SinghAsDev commented on pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#issuecomment-1025282448
@rdblue thanks for the review, updated to keep changes to hive and hadoop catalogs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r800239506
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -238,4 +249,63 @@ protected static String fullTableName(String catalogName, TableIdentifier identi
return sb.toString();
}
+
+ @Override
+ public Table createTable(
+ TableIdentifier identifier,
+ Schema schema,
+ PartitionSpec spec,
+ String location,
+ Map<String, String> 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<String, String> tableDefaultProperties() {
+ Map<String, String> 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<String, String> tableOverrideProperties() {
+ Map<String, String> 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<String, String> updateTableProperties(Map<String, String> tableProperties) {
Review comment:
I find it awkward how this is done outside the existing builder. Why not change how properties are built there instead? It would be pretty clean to apply the defaults in the builder's constructor, and to add the overrides in the `create`/`createTransaction`/`newReplaceTableTransaction` methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810541080
##########
File path: hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -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");
+ 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);
+
+ 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() {
Review comment:
I think these tests can be simplified into a single case. This should use:
* One default key that is not overridden by either table properties or overrides
* One default key that is overridden by a table property
* One default key that is overridden by an override property
* One default key that is overridden by a table property and then overridden by an override property
* One table key that is not overridden by an override property
* One table key that is overridden by an override property
* One override property that doesn't override anything
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540646
##########
File path: core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java
##########
@@ -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());
Review comment:
Style: In Iceberg, try to wrap lines at the highest semantic level possible, rather than trying to use the whole line. In this case, it is awkward that `temp.newFolder()...` is on its own line because it's inside a method call.
I'd change it to this:
```java
hadoopCatalog.initialize("hadoop",
ImmutableMap.<String, String>builder()
.putAll(catalogProperties)
.put(CatalogProperties.WAREHOUSE_LOCATION, temp.newFolder().getAbsolutePath())
.build());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r810540260
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +72,16 @@ public static String propertyAsString(Map<String, String> properties,
}
return defaultValue;
}
+
+ public static Map<String, String> propertiesWithPrefix(Map<String, String> properties,
+ String prefix) {
Review comment:
I agree with this suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r795267170
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -388,6 +392,59 @@ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
default void initialize(String name, Map<String, String> properties) {
}
+ /**
+ * Get catalog properties.
+ *
+ * @return catalog properties
+ */
+ Map<String, String> properties();
+
+ /**
+ * Get default table properties set at Catalog level through catalog properties.
+ *
+ * @return default table properties specified in catalog properties
+ */
+ default Map<String, String> tableCreateDefaultProperties() {
+ String tableCreateDefaultPrefix = "table.create.default.";
+ Map<String, String> props = properties() == null ? Collections.emptyMap() : properties();
+
+ return props.entrySet().stream()
+ .filter(e -> e.getKey().toLowerCase(Locale.ROOT).startsWith(tableCreateDefaultPrefix))
+ .collect(Collectors.toMap(e -> e.getKey().replace(tableCreateDefaultPrefix, ""),
+ Map.Entry::getValue));
+ }
+
+ /**
+ * Get table properties that are enforced at Catalog level through catalog properties.
+ *
+ * @return default table properties enforced through catalog properties
+ */
+ default Map<String, String> tableCreateEnforcedProperties() {
+ String tableCreateEnforcedPrefix = "table.create.enforced.";
Review comment:
I think we need a better name for this. This is really long and will be embedded in nested configs (for Spark) and will contain nested configs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r795267239
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -388,6 +392,59 @@ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
default void initialize(String name, Map<String, String> properties) {
}
+ /**
+ * Get catalog properties.
+ *
+ * @return catalog properties
+ */
+ Map<String, String> properties();
Review comment:
These methods don't need to change `Catalog`. This is something a catalog implementation can define.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4011: Allow table defaults to be configured and/ or enforced at catalog level using catalog properties.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r835956242
##########
File path: core/src/main/java/org/apache/iceberg/util/PropertyUtil.java
##########
@@ -70,4 +73,26 @@ public static String propertyAsString(Map<String, String> 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<String, String> propertiesWithPrefix(Map<String, String> properties,
+ String prefix) {
+ if (properties == null || properties.isEmpty()) {
+ 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().replaceFirst(prefix, ""), Map.Entry::getValue));
+ }
Review comment:
Looks like this can be removed. Thanks, @rajarshisarkar!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org