Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a roaring64 iterator and functions for it #558

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

SLieve
Copy link
Contributor

@SLieve SLieve commented Jan 14, 2024

For issue #549: "The ability to do external iteration (an iterator type that the caller controls arbitrarily)".

I decided not to expose the iterator type, because it would need to pull in a lot of stuff from ART. I'm open to changing this though.

These mirror the 32-bit roaring iterator functions.
@SLieve SLieve marked this pull request as ready for review January 14, 2024 12:41
This is more consistent with the rest of the codebase.
These allow reusing an existing iterator.
@Dr-Emann Dr-Emann merged commit ab06f50 into RoaringBitmap:master Jan 19, 2024
25 checks passed
@SLieve SLieve deleted the roaring64_it branch January 22, 2024 21:07
Comment on lines +448 to +451
* Once this returns false, `roaring64_iterator_advance` should not be called on
* the iterator again. Calling `roaring64_iterator_previous` is allowed.
*/
bool roaring64_iterator_advance(roaring64_iterator_t *it);
Copy link
Member

@Dr-Emann Dr-Emann Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SLieve

This seems to imply (to me) that something like this should work

roaring64_bitmap_t *b = roaring64_bitmap_of(1, 100ULL);
roaring64_iterator_t *it = roaring64_iterator_create(b);
// it points at the first item, 100

roaring64_iterator_previous(it);
// it now points before the first item

roaring64_iterator_advance(it);
assert(roaring64_iterator_has_value(it));
assert(roaring64_iterator_value(it) == 100);

But instead it fails, right at the beginning of roaring64_iterator_advance:

if (it->art_it.value == NULL) {
    return (it->has_value = false);
}

In fact, because of this check, it seems like it should be safe to call roaring64_iterator_advance repeatedly even after reaching the end of the bitmap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'd consider not being able to go back when reaching the end incorrect behavior, we should fix that.

@Dr-Emann
Copy link
Member

@SLieve roaring_uint32_iterator_move_equalorlarger is able to move the iterator forward/backward, is the difference between that and roaring64_iterator_move_equalorlarger (which can only move forward) on purpose?

@SLieve
Copy link
Contributor Author

SLieve commented Jan 25, 2024

I hadn't realized that the 32-bit version was able to go in both directions. I did this to simplify the implementation, but for consistency we should make it go in both directions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants