Core: Unimplement Map from CharSequenceMap to obey contract

`CharSequenceMap` did not follow `Map` contract fully

- `Map.keySet` is supposed to be a view, but `CharSequenceMap` returned
  a copy. Fixing this one can be hard given how `CharSequenceMap` is
  implemented internally.
- `Map.equals` should compare maps by value. I.e. different map
  implementations with same key/value pairs are supposed to be equal.
  `CharSequenceMap` could certainly implement this correctly, but it
  does not today. The benefit of doing so is unclear and time necessary
  to do so correctly is non-negligible (we would need to define
  semantics what to when `CharSequenceMap` is compared to a map with
  more keys, but such that each key compared as char sequences is equal
  to some key in the `CharSequenceMap`).

Also, having a Map implementation that does not compare the keys by
equality is error-prone:

- it's likely that new default Map interface methods assume
  equality-based comparison semantics. JDK maintains non-equality based
  Map implementation (Comparator-based and Identity-based), and it
  surely doesn't go without maintenance burden (and not without bugs
  either, e.g. https://bugs.openjdk.org/browse/JDK-8178355)
- it's not unlikely someone tries to make a defensive copy of a map with
  `ImmutableMap.copyOf` or `Map.copyOf`, resulting in a map with
  different semantics.

Instead of fixing the `CharSequenceMap` to obey the `Map` contract, it
seems more pragmatic to just stop implementing the `Map` interface,
which this commit does, resulting in a simple to understand class
contract and behavior.

Additional positive side-effects of the change

- less code and easier to understand contracts
- strongly typed `get` and `containsKey` methods (the need to accept
  `Object` in these was forced by `Map` interface).

`CharSequenceSet` has similar issues, to be addressed in a follow-up.

.palantir/revapi.yml

@@ -1145,6 +1145,134 @@ acceptedBreaks:
       new: "method org.apache.iceberg.BaseMetastoreOperations.CommitStatus org.apache.iceberg.BaseMetastoreTableOperations::checkCommitStatus(java.lang.String,\
         \ org.apache.iceberg.TableMetadata)"
       justification: "Removing deprecated code"
