collado-mike commented on code in PR #2735:
URL: https://github.com/apache/polaris/pull/2735#discussion_r2400441818
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1566,14 +1566,19 @@ public void doCommit(TableMetadata base, TableMetadata
metadata) {
"Generic table with same name already exists: %s",
tableIdentifier);
}
}
+ Map<String, String> storedProperties =
buildTableMetadataPropertiesMap(metadata);
IcebergTableLikeEntity entity =
IcebergTableLikeEntity.of(resolvedPath == null ? null :
resolvedPath.getRawLeafEntity());
String existingLocation;
if (null == entity) {
existingLocation = null;
entity =
new IcebergTableLikeEntity.Builder(
- PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier,
newLocation)
+ PolarisEntitySubType.ICEBERG_TABLE,
+ tableIdentifier,
+ Map.of(),
Review Comment:
The problem is in setting the `internalProperties` map in the builder
methods because we have these helper methods, such as `setMetadataLocation`
that aren't fields themselves, but modify entries in the map. If we do call the
following, we're fine
```java
builder
.setInternalProperties(props)
.setMetadataLocation(newLocation)
.build();
```
but if we reverse the order and call the following, we're broken:
```java
builder
.setMetadataLocation(newLocation)
.setInternalProperties(props)
.build();
```
It's not obvious from the caller's perspective, but `setMetadataLocation`
modifies the underlying map, then the `setInternalProperties` call completely
overwrites the map, losing the value set in the previous call.
With the existing constructor, it's impossible to order the setting of the
`metadataLocation` and the `internalProperties` via the builder methods. If we
used the builder for all properties all the time, it would be fine, but because
we pass in the `newLocation` parameter as a constructor arg, if we set the
`internalProperties` field using the builder method, we lose the location
parameter we just passed in.
--
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]
