Prefer Guava's Collectors over JDK's

[ssheikin]

FYI:

kokosing@m16:~/presto$ git grep 'toImmutableMap(' | wc -l
202
kokosing@m16:~/presto$ git grep 'toMap(' | wc -l
95

Originally posted by @kokosing in https://github.com/trinodb/trino/issues/6761#issuecomment-771581751

[hashhar]

Note to whoever implements this, there is a semantic difference between Guava ImmutableMap and other maps. The Guava ImmutableMap doesn't allow null values. Generally that is a good thing but the existing code in some places does put null values into the map. See https://github.com/trinodb/trino/pull/6429#pullrequestreview-557899879 for a similar previous example.

~Another difference is that if using stream()....collect(toImmutableMap(...)) then an exception will be thrown in case of duplicate keys in the stream.~

[kokosing]

Also keep in mind that SPI is not using guava.

Also: https://github.com/trinodb/trino/blob/master/DEVELOPMENT.md#prefer-guava-immutable-collections

[ruiyang00]

Hello @kokosing @ssheikin, I am new to the Trinodb community and this is my first time trying to contribute to the open-source community. I will be appreciated if anyone could confirm with me that the task for this issue. Do we only replace the toMap type to toImmutableMap in all the return statements or do we replace all the occurrence of toMap to toImmutableMap?Because I notice that some occurrences of toMap are involving in other purposes but not part of the return statement.

Thanks

[kokosing]

Before we rush into this i would like others to share their thoughts. @martint @findepi ?

[ruiyang00]

Thank you @kokosing for your reply. I will wait for more info before I start making changes.

[kokosing]

@ruiyang00 welcome in community!

[ruiyang00]

Hello all, any updates on this issue? Thank you.

[ssheikin]

@ruiyang00, this issue is about substitution of java.util.stream.Collectors to their immutable alternatives from guava library, but you have to be very careful because some occurrences cannot be substituted. See: https://github.com/trinodb/trino/issues/6800#issuecomment-771603856

[ruiyang00]

@ssheikin thank you. I see what you mean. I noticed you've just changed this issue's title. I will really appreciate it if you could confirm with me one more time. The work of this issue is to change all the occurrences of toMap() of java.util.stream.Collectors to Google's Guava toimmutableMap(). These occurrences that have null passed as args should not be changed. If this is correct, then I will dive into this issue right now. Hopefully, my questions won't bother you guys since I am new.