+  "1.7.0":
+    org.apache.iceberg:iceberg-api:
+    - code: "java.class.noLongerImplementsInterface"
+      old: "class org.apache.iceberg.util.CharSequenceMap<V extends java.lang.Object>"
+      new: "class org.apache.iceberg.util.CharSequenceMap<V extends java.lang.Object>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.parameterTypeChanged"
+      old: "parameter V java.util.Map<K, V>::getOrDefault(===java.lang.Object===,\
+        \ V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      new: "parameter V org.apache.iceberg.util.CharSequenceMap<V>::getOrDefault(===java.lang.CharSequence===,\
+        \ V)"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.parameterTypeChanged"
+      old: "parameter V org.apache.iceberg.util.CharSequenceMap<V>::get(===java.lang.Object===)"
+      new: "parameter V org.apache.iceberg.util.CharSequenceMap<V>::get(===java.lang.CharSequence===)"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.parameterTypeChanged"
+      old: "parameter V org.apache.iceberg.util.CharSequenceMap<V>::remove(===java.lang.Object===)"
+      new: "parameter V org.apache.iceberg.util.CharSequenceMap<V>::remove(===java.lang.CharSequence===)"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.parameterTypeChanged"
+      old: "parameter boolean org.apache.iceberg.util.CharSequenceMap<V>::containsKey(===java.lang.Object===)"
+      new: "parameter boolean org.apache.iceberg.util.CharSequenceMap<V>::containsKey(===java.lang.CharSequence===)"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map.Entry<K, V> java.util.Map<K, V>::entry(K,\
+        \ V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::copyOf(java.util.Map<?\
+        \ extends K, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of() @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V)\
+        \ @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
+        \ K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
+        \ K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
+        \ K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
+        \ K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
+        \ K, V, K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
+        \ K, V, K, V, K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
+        \ K, V, K, V, K, V, K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::of(K, V, K, V,\
+        \ K, V, K, V, K, V, K, V, K, V, K, V, K, V, K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method <K, V> java.util.Map<K, V> java.util.Map<K, V>::ofEntries(java.util.Map.Entry<?\
+        \ extends K, ? extends V>[]) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method V java.util.Map<K, V>::compute(K, java.util.function.BiFunction<?\
+        \ super K, ? super V, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method V java.util.Map<K, V>::computeIfAbsent(K, java.util.function.Function<?\
+        \ super K, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method V java.util.Map<K, V>::computeIfPresent(K, java.util.function.BiFunction<?\
+        \ super K, ? super V, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method V java.util.Map<K, V>::merge(K, V, java.util.function.BiFunction<?\
+        \ super V, ? super V, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method V java.util.Map<K, V>::putIfAbsent(K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method V java.util.Map<K, V>::replace(K, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method boolean java.util.Map<K, V>::remove(java.lang.Object, java.lang.Object)\
+        \ @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method boolean java.util.Map<K, V>::replace(K, V, V) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method boolean org.apache.iceberg.util.CharSequenceMap<V>::containsValue(java.lang.Object)"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method java.util.Set<java.lang.CharSequence> org.apache.iceberg.util.CharSequenceMap<V>::keySet()"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method java.util.Set<java.util.Map.Entry<java.lang.CharSequence, V>> org.apache.iceberg.util.CharSequenceMap<V>::entrySet()"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method void java.util.Map<K, V>::forEach(java.util.function.BiConsumer<?\
+        \ super K, ? super V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method void java.util.Map<K, V>::replaceAll(java.util.function.BiFunction<?\
+        \ super K, ? super V, ? extends V>) @ org.apache.iceberg.util.CharSequenceMap<V>"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
+    - code: "java.method.removed"
+      old: "method void org.apache.iceberg.util.CharSequenceMap<V>::putAll(java.util.Map<?\
+        \ extends java.lang.CharSequence, ? extends V>)"
+      justification: "CharSequenceMap no longer attempts to implement the Map interface"
   apache-iceberg-0.14.0:
     org.apache.iceberg:iceberg-api:
     - code: "java.class.defaultSerializationChanged"

api/src/main/java/org/apache/iceberg/util/CharSequenceMap.java

@@ -20,29 +20,32 @@
 
 import java.io.Serializable;
 import java.util.Collection;
+import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Objects;
-import java.util.Set;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
 import org.apache.iceberg.relocated.com.google.common.collect.Maps;
-import org.apache.iceberg.relocated.com.google.common.collect.Sets;
 
 /**
- * A map that uses char sequences as keys.
+ * A map-like data structure that uses char sequences as keys and compares them by char sequence
+ * value they represent.
  *
  * <p>This implementation wraps provided keys into {@link CharSequenceWrapper} for consistent
  * hashing and equals behavior. This ensures that objects of different types that represent the same
  * sequence of characters are treated as equal keys in the map.
  *
- * <p>Note: This map is not designed for concurrent modification by multiple threads. However, it
- * supports safe concurrent reads, assuming there are no concurrent writes.
+ * <p>Note: CharSequenceMap is not designed for concurrent modification by multiple threads.
+ * However, it supports safe concurrent reads, assuming there are no concurrent writes.
  *
- * <p>Note: This map does not support null keys.
+ * <p>Note: CharSequenceMap does not support null keys.
  *
  * @param <V> the type of values
  */
-public class CharSequenceMap<V> implements Map<CharSequence, V>, Serializable {
+public class CharSequenceMap<V> implements Serializable {
 
   private static final long serialVersionUID = 1L;
   private static final ThreadLocal<CharSequenceWrapper> WRAPPERS =
@@ -58,101 +61,63 @@ public static <T> CharSequenceMap<T> create() {
     return new CharSequenceMap<>();
   }
 
-  @Override
   public int size() {
     return wrapperMap.size();
   }
 
-  @Override
   public boolean isEmpty() {
     return wrapperMap.isEmpty();
   }
 
-  @Override
-  public boolean containsKey(Object key) {
-    if (key instanceof CharSequence) {
-      CharSequenceWrapper wrapper = WRAPPERS.get();
-      boolean result = wrapperMap.containsKey(wrapper.set((CharSequence) key));
-      wrapper.set(null); // don't hold a reference to the key
-      return result;
-    }
-
-    return false;
+  public boolean containsKey(CharSequence key) {
+    CharSequenceWrapper wrapper = WRAPPERS.get();
+    boolean result = wrapperMap.containsKey(wrapper.set(key));
+    wrapper.set(null); // don't hold a reference to the key
+    return result;
   }
 
-  @Override
-  public boolean containsValue(Object value) {
-    return wrapperMap.containsValue(value);
+  public V get(CharSequence key) {
+    CharSequenceWrapper wrapper = WRAPPERS.get();
+    V result = wrapperMap.get(wrapper.set(key));
+    wrapper.set(null); // don't hold a reference to the value
+    return result;
   }
 
-  @Override
-  public V get(Object key) {
-    if (key instanceof CharSequence) {
-      CharSequenceWrapper wrapper = WRAPPERS.get();
-      V result = wrapperMap.get(wrapper.set((CharSequence) key));
-      wrapper.set(null); // don't hold a reference to the value
-      return result;
-    }
-
-    return null;
+  public V getOrDefault(CharSequence filePath, V defaultValue) {
+    return MoreObjects.firstNonNull(get(filePath), defaultValue);
   }
 
-  @Override
   public V put(CharSequence key, V value) {
     return wrapperMap.put(CharSequenceWrapper.wrap(key), value);
   }
 
-  @Override
-  public V remove(Object key) {
-    if (key instanceof CharSequence) {
-      CharSequenceWrapper wrapper = WRAPPERS.get();
-      V result = wrapperMap.remove(wrapper.set((CharSequence) key));
-      wrapper.set(null); // don't hold a reference to the value
-      return result;
-    }
-
-    return null;
-  }
-
-  @Override
-  public void putAll(Map<? extends CharSequence, ? extends V> otherMap) {
-    otherMap.forEach(this::put);
+  public V remove(CharSequence key) {
+    CharSequenceWrapper wrapper = WRAPPERS.get();
+    V result = wrapperMap.remove(wrapper.set(key));
+    wrapper.set(null); // don't hold a reference to the value
+    return result;
   }
 
-  @Override
   public void clear() {
     wrapperMap.clear();
   }
 
-  @Override
-  public Set<CharSequence> keySet() {
-    CharSequenceSet keySet = CharSequenceSet.empty();
-
-    for (CharSequenceWrapper wrapper : wrapperMap.keySet()) {
-      keySet.add(wrapper.get());
-    }
-
-    return keySet;
+  public List<CharSequence> keys() {
+    return wrapperMap.keySet().stream().map(CharSequenceWrapper::get).collect(Collectors.toList());
   }
 
-  @Override
   public Collection<V> values() {
     return wrapperMap.values();
   }
 
-  @Override
-  public Set<Entry<CharSequence, V>> entrySet() {
-    Set<Entry<CharSequence, V>> entrySet = Sets.newHashSet();
-
-    for (Entry<CharSequenceWrapper, V> entry : wrapperMap.entrySet()) {
-      entrySet.add(new CharSequenceEntry<>(entry));
-    }
-
-    return entrySet;
+  public Stream<Entry<CharSequence, V>> entries() {
+    return wrapperMap.entrySet().stream()
+        .map(entry -> Maps.immutableEntry(entry.getKey().get(), entry.getValue()));
   }
 
   public V computeIfAbsent(CharSequence key, Supplier<V> valueSupplier) {
-    return computeIfAbsent(key, ignored -> valueSupplier.get());
+    return wrapperMap.computeIfAbsent(
+        CharSequenceWrapper.wrap(key), ignored -> valueSupplier.get());
   }
 
   @Override
@@ -174,52 +139,12 @@ public int hashCode() {
 
   @Override
   public String toString() {
-    return entrySet().stream().map(this::toString).collect(Collectors.joining(", ", "{", "}"));
+    return entries().map(this::toString).collect(Collectors.joining(", ", "{", "}"));
   }
 
   private String toString(Entry<CharSequence, V> entry) {
     CharSequence key = entry.getKey();
     V value = entry.getValue();
     return key + "=" + (value == this ? "(this Map)" : value);
   }
-
-  private static class CharSequenceEntry<V> implements Entry<CharSequence, V> {
-    private final Entry<CharSequenceWrapper, V> inner;
-
-    private CharSequenceEntry(Entry<CharSequenceWrapper, V> inner) {
-      this.inner = inner;
-    }
-
-    @Override
-    public CharSequence getKey() {
-      return inner.getKey().get();
-    }
-
-    @Override
-    public V getValue() {
-      return inner.getValue();
-    }
-
-    @Override
-    public int hashCode() {
-      return inner.hashCode();
-    }
-
-    @Override
-    public boolean equals(Object other) {
-      if (this == other) {
-        return true;
-      } else if (other == null || getClass() != other.getClass()) {
-        return false;
-      }
-
-      CharSequenceEntry<?> that = (CharSequenceEntry<?>) other;
-      return inner.equals(that.inner);
-    }
-
-    @Override
-    public V setValue(V value) {
-      throw new UnsupportedOperationException("Cannot set value");
-    }
-  }
 }