kasakrisz commented on code in PR #5498:
URL: https://github.com/apache/hive/pull/5498#discussion_r1810265995
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -129,19 +129,19 @@ public static Table getTable(Configuration configuration,
org.apache.hadoop.hive
*/
static Table getTable(Configuration configuration, Properties properties,
boolean skipCache) {
String metaTable =
properties.getProperty(IcebergAcidUtil.META_TABLE_PROPERTY);
- String tableName = properties.getProperty(Catalogs.NAME);
- String location = properties.getProperty(Catalogs.LOCATION);
+ Properties props = (metaTable != null) ? new Properties() : properties;
+
if (metaTable != null) {
+ props.computeIfAbsent(InputFormatConfig.CATALOG_NAME, properties::get);
// HiveCatalog, HadoopCatalog uses NAME to identify the metadata table
- properties.setProperty(Catalogs.NAME, tableName + "." + metaTable);
+ props.computeIfAbsent(Catalogs.NAME, k -> properties.get(k) + "." +
metaTable);
// HadoopTable uses LOCATION to identify the metadata table
- properties.setProperty(Catalogs.LOCATION, location + "#" + metaTable);
+ props.computeIfAbsent(Catalogs.LOCATION, k -> properties.get(k) + "#" +
metaTable);
}
Review Comment:
These are the `if`s in new code:
* `(metaTable != null) ? new Properties() : properties`
* `if (metaTable != null) { ... }`
* 3 `computeIfAbsent` calls
When `metaTable` present a new empty `Properties` instance is created and
`computeIfAbsent` is always absent.
Ternary operator is one line but it still an if-else with the same condition
like the follow-up if statement.
I agree that the side affect you mentioned should be fixed. It can be
achieved by only one if statement:
```
Properties props;
if (metaTable != null) {
props = new Properties();
props.put(InputFormatConfig.CATALOG_NAME,
properties.get(InputFormatConfig.CATALOG_NAME));
props.put(Catalogs.NAME, properties.get(Catalogs.NAME) + "." +
metaTable);
props.put(Catalogs.LOCATION, properties.get(Catalogs.LOCATION) + "#" +
metaTable);
} else {
props = properties;
}
```
The else block can also be removed:
```
Properties props = new Properties(properties); // use input properties as
default
if (metaTable != 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: [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]
