findepi opened a new pull request, #11704: URL: https://github.com/apache/iceberg/pull/11704
`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. - supersedes https://github.com/apache/iceberg/pull/11304 - supersedes https://github.com/apache/iceberg/pull/11308 cc @anuragmantri @aokolnychyi @amogh-jahagirdar @nastra -- 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
