RoaringBitmap / roaring-rs

A better compressed bitset in Rust
https://docs.rs/roaring/
Apache License 2.0
757 stars 84 forks source link

Implement SkipTo for iter::Iter #287

Open grelner opened 3 months ago

grelner commented 3 months ago

This is the start of the implementation of #286 . It adds a new trait SkipTo which for now is only implemented for iter::Iter.

The SkipTo trait contains a method skip_to(&mut self, n:u32) which should return an Iterator that yields values >= n. The implementation for iter::Iter achieves this by doing a binary search for the correct container, and then another binary search if the container is an ArrayStore, or forwarding like a normal BitmapIter until a suitable value is found for BitmapStore.

Right now SkipTo is implemented for iter::Iter, but I don't think this is semantically sound, and it should probably rather be implemented for RoaringBitmap. In the discussion for #286 @Kerollmops wanted a similar api to SkipWhile, but this does not really fit, as SkipWhile can perform it's operation only using an existing iterator, but this is not true for SkipTo, as it needs direct access to the containers to do it's skip efficiently, not only the existing iterator. A reference to the container vec has been added to iter::Iter only for this purpose. The iterator returned is fully independent, and not related to the iterator on which the skip_to call was made at all.

This PR in it's current state implements what I need for my project, but I would like to finish it, albeit at a slightly slower pace.

grelner commented 2 months ago

Hi sorry for the slow progress, I've been on vacation too. I've pushed some changes and I think it's ready for review now. This implements:

grelner commented 2 months ago

@Kerollmops sorry this 1.66 requirement keeps tripping me up. I pushed a fix for it now. Maybe that should be added to https://github.com/RoaringBitmap/roaring-rs?tab=readme-ov-file#developing

grelner commented 2 months ago

bench-skip_to.txt cargo bench iteration results of the branch

grelner commented 1 month ago

@Kerollmops any chance to get this reviewed and hopefully merged?

grelner commented 3 weeks ago

@Kerollmops did you change something in CI? It's failing on parts of the code I never touched

Kerollmops commented 3 weeks ago

Hey @grelner šŸ‘‹ It's may be due to https://github.com/RoaringBitmap/roaring-rs/pull/293, would you mind rebasing on main? šŸ¤”

Kerollmops commented 5 days ago

Hey @grelner šŸ‘‹

It seems the CI is complaining about missing documentation on some macros, would mind adding the doc in a dedicated commit and rebase on main now that I merged #295, please?

Have a nice day šŸŽƒ

grelner commented 4 days ago

Similar to #290, I think I have a good idea of basically exactly what I'd want this implementation to look like, let me know if you'd rather I finish this off. I know there's been a lot of back and forth on this already (add the size hint, now remove the size hint)

Hi @Dr-Emann , thanks for the feedback! I'm hilariously busy at the moment and unfortunately won't have time to work on this for at least 2-3 weeks so feel free to finish up. If not I'm happy to do it when I have some time again