Core: Map methods should return immutable collections

[anuragmantri]

StructLikeMap and CharSequenceMap methods keySet(), values(), entrySet() return collections which when modified by consumers will change the underlying maps. We should instead return unmodifiable collections like in PartitionMap#keySet()

[anuragmantri]

TestStructLikeMap#testKeyAndEntrySetEquality() is failing. Investigating if equals() method needs some changes.

[anuragmantri]

StructLikeSet has an overridden equals() method which compared the classes. This will fail when we compare against an UnmodifiableSet. The equals() method is checking for StructType field which prevents us from casting it to parent Set. PartitionSet does not check for equality of the StructType field.

[findepi]

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

[findepi]

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:

/**
 * 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);
  }
}

[findepi]

I submitted this as a separate PR: #11308

[anuragmantri]

Thanks @findepi. Let me check the other map implementations and make sure they follow the Map contract.

[findepi]

Generally writing a good Map implementation from scratch is super hard.
I did that for Trino once because JDK's IdentityMap violates (violated?) some Map contract aspects, and it was ton of code and even more tests. Using mainstream implementations is really much saner.

[findepi]

Generally writing a good Map implementation from scratch is super hard.

... and sometimes not worth the effort
After all, one can have map-like class without implementing Map interface -- #11704

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.