Use a bimap for reverse lookups on injective maps#5681
Use a bimap for reverse lookups on injective maps#5681drcrallen merged 3 commits intoapache:masterfrom dylwylie:lookup-bimap
Conversation
- A BiMap provides constant-time lookups for mapping values to keys
| if (this.map instanceof HashBiMap) { | ||
| return Lists.newArrayList(((HashBiMap<String, String>) this.map).inverse().get(value)); | ||
| } else { | ||
| return Lists.newArrayList(Maps.filterKeys(map, new Predicate<String>() |
There was a problem hiding this comment.
(minor) can this use java 8 lambdas like
key -> map.get(key).equals(Strings.nullToEmpty(value))
?
| { | ||
| this.map = Preconditions.checkNotNull(map, "map"); | ||
| this.isOneToOne = isOneToOne; | ||
| this.map = Preconditions.checkNotNull(this.isOneToOne ? HashBiMap.create(map) : map, "map"); |
There was a problem hiding this comment.
It looks like HashBiMap#inverse creates a new object in a not thread safe manner, but the object is pretty light weight. Due to the nature of lookups this might cause a thundering of racy objects, which should be benign since the map is immutable, but annoying. Would it make sense to create the inverse map as a Map<String, String> eagerly, and null the object if not one-to-one, then in the unapply simply check for the eager inverse map being non null and do a Collections.singletonList on the result, if it exists?
There was a problem hiding this comment.
Nice catch - thanks!
| { | ||
| @Override public boolean apply(@Nullable String key) | ||
| if (this.map instanceof HashBiMap) { | ||
| return Lists.newArrayList(((HashBiMap<String, String>) this.map).inverse().get(value)); |
There was a problem hiding this comment.
from the Interface docs:
* @return the list of keys that maps to value or empty list.
* Note that for the case of a none existing value in the lookup we have to cases either return an empty list OR list with null element.
* returning an empty list implies that user want to ignore such a lookup value.
* In the other hand returning a list with the null element implies user want to map the none existing value to the key null.
I'm actually not 100% sure which one should be done here, @b-slim do you have any insight for this?
There was a problem hiding this comment.
I've modified to match what the existing implementation and other interface implementations (I think) do.
|
Hmm realise this might be naive, if we get passed in something like a MapDB offheap htreemap we'd be defeating the point of the off-heap collection by copying everything into the inverse offheap. Wondering about letting the caller pass in the reverse map. |
* Use a bimap for reverse lookups on injective maps - A BiMap provides constant-time lookups for mapping values to keys * Address comments * Fix Tests
|
Reverting this, the following code passes in an offHeap backed map from MapDb, so we'd inadvertently be copying all of its data onHeap when creating the BiMap |
* Revert "Consider waiting and pending compaction tasks as well as running tasks in DruidCoordinatorSegmentCompactor (#5704)" This reverts commit c7a5939. * Revert "Fix metrics for inserting segments (#5749)" This reverts commit c9d6451. * Revert "Typo fix in historical doc (#5753)" This reverts commit aa23fe6. * Revert "Use a bimap for reverse lookups on injective maps (#5681)" This reverts commit e1277d3.
2 participants
Using this as an excuse to play around with the code, comments welcome!