Implement SkipTo for iter::Iter #287

grelner · 2024-08-06T12:51:39Z

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.

Dr-Emann · 2024-08-07T15:10:28Z

src/bitmap/iter.rs

+    fn skip_to(self, n: u32) -> Self::Iter {
+        SkipToIter::new(&self.containers, n)
+    }
+}


Note: using the full unmodified self.containers means this might skip backwards.

To do this optimally, we might need to somewhat re-implement std::iter::Flatten so we can access the slice::Iter directly (from which we can get the remaining slice), and access to the current container iterator. :/

Really? self.containers is just a reference to RoaringBitmap.containers. It was added in this PR
edit: ah, self.containers is RoaringBitmap.containers here. Then there's something I don't understand here if they can be backwards 😅

I thought about changing iter::Iter to only contain a reference to containers, an index, and the iterator for the current container, and do the flatten part in ::next(), which I guess is what you're saying. This would also allow the SkipTo iterator to go backwards.
Still, are all these changes worth it to have a double ended iterator and cover the kinda weird case of skip_to to a point the iterator has already passed?

Hey @grelner 👋

Sorry for the late reply, I'm in vacation 🌞

To do this optimally, we might need to somewhat re-implement std::iter::Flatten so we can access the slice::Iter directly (from which we can get the remaining slice), and access to the current container iterator.

I agree with what @Dr-Emann explains here. This is, to me, the best way to implement this iterator. Flatten is not hard to implement. We can have a special Flatten type with all the original Flatten type logic but dedicated to slices and bitmaps so that it can skip groups according to the split high group (skipping whole slices or bitmaps if the number we want to find is not in this group).

I thought about changing iter::Iter to only contain a reference to containers, an index, and the iterator for the current container, and do the flatten part in ::next(), which I guess is what you're saying. This would also allow the SkipTo iterator to go backwards.

Let's forget about the SkipTo wrapper and only implement a non-lazy method like Iterator::advance_by method that directly advances the iterator to the requested number. This method, skip_to, is only available on the bitmap::Iter/IntoIter types, so there is no need to think about std::iter::Rev iterator adaptator.

grelner · 2024-08-22T05:03:39Z

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:

advance_to for Iter and IntoIter, which skips containers using the value key. This uses an existing iterator for data
Bitmap::iter_from, which is faster that advance_to, because it does a binary search to find the container to start from
This change significantly changes the iterator implementations, which now implement their own flatten functionality.

grelner · 2024-08-22T09:01:44Z

@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 · 2024-08-22T11:20:53Z

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

grelner · 2024-09-23T19:23:53Z

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

Kerollmops

Thank you for your work @grelner. That's very good, actually.

I think I am not a huge fan of having a new AdvanceToIter wrapper. Can you tell me why the Iter/IntoIter/advance_to method cannot just advance the iterator with a loop like you did? It will advance container by container until it reaches the right one.

roaring/src/bitmap/iter.rs

roaring/tests/iter_advance_to.rs

roaring/src/bitmap/store/mod.rs

roaring/src/bitmap/store/array_store/mod.rs

roaring/tests/iter_advance_to.rs

Dr-Emann · 2024-10-01T23:06:21Z

roaring/src/bitmap/iter.rs

+        fn advance_to_inner(&mut self, n: u32) {
+            let (key, _) = util::split(n);
+            fn advance_iter(iter: &mut container::Iter<'_>, n: u32) -> Option<u32> {
+                while let Some(next) = iter.peek() {
+                    if next < n {
+                        iter.next();
+                    } else {
+                        return Some(next);
+                    }
+                }
+                None
+            }
+            if let Some(index) = self.find_container(key) {
+                self.drain_containers_until(index);
+                let container = self.pop_container_front().expect("bug!");
+                let mut iter = container.into_iter();
+                advance_iter(&mut iter, n);
+                *self.iter_front_mut() = iter;
+            } else {
+                // there are no containers with given key. Look in iter_front and iter_back.
+                let iter_front = self.iter_front_mut();
+                if iter_front.peek().map(|n| util::split(n).0) == Some(key) {
+                    advance_iter(iter_front, n);
+                    return;
+                }
+                if let Some(iter_back) = self.iter_back_mut() {
+                    if iter_back.peek().map(|n| util::split(n).0) == Some(key) {
+                        advance_iter(iter_back, n);
+                        *self.iter_front_mut() = Self::empty_inner_iter();
+                        return;
+                    }
+                }
+                self.clear_containers();
+                *self.iter_front_mut() = Self::empty_inner_iter();
+                *self.iter_back_mut() = None;
+            }
+        }


