Add optional support for borsh serialisation#1335
Add optional support for borsh serialisation#1335Fuuzetsu wants to merge 1 commit intorust-ndarray:masterfrom
Conversation
nilgoyette
left a comment
nilgoyette
left a comment
There was a problem hiding this comment.
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.
Well, there is 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... |
| <D as BorshSerialize>::serialize(&self.raw_dim(), writer)?; | ||
| // Followed by length of data | ||
| let iter = self.iter(); | ||
| <usize as BorshSerialize>::serialize(&iter.len(), writer)?; |
| 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.
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.
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.
I would look at indexmap-rs/indexmap#313 for inspiration - fixed size integers and check error cases
There was a problem hiding this comment.
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.
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.
3 participants
Behind a feature flag.