- Notifications
You must be signed in to change notification settings - Fork338
Add optional support for borsh serialisation#1335
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 don't know the opinion of the other maintainers, but I think we don't have much to lose since it's behind a feature gate.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Well, thereis additional maintenance burden as presumably people that rely on this will expect it to work going forward. If there's something I can do to try to proactively alleviate the concerns, let me know. An alternative might be to implement this directly in |
Behind a feature flag.
I addressed the couple of comments. |
Hum, right. I'm not the usual maintainer here and I hadn't thought about that :) The problem of ndarray in the last 2-3 years is the lack of maintainer. Because of this, I'm in no position to accept this MR. I'm not saying this will never get merged, but probably not by me. |
| // Deserialize the length. | ||
| let len = <usize as BorshDeserialize>::deserialize_reader(reader)?; | ||
| // Deserialize the given number of elements. We assume the source is | ||
| // trusted so we use a capacity hint... |
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.
Why trusted?
| <D as BorshSerialize>::serialize(&self.raw_dim(), writer)?; | ||
| // Followed by length of data | ||
| let iter = self.iter(); | ||
| <usize as BorshSerialize>::serialize(&iter.len(), writer)?; |
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.
Why both length and dimensions?
| I: BorshDeserialize, | ||
| { | ||
| fn deserialize_reader<R: borsh::io::Read>(reader: &mut R) -> borsh::io::Result<Self> { | ||
| <I as BorshDeserialize>::deserialize_reader(reader).map(Dim::new) |
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 would be worried that we have to break compatibility here if we change how dimensions are stored?
| // Dimensions | ||
| let dim = <D as BorshDeserialize>::deserialize_reader(reader)?; | ||
| // Followed by length of data | ||
| let len = <usize as BorshDeserialize>::deserialize_reader(reader)?; |
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.
How do you handle integer size here? What if it was serialized on a 64-bit usize platform but you deserialize on a 32-bit?
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 would look atindexmap-rs/indexmap#313 for inspiration - fixed size integers and check error cases
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.
borsh always serialises usize as u64:https://docs.rs/borsh/latest/src/borsh/ser/mod.rs.html#128-132
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.
borsh says that container sizes should be stored as u32. Unsure how it interacts with ndarray. For me, developing this PR is not my priority unfortunately. Help from others welcome.
Behind a feature flag.