smaheshwar-pltr commented on code in PR #12534:
URL: https://github.com/apache/iceberg/pull/12534#discussion_r1996771598
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -814,6 +814,20 @@ private Map<String, String> tableDefaultProperties() {
return tableDefaultProperties;
}
+ /**
+ * Get table properties that are enforced at Catalog level through catalog
properties.
+ *
+ * @return default table properties enforced through catalog properties
Review Comment:
(Minor / Docstring) These are overrides, not defaults - I think the other
docstrings in this PR might need changing too
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -744,6 +744,97 @@ public void testDefaultTablePropertiesReplaceTransaction()
{
assertThat(catalog.dropTable(ident)).as("Should successfully drop
table").isTrue();
}
+ @Test
+ public void testOverrideTableProperties() {
+ C catalog = catalog();
+
+ TableIdentifier ident = TableIdentifier.of("ns", "table");
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(ident.namespace());
+ }
+
+ assertThat(catalog.tableExists(ident)).as("Table should not
exist").isFalse();
+
+ Table table =
+ catalog()
+ .buildTable(ident, SCHEMA)
+ .withProperty("override-key4", "catalog-overridden-key4")
Review Comment:
Extreme nit: these tests look great but I'm bad at reading so at first was
confused by this 😄. Could rename value here and elsewhere to `table-key4` /
`view-key4` to be super explicit but do feel free to ignore
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -849,6 +863,7 @@ public Builder withProperty(String key, String value) {
@Override
public Table create() {
Endpoint.check(endpoints, Endpoint.V1_CREATE_TABLE);
+ propertiesBuilder.putAll(tableOverrideProperties());
Review Comment:
Nit from me (feel free to ignore): I wonder whether some private
`buildProperties: Map<String, String>` method that puts overrides and calls
`buildKeepingLast` to be used in the various terminal methods of this class is
cleaner, but not sure.
##########
core/src/test/java/org/apache/iceberg/inmemory/TestInMemoryCatalog.java:
##########
@@ -37,6 +37,15 @@ public void before() {
CatalogProperties.TABLE_DEFAULT_PREFIX + "default-key1",
"catalog-default-key1")
.put(
CatalogProperties.TABLE_DEFAULT_PREFIX + "default-key2",
"catalog-default-key2")
+ .put(
+ CatalogProperties.TABLE_DEFAULT_PREFIX + "override-key3",
+ "catalog-default-key3")
Review Comment:
Hmm I realise this has been done for previous PRs so can look past this, but
I wonder whether, given these catalog properties are specific to
`CatalogTests`, are *required* for them to pass, and aren't specific to
individual catalogs, they should instead be provided on the `CatalogTests`
themselves somehow instead of being hardcoded strings in each initialisation of
the `XCatalogTest` subclasses.
Maybe: Could provide some `CATALOG_PROPERTIES` map containing them on
`CatalogTests` that subclasses are contracted to initialise respective catalogs
with (along with those specific to that catalog e.g. JDBC username / password).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
