HashMap based on Eq and Object#hashCode

[s5bug]

For when there's an Eq with no Order. I'm not the best at designing HashMaps nor cats-centric collections; I'm opening as a draft PR in hopes of live feedback.

[codecov-commenter]

Codecov Report

Merging #304 into master will decrease coverage by 0.89%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   91.56%   90.67%   -0.90%     
==========================================
  Files          24       24              
  Lines        1625     1641      +16     
  Branches      214      211       -3     
==========================================
  Hits         1488     1488              
- Misses        137      153      +16     

Impacted Files	Coverage Δ
core/src/main/scala/cats/collections/Map.scala	39.28% <0.00%> (-15.72%)	⬇️
core/src/main/scala/cats/collections/BitSet.scala	97.36% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b1686d...3d7614f. Read the comment docs.

[s5bug]

How do I add tests? SetSpec refers specifically to AvlSet (same for MapSpec/AvlMap).

[larsrh]

Hi @sorenbug and thanks for your contribution! The current set/map files should really be prefixed with Avl (this includes the tests).

As for your contribution, I recommend putting them into separate files, such as HashMap.scala. I can send a follow-up PR that renames the existing files.

To write tests, I think you could largely copy the existing tests and change occurrences of AvlSet to your HashSet.

[s5bug]

Sounds good! I will do so.

[johnynek]

Also, @sorenbug I think we'd want to require Hash[K] not Eq[K]:

https://github.com/typelevel/cats/blob/master/kernel/src/main/scala/cats/kernel/Hash.scala

We don't want to use universal hashing .hashCode in a typeclass based library like this one.

Lastly, getting a high performance HashMap will be a bit tricky, but I think directly implementing this via the HAMT approach:
https://en.wikipedia.org/wiki/Hash_array_mapped_trie

vs using a Vector, will be more performant, but maybe not... it will be fine.

In any case, if we make the details private, we can merge it with the right API and optimize later. I think we want a minimal API and as much as possible private.

Something like:

trait HashMap[K, +V] {
  def get(k: K)(implicit hash: Hash[K]): Option[V]
  def updated[V1 >: V](k: K, v: V1)(implicit hash: Hash[K]): HashMap[K, V1]
  // return the given value of the key and the state of the map without it
  def take(k: K)(implicit hash: Hash[K]): (Option[V], HashMap[K, V])
  def length: Long
  def iterator: Iterator[(K, V)]
 
  // without having to hash the key twice, transform a given key
  def transformKey[R, V1 >: V](k: K)(fn: Option[V1] => (R, Option[V1])): (R, HashMap[K, V1]) 

  // transform the values with the key given as input,but we cannot change the key
   // this can be faster since you don't change the structure or rehash the keys
  def mapWithKey[V1](fn: (K, V) => V1): HashMap[K, V1]
}

object HashMap {
  def empty[K, V]: HashMap[K, V]
}

[s5bug]

Ah! I couldn't find a Hash when looking for it earlier. Thank you!

Re HAMT: I didn't know how to make a HashMap so I just copied a Java example I found online. I'll check it out.

[s5bug]

So, the new typeclass dependencies for some things seem a little odd. I haven't implemented a HAMT as of yet, but that'll be next. I'd like a review on my usage of Hash so far though.

[s5bug]

The Wikipedia page is not helping for how to implement a HAMT, and my data structures course has nothing on it. What should I look at to implement one?

[codecov-io]

Codecov Report

Merging #304 (a5f2638) into master (fd2d97f) will decrease coverage by 1.76%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   91.37%   89.61%   -1.77%     
==========================================
  Files          24       26       +2     
  Lines        1623     1656      +33     
  Branches      216      215       -1     
==========================================
+ Hits         1483     1484       +1     
- Misses        140      172      +32     

Impacted Files	Coverage Δ
core/src/main/scala/cats/collections/AvlMap.scala	55.00% <ø> (ø)
core/src/main/scala/cats/collections/AvlSet.scala	88.69% <ø> (ø)
core/src/main/scala/cats/collections/HashMap.scala	0.00% <0.00%> (ø)
core/src/main/scala/cats/collections/HashSet.scala	0.00% <0.00%> (ø)
...ore/src/main/scala/cats/collections/TreeList.scala	99.65% <0.00%> (-0.35%)	⬇️
core/src/main/scala/cats/collections/Dequeue.scala	56.63% <0.00%> (+2.65%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd2d97f...a5f2638. Read the comment docs.

[larsrh]

@sorenbug Thanks for following up! From my PoV, this implementation is ready for review (even without a HAMT implementation). Only thing missing now is tests for HashSet and HashMap. Ideally, the ones from AvlSet and AvlMap can be copied.

[s5bug]

Alright, I learned how to make a HAMT: It seems like I'd want to basically copy stdlib's Vector and index the tries by the bits of the hash. I'll see what I can do soon to add that here.

The current implementation has O(n) lookup because there's no resizing logic, and it's not a good implementation for an immutable HashMap anyways: usually this implementation is used for mutable HashMaps, even though adding an element is O(1).

[johnynek]

I am closing this due to #534 but I appreciate you starting the ball rolling.

If you are still interested, I think there are still optimization opportunities in the hashmap code that was merged.

[s5bug]

Yeah, as much as I wanted to finish this up I never really got around to understanding how to make better HashMaps. Thanks for keeping up with things!