-
-
Notifications
You must be signed in to change notification settings - Fork 688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve document docs #2359
improve document docs #2359
Conversation
//! - `BTreeMap<Field, Value>` a mapping of field_ids to their relevant schema value using a | ||
//! - `BTreeMap<Field, OwnedValue>` a mapping of field_ids to their relevant schema value using a | ||
//! BTreeMap. | ||
//! - `HashMap<Field, Value>` a mapping of field_ids to their relevant schema value using a HashMap. | ||
//! - `HashMap<Field, OwnedValue>` a mapping of field_ids to their relevant schema value using a | ||
//! HashMap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to explain what these bring to the table, i.e. how the three built-in options compare, e.g. BTreeMap
provides associative access but ordered where HashMap
has faster associative access, but neither supports multiple values per field if I understand this correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not sure why we have that actually. Seems too niche
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote the original doc originally IIRC, but the motive for the impl was because a lot of people have situations where they have single-value fields and just want to deserialize and serialize the data to and from JSON as is without needing to convert the by-default multi-value fields into single-value fields. So this was a utility for that.
One other question I had while trying to migrate my code base to this and which I think would be nice to clarify in the docs: Can I rely on If I cannot match field positionally between serialization and deserialization, I would need access to the relevant |
From the API contract there is no such guarantee, e.g. we may choose whatever is better compressible in the future. |
But then we need to extend the API with a seed as otherwise, I do not see how I can deserialize the compile-time fields of struct without knowing their runtime field ID. |
#2362 should provide what is required pass the runtime field ID to a deserialize implementation. |
No description provided.