I'm pretty sure this isn't updating the size_hint for the iterator.

I really think trying to keep track of the size_hint for these iterators was a mistake, and would rather just remove the exact size_hint tracking.

You're completely right of course. I've fixed this in the latest commit. But I agree, maybe it would be better to remove this code. It certainly would make it cleaner.

grelner · 2024-10-10T12:08:07Z

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

Kerollmops · 2024-10-13T09:43:09Z

Hey @grelner 👋
It's may be due to #293, would you mind rebasing on main? 🤔

Kerollmops · 2024-10-31T10:27:14Z

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 🎃

Dr-Emann

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)

Dr-Emann · 2024-11-01T18:06:52Z

roaring/src/bitmap/store/mod.rs

@@ -497,26 +510,46 @@ impl PartialEq for Store {
    }
 }

-impl<'a> Iterator for Iter<'a> {
+impl Iter<'_> {
+    pub fn peek(&mut self) -> Option<u16> {


This can take &self

Dr-Emann · 2024-11-01T18:13:32Z

roaring/src/bitmap/iter.rs

+            fn advance_iter(iter: &mut container::Iter<'_>, n: u32) -> u64 {
+                let mut items_skipped = 0;
+                while let Some(next) = iter.peek() {
+                    if next < n {
+                        iter.next();
+                        items_skipped += 1;
+                    } else {
+                        return items_skipped;
+                    }
+                }
+                items_skipped
+            }


Both array and bitmap container iterators should be able to do this more efficiently, we should be able to implement an store::Iter::advance_to(&mut self, n: u16) function that can do better than O(n):

For an array container, we can binary search for n, and use Iterator::nth to efficiently skip ahead to just before the element. For a bitmap container, we should be able to just directly advance key, and mask the value (with some edge cases where key is unchanged and we need to mask the current value, or key becomes equal to key_back, and we need to steal value_back).

Dr-Emann · 2024-11-01T18:21:10Z

roaring/src/bitmap/iter.rs

+        fn pop_container_front(&mut self) -> Option<Self::Container>;
+        fn pop_container_back(&mut self) -> Option<Self::Container>;
+
+        fn drain_containers_until(&mut self, index: usize);
+
+        fn clear_containers(&mut self);
+
+        fn find_container(&self, key: u16) -> Option<usize>;
+
+        fn iter_front_mut(&mut self) -> &mut container::Iter<'a>;
+        fn iter_back_mut(&mut self) -> &mut Option<container::Iter<'a>>;


We should be able to avoid these, if we use slice::Iter<'a, Container> and vec::IntoIter<Container>, rather than &[Container] and VecDeque<Container>. Both implement AsRef<[Container]>, Iterator (for next + nth), and Iterator::Item: IntoIterator<IntoIter=container::Iter>, so it should be possible to write a function something like:

fn advance_to_impl<'a, It>( n: u32, front_iter: &mut Option<container::Iter<'a>>, containers: &mut It, back_iter: &mut Option<container::Iter<'a>>, ) where It: Iterator, It: AsRef<[Container]>, It::Item: IntoIterator<IntoIter = container::Iter<'a>>, { let (key, index) = util::split(n); // Compare the key of n to the front iter's key {LT => Do nothing, EQ => advance front to index, GT => consume front iter, continue} // Binary search for the key in containers, if found, front_iter = containers.nth(idx) and advance it, else use _ = nth(idx - 1) to consume up to where it would be. // If containers is now empty, check the back iter's key, and possibly advance it or consume it }

Which has all the logic that could be called by both advance_to implementations.

Dr-Emann · 2024-11-01T18:46:29Z

roaring/src/bitmap/store/mod.rs

    Array(slice::Iter<'a, u16>),
    Vec(vec::IntoIter<u16>),
    BitmapBorrowed(BitmapIter<&'a [u64; BITMAP_LENGTH]>),
    BitmapOwned(BitmapIter<Box<[u64; BITMAP_LENGTH]>>),
 }

+pub struct Iter<'a> {
+    inner: IterInner<'a>,
+    size_hint: u64,


Sorry you went to all the trouble of adding tracking the size hint, but with #295, this can all go back away now 😢

grelner · 2024-11-01T18:55:05Z

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

grelner added 3 commits August 6, 2024 14:23

Implement SkipTo for iter::Iter

6d11209

Fix panic when calling skip_to on iterator from empty bitmap

df0b3cd

Make it compile on rust 1.66

c7f02c6

Dr-Emann reviewed Aug 7, 2024

View reviewed changes

grelner mentioned this pull request Aug 9, 2024

Iterate from minimum value #286

Closed

grelner added 2 commits August 13, 2024 12:26

Fix clippy warnings

e8c57e0

Implement advance_to and iter_from

bb2a249

grelner marked this pull request as ready for review August 22, 2024 04:59

grelner added 3 commits August 22, 2024 07:17

Merge in workspace pr

025d489

Fix advance_to when coming from an iterator with an iter_back

d25a8fe

Make it compile on 1.66

d3f9cdb

Kerollmops requested changes Sep 24, 2024

View reviewed changes

grelner added 3 commits September 25, 2024 16:56

Remove Empty variant in bitmap::store::Iter

2699703

Revert change in imports

65b5330

Refactor iterators

9a325bc

grelner requested a review from Kerollmops September 25, 2024 15:10

Fix performance regression in iterators

feca297

Dr-Emann mentioned this pull request Sep 29, 2024

Add range-based iteration #290

Closed

Dr-Emann reviewed Oct 1, 2024

View reviewed changes

grelner added 4 commits October 2, 2024 08:57

Remove dangling comment

de80748

Simplify store::Iter::peek

32a3695

Fix size_hint() when using advance_to

522e945

Fix nightly clippy errors

3c86fd9

Dr-Emann mentioned this pull request Oct 16, 2024

Lazily compute Iterator len #295

Merged

Dr-Emann reviewed Nov 1, 2024

View reviewed changes

Dr-Emann mentioned this pull request Nov 2, 2024

feat: implement advance_to and advance_back_to for Iter #296

Merged

Kerollmops closed this in #296 Nov 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SkipTo for iter::Iter #287

Implement SkipTo for iter::Iter #287

grelner commented Aug 6, 2024 •

edited

Loading

Dr-Emann Aug 7, 2024

grelner Aug 7, 2024 •

edited

Loading

Kerollmops Aug 13, 2024 •

edited

Loading

grelner commented Aug 22, 2024

grelner commented Aug 22, 2024

grelner commented Aug 22, 2024

grelner commented Sep 23, 2024

Kerollmops left a comment

Dr-Emann Oct 1, 2024

grelner Oct 2, 2024

grelner commented Oct 10, 2024

Kerollmops commented Oct 13, 2024

Kerollmops commented Oct 31, 2024

Dr-Emann left a comment

Dr-Emann Nov 1, 2024

Dr-Emann Nov 1, 2024

Dr-Emann Nov 1, 2024

Dr-Emann Nov 1, 2024

grelner commented Nov 1, 2024

Implement SkipTo for iter::Iter #287

Implement SkipTo for iter::Iter #287

Conversation

grelner commented Aug 6, 2024 • edited Loading

Dr-Emann Aug 7, 2024

Choose a reason for hiding this comment

grelner Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Kerollmops Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

grelner commented Aug 22, 2024

grelner commented Aug 22, 2024

grelner commented Aug 22, 2024

grelner commented Sep 23, 2024

Kerollmops left a comment

Choose a reason for hiding this comment

Dr-Emann Oct 1, 2024

Choose a reason for hiding this comment

grelner Oct 2, 2024

Choose a reason for hiding this comment

grelner commented Oct 10, 2024

Kerollmops commented Oct 13, 2024

Kerollmops commented Oct 31, 2024

Dr-Emann left a comment

Choose a reason for hiding this comment

Dr-Emann Nov 1, 2024

Choose a reason for hiding this comment

Dr-Emann Nov 1, 2024

Choose a reason for hiding this comment

Dr-Emann Nov 1, 2024

Choose a reason for hiding this comment

Dr-Emann Nov 1, 2024

Choose a reason for hiding this comment

grelner commented Nov 1, 2024

grelner commented Aug 6, 2024 •

edited

Loading

grelner Aug 7, 2024 •

edited

Loading

Kerollmops Aug 13, 2024 •

edited

Loading