From a5507334240cec2dab5ab467105d071f728d4c96 Mon Sep 17 00:00:00 2001 From: Soerian Lieve Date: Thu, 1 Feb 2024 17:42:41 +0000 Subject: [PATCH] r64 iterator: allow moving from beyond the start / end (#578) Previously calling `roaring64_iterator_previous` after `roaring64_iterator_advance` returned false would not move the iterator. With this commit, this behavior and its inverse are both possible. I chose to implement this by adding a new field in the iterator to indicate saturation. Another option would be to allow the ART iterator to allow moving from beyond the start / end, but that would require changing the index to a signed integer and thus take up more space. --- src/roaring64.c | 37 +++++++++++++++++++++++++++++-------- tests/roaring64_unit.cpp | 28 +++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/src/roaring64.c b/src/roaring64.c index f5f7fb76b..ca61423a9 100644 --- a/src/roaring64.c +++ b/src/roaring64.c @@ -48,6 +48,12 @@ typedef struct roaring64_iterator_s { uint64_t value; bool has_value; + + // If has_value is false, then the iterator is saturated. This field + // indicates the direction of saturation. If true, there are no more values + // in the forward direction. If false, there are no more values in the + // backward direction. + bool saturated_forward; } roaring64_iterator_t; // Splits the given uint64 key into high 48 bit and low 16 bit components. @@ -126,6 +132,8 @@ static inline roaring64_iterator_t *roaring64_iterator_init_at( } else { roaring64_iterator_init_at_leaf_last(it); } + } else { + it->saturated_forward = first; } return it; } @@ -1955,7 +1963,11 @@ uint64_t roaring64_iterator_value(const roaring64_iterator_t *it) { bool roaring64_iterator_advance(roaring64_iterator_t *it) { if (it->art_it.value == NULL) { - return (it->has_value = false); + if (it->saturated_forward) { + return (it->has_value = false); + } + roaring64_iterator_init_at(it->parent, it, /*first=*/true); + return it->has_value; } leaf_t *leaf = (leaf_t *)it->art_it.value; uint16_t low16 = (uint16_t)it->value; @@ -1964,15 +1976,21 @@ bool roaring64_iterator_advance(roaring64_iterator_t *it) { it->value = it->high48 | low16; return (it->has_value = true); } - if (!art_iterator_next(&it->art_it)) { - return (it->has_value = false); + if (art_iterator_next(&it->art_it)) { + return roaring64_iterator_init_at_leaf_first(it); } - return roaring64_iterator_init_at_leaf_first(it); + it->saturated_forward = true; + return (it->has_value = false); } bool roaring64_iterator_previous(roaring64_iterator_t *it) { if (it->art_it.value == NULL) { - return (it->has_value = false); + if (!it->saturated_forward) { + // Saturated backward. + return (it->has_value = false); + } + roaring64_iterator_init_at(it->parent, it, /*first=*/false); + return it->has_value; } leaf_t *leaf = (leaf_t *)it->art_it.value; uint16_t low16 = (uint16_t)it->value; @@ -1981,10 +1999,11 @@ bool roaring64_iterator_previous(roaring64_iterator_t *it) { it->value = it->high48 | low16; return (it->has_value = true); } - if (!art_iterator_prev(&it->art_it)) { - return (it->has_value = false); + if (art_iterator_prev(&it->art_it)) { + return roaring64_iterator_init_at_leaf_last(it); } - return roaring64_iterator_init_at_leaf_last(it); + it->saturated_forward = false; // Saturated backward. + return (it->has_value = false); } bool roaring64_iterator_move_equalorlarger(roaring64_iterator_t *it, @@ -2000,6 +2019,7 @@ bool roaring64_iterator_move_equalorlarger(roaring64_iterator_t *it, // move to a leaf with a key equal or greater. if (!art_iterator_lower_bound(&it->art_it, val_high48)) { // Only smaller keys found. + it->saturated_forward = true; return (it->has_value = false); } it->high48 = combine_key(it->art_it.key, 0); @@ -2019,6 +2039,7 @@ bool roaring64_iterator_move_equalorlarger(roaring64_iterator_t *it, } // Only smaller entries in this container, move to the next. if (!art_iterator_next(&it->art_it)) { + it->saturated_forward = true; return (it->has_value = false); } } diff --git a/tests/roaring64_unit.cpp b/tests/roaring64_unit.cpp index f3ff68977..cbea74653 100644 --- a/tests/roaring64_unit.cpp +++ b/tests/roaring64_unit.cpp @@ -1388,6 +1388,15 @@ DEFINE_TEST(test_iterator_advance) { i++; } while (roaring64_iterator_advance(it)); assert_int_equal(i, values.size()); + + // Check that we can move backward from after the last entry. + assert_true(roaring64_iterator_previous(it)); + i--; + assert_int_equal(roaring64_iterator_value(it), values[i]); + + // Check that we can't move forward again. + assert_false(roaring64_iterator_advance(it)); + roaring64_iterator_free(it); roaring64_bitmap_free(r); } @@ -1402,13 +1411,22 @@ DEFINE_TEST(test_iterator_previous) { values.push_back(v); roaring64_bitmap_add_bulk(r, &context, v); } - size_t i = values.size(); + int i = ((int)values.size()) - 1; roaring64_iterator_t* it = roaring64_iterator_create_last(r); do { - i--; assert_int_equal(roaring64_iterator_value(it), values[i]); + i--; } while (roaring64_iterator_previous(it)); - assert_int_equal(i, 0); + assert_int_equal(i, -1); + + // Check that we can move forward from before the first entry. + assert_true(roaring64_iterator_advance(it)); + i++; + assert_int_equal(roaring64_iterator_value(it), values[i]); + + // Check that we can't move backward again. + assert_false(roaring64_iterator_previous(it)); + roaring64_iterator_free(it); roaring64_bitmap_free(r); } @@ -1448,6 +1466,10 @@ DEFINE_TEST(test_iterator_move_equalorlarger) { assert_false(roaring64_iterator_move_equalorlarger(it, (1ULL << 36) + 1)); assert_false(roaring64_iterator_has_value(it)); + // Check that we can move backward from after the last entry. + assert_true(roaring64_iterator_previous(it)); + assert_int_equal(roaring64_iterator_value(it), (1ULL << 36)); + roaring64_iterator_free(it); roaring64_bitmap_free(r); }