CarlKCarlK / range-set-blaze

Integer sets as fast, sorted, integer ranges with full set operations
Apache License 2.0
76 stars 10 forks source link

Add a into_inner and from_inner to would make serialization easier. #17

Open CarlKCarlK opened 8 months ago

CarlKCarlK commented 8 months ago

As per @bonsairobo's suggestion on issue #15

Add a into_inner and from_inner method that would return the underlying BTreeMap and construct from it.

They would not return or require the running length. Instead, the from_inner would validate the BTreeMap and compute the length (O(#of ranges)).

One complication: When the map RangeMapBlaze is ready, RangeSetBlaze may be re-defined as RangeMapBlaze<T,()) with the unit type as the value. This will change the type of the inner BTreeMap from BTreeMap<T,T> to something like BTreeMap<T,(T,())>, which means old, serialized data may be incompatible.

bonsairobo commented 8 months ago

I suppose it's still worth it to make RangeSetBlaze implement serde traits so that you don't need to do any validation on serialization. Although it might be possible for someone to serialize something that looks like a RangeSetBlaze but isn't actually a valid one (depends on the serializer used). Maybe let the user decide whether to do validation.

CarlKCarlK commented 8 months ago

My thought is that because serialization is also O(#ranges) the validation cost would not be noticed (also, it is just comparing a couple of integers per range). So, the conservative thing to do for now is to validate from_inner. If/when a need is shown, Serde support and/or an (unsafe?) constructor could be added.

bonsairobo commented 8 months ago

Makes sense. FWIW serde support is pretty easy to add. And I don't think an unvalidated constructor warrants unsafe unless there is other unsafe in this crate using the disjoint ranges assumption, which I don't think there is.

CarlKCarlK commented 8 months ago

Good! I can likely make a branch with this feature today if you might find it useful. Let me know.

[Good to hear Serde isn't bad. I've not used it but have done serializers in other languages. I think at a minimum Serde support would require a new Cargo feature.]

bonsairobo commented 8 months ago

Good! I can likely make a branch with this feature today if you might find it useful.

I'm satisfied for now :). We'll see how it goes as I continue using this crate in one of my data structure projects. I'm glad to replace a big chunk of code with something thoroughly tested.

Good to hear Serde isn't bad. I've not used it but have done serializers in other languages. I think at a minimum Serde support would require a new Cargo feature.

Yea it's usually as simple as adding some derive macros within a conditional compilation attribute:

#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
struct Foo { ... }

and then adding this to your Cargo.toml:

serde = { version "1.0.x", optional = true }
bonsairobo commented 8 months ago

Although sometimes generics require special treatment.