[GitHub] [iceberg] rdblue commented on a change in pull request #2258: Core : remove the UnmodifiableMap

[GitBox <gi...@apache.org>]

rdblue commented on a change in pull request #2258:
URL: https://github.com/apache/iceberg/pull/2258#discussion_r588809379



##########
File path: core/src/main/java/org/apache/iceberg/BaseFile.java
##########
@@ -434,7 +433,7 @@ public Integer sortOrderId() {
     if (map != null) {
       Map<K, V> copy = Maps.newHashMapWithExpectedSize(map.size());
       copy.putAll(map);
-      return Collections.unmodifiableMap(copy);

Review comment:
       @yyanyy is right. This is a method for defensive copies. I think it can be fine to do this, but we want to provide the same assurance that the data won't be modified. We can do that by moving the unmodifiable map creation to other places, but that is a big change and will result in a lot more object creation.
   
   Another option is to use an alternative wrapper class that can be cleanly serialized. Elsewhere in this class we use `SerializableByteBufferMap`. Does that work with Kryo? If so, can we use the same pattern here? Is that map modifiable?




----------------------------------------------------------------
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.

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