add generic implementation of bidirectional map #303
Conversation
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: logicalhan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
|
I don't actually think the apidiff compatibility check is valid here. Any concrete usage of the set library should continue to work, since the interface itself has not changed it's underlying signature. |
There was a problem hiding this comment.
Without a concrete use case in k/k I don't know if we want to check this in. Do we have one?
I'm not sure on names. Having a pkg per type feels bad, but it's not clear what package this should be in.
Go has strings and slices. We have set (which should probably be sets). Maybe maps ?
Would BiMap be sufficient? Just looking for shorter. Not critical
| "k8s.io/utils/set" | ||
| ) | ||
|
|
||
| // BidirectionalMap is a bidirectional map. |
There was a problem hiding this comment.
I would flesh this out more. What is the semantic, what guarantees does it offer or not offer, what is it for (a real example)? Also explaion which is the left-key and right-value vs. the left-value and right-key.
Naively I would expect BiDiMap[X, Y] to mean map[X]Y + Map[Y]X, but this seems to be the opposite
There was a problem hiding this comment.
I'm not sure what you mean by opposite, map[X]Y + Map[Y]X can be invalidated by have multiple keys in map[x]y map to the same y value. e.g.:
a = map[string]string
a["a"] = "1"
a["b"] = "1"
The reverse mapping must return a list of the keys of map a otherwise we have a lossy reverse map.
Therefore, a bidirectional map must be two maps, each of which map to a list (or set) of values.
bidirectionalmap/map.go
Outdated
| } | ||
| } | ||
|
|
||
| // InsertRight inserts a new item into the right map. |
| import "testing" | ||
|
|
||
| func TestMultipleInserts(t *testing.T) { | ||
| bidimap := NewBidirectionalMap[string, string]() |
There was a problem hiding this comment.
Examples with different types would better illustrate.
You could add an example_test.go to get a nice godoc example
furthermore, this PR extracts out the generic interfaces we use in the generic set implementation so that we can reuse them in the bidirectional map.
| ss := Set[E]{} | ||
| ss.Insert(items...) | ||
| return ss | ||
| } | ||
|
|
||
| // KeySet creates a Set[E] from a keys of a map[E](? extends interface{}). | ||
| func KeySet[E ordered, A any](theMap map[E]A) Set[E] { | ||
| func KeySet[E genericinterfaces.Ordered, A any](theMap map[E]A) Set[E] { |
There was a problem hiding this comment.
What's the motivation for exporting the type? are we expecting other things to reference the interface? They couldn't previously, which had the benefit of only allowing people to declare KeySet with their own types, which had to be compatible with our constraints... that seemed reasonable to me.
There was a problem hiding this comment.
I'd need to redeclare the exact same type in the maps package otherwise, since the output of the value of a map is a generic set.
There was a problem hiding this comment.
How about waiting for the bump to Go 1.21 in utils and relying on cmp.Ordered? See also #295.
furthermore, this PR extracts out the generic interfaces we use in the generic set implementation so that we can reuse them in the bidirectional map.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Generics provide a path to implementing generic implementations of arbitrary (but useful) data-structures. This PR creates a bidirectional map.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: