search

RoaringBitmap / croaring-rs

Rust FFI wrapper for CRoaring
Apache License 2.0
157 stars 43 forks source link

Support native serialization format #106

Closed andreas closed 1 year ago

andreas commented 1 year ago

This PR adds support for the native serialization format from CRoaring. It builds on #105 to get support for roaring_bitmap_deserialize_safe. I'm admittedly an absolute newcomer to Rust, so I'd appreciate feedback on how to do this best. I tried a few approaches and found this struck the best balance in terms of aligning with the serialization approach used for TreeMap, not breaking backwards compatibility, and avoiding a proliferation of (de-)serialization functions on Bitmap. If this approach is acceptable, I'm happy to add tests.

andreas commented 1 year ago

I'll rebase on master when #107 is merged.

saulius commented 1 year ago

Sorry @andreas, really struggling to find proper time to review the code, but I'm here :)

Can you please rebase on #107?

andreas commented 1 year ago

Thanks for the update, @saulius!

I've rebased on master now. Tests are still missing, but I added/updated docs. I'm happy to add tests if the general approach is considered right 🙂

saulius commented 1 year ago

Thanks for a great change @andreas , I agree with @Dr-Emann that this looks like a much Rust-ier interface to serialization. Addition of tests for Native would be welcomed, of course.

andreas commented 1 year ago

Alright, I've fixed tests and added rudimentary tests for the native format, so I think the changes are ready to be reviewed.

saulius commented 1 year ago

Thanks, @andreas, this looks great!