kbendick commented on code in PR #4011:
URL: https://github.com/apache/iceberg/pull/4011#discussion_r875089971
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -110,6 +111,7 @@ public GlueCatalog() {
@Override
public void initialize(String name, Map<String, String> properties) {
+ this.catalogProperties = properties;
Review Comment:
Nit: Might want to wrapt his in `ImmutableMap.copyOf(properties)` (which is
a no-op if the underlying map is an `ImmutableMap`. Though again if you need
null-values then that won't work for you.
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -162,6 +164,13 @@ private FileIO initializeFileIO(Map<String, String>
properties) {
}
}
+ @VisibleForTesting
+ void initialize(String name, String path, AwsProperties properties,
GlueClient client,
+ LockManager lock, FileIO io, Map<String, String> catalogProps) {
Review Comment:
Nit / non-blocking: The indentation for this seems a little weird to me.
If all of the properties are on the next line, will it fit? I do see
conflicting things at times so it's not a huge deal, but I believe the
preferred way would be something more like:
```java
@VisibleForTesting
void initialize(String name, String path, AwsProperties properties,
GlueClient client,
LockManager lock, FileIO io, Map<String, String>
catalogProps) {
```
But again, I see different formats so without clarification, I'd either go
with the above, a single line (the next line) if they _all_ fit, or just leave
it unless somebody states otherwise. I'll follow up for more clarification, but
this is absolutley non-blocking.
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -106,7 +112,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> tableProperties = Maps.newHashMap();
Review Comment:
Is this needed for null-values? If so, can we ensure that we have tests that
include null values? Otherwise it's likely to get refactored at some point.
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -94,6 +96,10 @@ protected boolean isValidIdentifier(TableIdentifier
tableIdentifier) {
return true;
}
+ protected Map<String, String> properties() {
+ return Collections.emptyMap();
Review Comment:
Nit: If it's empty, usually we use `ImmutableMap.of()`. I see you updated
from `ImmutableMap.builder` so I assume you need null values, but I think even
still we'd normally use `ImmutableMap.of()` in the empty case.
##########
core/src/main/java/org/apache/iceberg/CatalogProperties.java:
##########
@@ -29,6 +29,8 @@ private CatalogProperties() {
public static final String CATALOG_IMPL = "catalog-impl";
public static final String FILE_IO_IMPL = "io-impl";
public static final String WAREHOUSE_LOCATION = "warehouse";
+ public static final String TABLE_DEFAULT_PREFIX = "table-default.";
+ public static final String TABLE_OVERRIDE_PREFIX = "table-override.";
Review Comment:
Nit / open-question: How do people feel about these names? I'd kind of like
them to have `properties` or something in them, but this might be overkill.
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java:
##########
@@ -93,12 +94,14 @@ public class HadoopCatalog extends BaseMetastoreCatalog
implements Closeable, Su
private FileIO fileIO;
private LockManager lockManager;
private boolean suppressPermissionError = false;
+ private Map<String, String> catalogProperties = Collections.emptyMap();
Review Comment:
Nit / non-blocking : Does this _need_ to be initialized? I'm assuming this
is to suppress potential not-initialized errors?
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java:
##########
@@ -393,6 +396,11 @@ public Configuration getConf() {
return conf;
}
+ @Override
+ protected Map<String, String> properties() {
+ return catalogProperties;
Review Comment:
Nit: For here, _if it will remove the not-initialized warning from
`catalogProperties`, we often do the following:
```java
return catalogProperies != null ? ImmutableMap.of() : catalogProperties;
```
--
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]
