search

sstadick / rust-lapper

Rust implementation of a fast, easy, interval tree library nim-lapper
https://docs.rs/rust-lapper
MIT License
57 stars 8 forks source link

Add support for insert(elem) #18

Closed zaporter closed 2 years ago

zaporter commented 2 years ago

The ability to insert into this structure can be very important if you have a read-heavy but right-necessary workload where rebuilding the whole data structure for every write is infeasible.

I have tried to implement this function so that it is easy to understand and maintain. It might not be as fast as it could be (that is outside my area of expertise).

Please let me know if you have intentionally avoided adding this function for whatever reason or if you think that I should be using a different data structure.

I also refactored some of the tests. I did this just in case you want to add other methods in the future, it seemed like the right thing to do. Let me know if you have any opinions on that.

Thank you for your cool library :heart:

sstadick commented 2 years ago

@zaporter - lookslike it's just failing the cargofmt lint. Could you run that on your branch and push to here? I think we should be in a good spot after that.

sstadick commented 2 years ago

I only never added an insert because I didn't have that use case and I knew it would be slooooow. But I have nothing against having it available with the appropriate caveats in place in the docs 👍

zaporter commented 2 years ago

@sstadick Thanks for the formatting advice! I forgot to run a format check before submitting this :facepalm: .

Let me know if you have any further comments, but I think I addressed everything.

Re: not implemented: Yeah, an insert is slow, but hopefully this will still be faster than other data structures for a read heavy workload. I need to do some benchmarking... If I get any interesting numbers I might submit them just so other people know.

Again, thanks for making this!

zaporter commented 2 years ago

I woke up this morning and realized that my tests weren't testing what they should be. Just rewrote the tests. This should be a no-brainer switch.

All of the tests pass and it guarantees that creating from ::new() or creating from ::insert() create the same Lapper.

Ready to merge after your approval.

sstadick commented 2 years ago

@zaporter This all looks great, thank you again! I'll make the release tomorrow 👍 and I haven't done it by tomorrow feel free to ping me.

sstadick commented 2 years ago

@zaporter released, see v1.1.0. Thanks again!