Add optional support for borsh serialisation - #1335
Conversation
Sorry, something went wrong.
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.
Sorry, something went wrong.
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 borsh and push the burden in there but it gets much more convoluted this way due to versions and such. It might end up this way for chrono at least though: chronotope/chrono#1366 |
Sorry, something went wrong.
Behind a feature flag.
|
I addressed the couple of comments. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
| // 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.
Why trusted?
Sorry, something went wrong.
| <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.
Why both length and dimensions?
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| // 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?
Sorry, something went wrong.
There was a problem hiding this comment.
I would look at indexmap-rs/indexmap#313 for inspiration - fixed size integers and check error cases
Sorry, something went wrong.
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
Sorry, something went wrong.
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.
Sorry, something went wrong.
Behind a feature flag.