Disambiguate hash collisions in internal/hashmap via Hasher.Equal#542
Disambiguate hash collisions in internal/hashmap via Hasher.Equal#542PratikDhanave wants to merge 4 commits into
Conversation
The Hasher interface requires an Equal method and Map stores each entry's key, but Load, Delete and Set keyed purely on the hash value and never called Equal. When two distinct keys hashed to the same value: - Set overwrote the earlier key's entry (silent data loss), - Load returned another key's value — even for a key never stored, - Delete removed a different key's entry. Store entries in per-hash buckets and use Hasher.Equal to select the matching entry in Load, Delete and Set, honoring the interface contract the stored keys and Equal method were already there to support. No exported method signature changes. Adds the package's first test, covering distinct keys that share a hash.
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness issues in internal/hashmap where Load, Set, and Delete previously keyed only on the 64-bit hash and did not use Hasher.Equal, causing silent data corruption on hash collisions. It changes the internal storage to per-hash buckets and adds a focused regression test that forces collisions to verify correct behavior.
Changes:
- Store entries as
map[uint64][]entryto support multiple keys per hash. - Use
Hasher.Equalto disambiguate collisions inLoad,Set, andDelete(and update iterators/Stringaccordingly). - Add
TestMap_DistinctKeysWithCollidingHashCoexistto validate collision correctness.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/hashmap/hashmap.go | Switches to per-hash buckets and uses Equal to correctly handle hash collisions across map operations. |
| internal/hashmap/hashmap_test.go | Adds a regression test using a forced-collision hasher to ensure distinct colliding keys coexist and delete/load behave correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(bucket) == 1 { | ||
| delete(m.entries, hash) | ||
| } else { | ||
| m.entries[hash] = append(bucket[:i], bucket[i+1:]...) | ||
| } |
| // Len returns the number of map entries. | ||
| func (m *Map[K, V]) Len() int { | ||
| return len(m.entries) | ||
| n := 0 | ||
| for _, bucket := range m.entries { | ||
| n += len(bucket) | ||
| } | ||
| return n |
There was a problem hiding this comment.
Done — added an entry count updated in Set/Delete/Clear so Len() is O(1) again, and Delete now shifts the tail down and zeroes the vacated slot to avoid retaining the removed key/value.
Address review feedback: - Len() is O(1) again via an entry count updated in Set/Delete/Clear, instead of summing bucket lengths. - Delete shifts the tail down and zeroes the vacated slot so the removed key/value are not retained by the bucket's backing array. Adds a test covering Len across insert/update/delete/clear.
Problem
internal/hashmap'sHasherinterface requires anEqual(T, T) boolmethod, andMapstores each entry's key — butLoad,DeleteandSetkeyed purely on the 64-bit hash and never calledEqual. The stored key and theEqualmethod were dead unless used for collision disambiguation.When two distinct keys hash to the same value:
Deletesimilarly removed whatever entry shared the hash, not the one whose key actually matched.The hashers in use (e.g. the workflow scope/update-key hashers) are
maphash-based, so collisions are rare — but the map is a general-purpose keyed container and theEqualcontract exists precisely to make it correct regardless.Fix
Store entries in per-hash buckets (
map[uint64][]entry) and useHasher.Equalto select the matching entry inLoad,DeleteandSet. This honors the interface contract that the stored keys andEqualmethod were already there to support. No exported method signature changes. (Also fixes ath//ecomment typo.)Test
Adds the package's first test,
TestMap_DistinctKeysWithCollidingHashCoexist, using a hasher that forces all keys to one hash. It asserts distinct colliding keys coexist, a never-stored colliding key resolves to(zero, false), and deleting one colliding key leaves the other. Fails onmain(wrong values,Len()==1), passes with the fix. All existing workflow/checkpoint/state callers still pass.