findepi commented on code in PR #11304:
URL: https://github.com/apache/iceberg/pull/11304#discussion_r1797751372
##########
api/src/main/java/org/apache/iceberg/util/CharSequenceMap.java:
##########
@@ -132,12 +133,12 @@ public Set<CharSequence> keySet() {
keySet.add(wrapper.get());
}
- return keySet;
+ return Collections.unmodifiableSet(keySet);
Review Comment:
`Map.keySet` must be a view of the map, so removals from this collection
should affect the maps.
The code before the changes was not correct, because it was not a view and
it allowed removals, and removals wouldn't affect the map.
The code after the change is less incorrect: the returned Set is still not a
view, but it doesn't allow modifications (so _all_ modifications do affect the
map).
A better change would be to fix this map
##########
api/src/main/java/org/apache/iceberg/util/CharSequenceMap.java:
##########
@@ -132,12 +133,12 @@ public Set<CharSequence> keySet() {
keySet.add(wrapper.get());
}
- return keySet;
+ return Collections.unmodifiableSet(keySet);
Review Comment:
A simple way to make `CharSequenceMap` obey `Map` contract (which is super
important!) is to re-use existing implementation. `TreeMap` is one that can be
used for that.
The whole class can therefore be replaced with:
```java
/**
* A map that uses char sequences as keys and compares them by value.
*
* @param <V> the type of values
*/
public abstract class CharSequenceMap<V> implements Map<CharSequence, V>,
Serializable {
public static <V> Map<CharSequence, V> create() {
return new TreeMap<>(CharSequence::compare);
}
}
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
