RUST-283 Replace linked-hash-map with indexmap #247
Conversation
| @@ -39,11 +39,12 @@ decimal128 = ["decimal"] | |||
| name = "bson" | |||
|
|
|||
| [dependencies] | |||
| ahash = "0.7.2" | |||
There was a problem hiding this comment.
The ahash crate provides a fast yet safe hashing algorithm. In fact, I think it's now the default one used by std::HashMap: https://github.com/rust-lang/hashbrown/blob/master/Cargo.toml. In 2.0, indexmap looks like they will be updating their default to use it, so we can remove this dependency then (indexmap-rs/indexmap#135)
|
The clippy failures appear to be on master too, I'll fix them separately. |
| /// A view into a single entry in a map, which may either be vacant or occupied. | ||
| /// | ||
| /// This enum is constructed from the entry method on HashMap. | ||
| pub enum Entry<'a> { |
There was a problem hiding this comment.
I updated this to match both IndexMap and std::HashMap's Entry API. I updated just the parts that are breaking and left the remaining bits to future, additive work. I'll file a ticket once this is merged for us to remember to do the rest of it. It's basically just adding a few methods to OccupiedEntry and VacantEntry.
There was a problem hiding this comment.
from looking at the docs for std::HashMap's OccupiedEntry type, I assume that will cover a way to obtain the value in an entry?
this does make me a little confused about the old API. how were users expected to obtain an entry's underlying value? going back to the document and looking up the entry's key? that seems important to me, but maybe I'm misunderstanding how people would use this API. (I know we inherited this BSON library, so maybe you don't know the answer there, I'm mainly just curious... and this subject is kind of relevant for the phone screens I'm doing these days...)
anyway, definitely support having an API consistent w/ the standard library's HashMap.
There was a problem hiding this comment.
Yep, OccupiedEntry will have a fn get() -> &Bson on it which will return the value.
In the old API (and the new one, for now), it looks like you had to use or_insert or or_insert_with to get the value out from an entry, inserting into the into it if it wasn't occupied. It was definitely incomplete and missing a lot of the useful parts of the std::HashMap one.
The Entry API looks mostly useful for mutating / inspecting the map itself without having to compute the hash of an input multiple times. I remember we were doing that in the driver and a user actually submitted a PR to update it to use entry and be a little more efficient.
let mut map = HashMap::new();
// old
if map.get("key").is_none() {
map.insert("key".to_string(), "value".to_string());
}
// new
if let Entry::Vacant(entry) = map.entry("key") {
entry.insert("value".to_string());
}There was a problem hiding this comment.
ahh I see. thanks for explanation! cool, seems like the new API should be a nice usability improvement then.
| chrono = "0.4" | ||
| rand = "0.7" | ||
| serde = { version = "1.0", features = ["derive"] } | ||
| serde_json = { version = "1.0", features = ["preserve_order"] } | ||
| linked-hash-map = "0.5.3" | ||
| indexmap = "1.6.2" |
There was a problem hiding this comment.
the README for the crate it notes that insertion order is not preserved if anything is deleted (though it's not clear to me what the behavior in that case is). I don't see us deleting from the index map anywhere, but is there a chance we'd need to do so in the future and might do so without realizing it would break things?
There was a problem hiding this comment.
Oh good catch. It looks like we want to use shift_remove if we want to preserve the order. It's a little bit slower, but this seems like the correct default. I updated remove to use it and made the test for it a little more robust.
| /// A view into a single entry in a map, which may either be vacant or occupied. | ||
| /// | ||
| /// This enum is constructed from the entry method on HashMap. | ||
| pub enum Entry<'a> { |
There was a problem hiding this comment.
from looking at the docs for std::HashMap's OccupiedEntry type, I assume that will cover a way to obtain the value in an entry?
this does make me a little confused about the old API. how were users expected to obtain an entry's underlying value? going back to the document and looking up the entry's key? that seems important to me, but maybe I'm misunderstanding how people would use this API. (I know we inherited this BSON library, so maybe you don't know the answer there, I'm mainly just curious... and this subject is kind of relevant for the phone screens I'm doing these days...)
anyway, definitely support having an API consistent w/ the standard library's HashMap.
| /// A view into a single entry in a map, which may either be vacant or occupied. | ||
| /// | ||
| /// This enum is constructed from the entry method on HashMap. | ||
| pub enum Entry<'a> { |
There was a problem hiding this comment.
ahh I see. thanks for explanation! cool, seems like the new API should be a nice usability improvement then.
RUST-283
This PR updates
Documentto wrap anIndexMap(from theindexmapcrate) instead ofLinkedHashMapfrom the now defunctlinked-hash-mapcrate.As part of that, I also updated the
EntryAPI to match that ofIndexMap/std::HashMap, since ours used to be slightly different.