Skip to content

Commit 9f671ed

Browse files
authored
fix roaring64 iterator state after reading past end (#731)
Set saturated_forward flag when iterator reaches end in `roaring64_iterator_read` to ensure iterator can move backwards. Add test coverage to validate.
1 parent afdce15 commit 9f671ed

File tree

4 files changed

+32
-3
lines changed

4 files changed

+32
-3
lines changed

microbenchmarks/synthetic_bench.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1+
#include <array>
12
#include <benchmark/benchmark.h>
23
#include <random>
34
#include <set>
4-
#include <array>
55

66
#include "performancecounters/event_counter.h"
77
#include "roaring/roaring64.h"

src/roaring.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1596,7 +1596,7 @@ roaring_bitmap_t *roaring_bitmap_deserialize_safe(const void *buf,
15961596
for (uint32_t i = 0; i < card; i++) {
15971597
// elems may not be aligned, read with memcpy
15981598
uint32_t elem;
1599-
memcpy((char*)&elem, (char*)(elems + i), sizeof(elem));
1599+
memcpy((char *)&elem, (char *)(elems + i), sizeof(elem));
16001600
roaring_bitmap_add_bulk(bitmap, &context, elem);
16011601
}
16021602
return bitmap;
@@ -3016,7 +3016,7 @@ void roaring_bitmap_frozen_serialize(const roaring_bitmap_t *rb, char *buf) {
30163016
uint16_t *key_zone = (uint16_t *)arena_alloc(&buf, 2 * ra->size);
30173017
uint16_t *count_zone = (uint16_t *)arena_alloc(&buf, 2 * ra->size);
30183018
uint8_t *typecode_zone = (uint8_t *)arena_alloc(&buf, ra->size);
3019-
char *header_zone = (char*)arena_alloc(&buf, 4);
3019+
char *header_zone = (char *)arena_alloc(&buf, 4);
30203020

30213021
for (int32_t i = 0; i < ra->size; i++) {
30223022
uint16_t count;

src/roaring64.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2707,6 +2707,8 @@ uint64_t roaring64_iterator_read(roaring64_iterator_t *it, uint64_t *buf,
27072707
it->has_value = art_iterator_next(&it->art_it);
27082708
if (it->has_value) {
27092709
roaring64_iterator_init_at_leaf_first(it);
2710+
} else {
2711+
it->saturated_forward = true;
27102712
}
27112713
}
27122714
return consumed;

tests/roaring64_unit.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1974,6 +1974,32 @@ DEFINE_TEST(test_stats) {
19741974
roaring64_bitmap_free(r1);
19751975
}
19761976

1977+
DEFINE_TEST(test_iterator_read_past_end_can_go_previous) {
1978+
roaring64_bitmap_t* bitmap = roaring64_bitmap_create();
1979+
assert_non_null(bitmap);
1980+
1981+
roaring64_bitmap_add(bitmap, 10);
1982+
assert_r64_valid(bitmap);
1983+
1984+
roaring64_iterator_t* iter = roaring64_iterator_create(bitmap);
1985+
assert_non_null(iter);
1986+
1987+
uint64_t buffer[100];
1988+
uint64_t actual_read1 = roaring64_iterator_read(
1989+
iter, buffer, sizeof(buffer) / sizeof(buffer[0]));
1990+
assert_int_equal(actual_read1, 1); // Only one value should be present
1991+
1992+
// Should now be one past the end, but should be able to move backwards
1993+
assert_false(roaring64_iterator_has_value(iter));
1994+
bool prev_result = roaring64_iterator_previous(iter);
1995+
assert_true(prev_result);
1996+
assert_true(roaring64_iterator_has_value(iter));
1997+
assert_int_equal(roaring64_iterator_value(iter), 10);
1998+
1999+
roaring64_iterator_free(iter);
2000+
roaring64_bitmap_free(bitmap);
2001+
}
2002+
19772003
} // namespace
19782004

19792005
int main() {
@@ -2038,6 +2064,7 @@ int main() {
20382064
cmocka_unit_test(test_iterator_move_equalorlarger),
20392065
cmocka_unit_test(test_iterator_read),
20402066
cmocka_unit_test(test_stats),
2067+
cmocka_unit_test(test_iterator_read_past_end_can_go_previous),
20412068
};
20422069
return cmocka_run_group_tests(tests, NULL, NULL);
20432070
}

0 commit comments

Comments
 (0)