Fix previousValue not returning correct results in some cases (#736)

* Step down container index if next present container key is larger than target container key This means that previous value (if exist) must be always in the previous container * Return value as unsigned long when previous value is also the last (largest) value in bitmap * Remove unnecessary condition
RoaringBitmap · Jul 8, 2024 · 9a862d5 · 9a862d5
1 parent 08b1e41
commit 9a862d5
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 5 deletions.
diff --git a/RoaringBitmap/src/main/java/org/roaringbitmap/RoaringBitmap.java b/RoaringBitmap/src/main/java/org/roaringbitmap/RoaringBitmap.java
@@ -2861,10 +2861,11 @@ public long previousValue(int fromValue) {
     char key = Util.highbits(fromValue);
     int containerIndex = highLowContainer.advanceUntil(key, -1);
     if (containerIndex == highLowContainer.size()) {
-      return last();
+      return Util.toUnsignedLong(last());
     }
     if (highLowContainer.getKeyAtIndex(containerIndex) > key) {
-      return -1L;
+      // target absent, key of first container after target too high
+      --containerIndex;
     }
     long prevSetBit = -1L;
     while (containerIndex != -1 && prevSetBit == -1L) {

diff --git a/RoaringBitmap/src/main/java/org/roaringbitmap/buffer/ImmutableRoaringBitmap.java b/RoaringBitmap/src/main/java/org/roaringbitmap/buffer/ImmutableRoaringBitmap.java
@@ -1820,13 +1820,14 @@ public long previousValue(int fromValue) {
     char key = highbits(fromValue);
     int containerIndex = highLowContainer.advanceUntil(key, -1);
     if (containerIndex == highLowContainer.size()) {
-      return last();
+      return Util.toUnsignedLong(last());
     }
     if (highLowContainer.getKeyAtIndex(containerIndex) > key) {
-      return -1L;
+      // target absent, key of first container after target too high
+      --containerIndex;
     }
     long prevSetBit = -1L;
-    while (containerIndex != -1 && containerIndex < highLowContainer.size() && prevSetBit == -1L) {
+    while (containerIndex != -1 && prevSetBit == -1L) {
       char containerKey = highLowContainer.getKeyAtIndex(containerIndex);
       MappeableContainer container = highLowContainer.getContainerAtIndex(containerIndex);
       int bit = (containerKey < key

diff --git a/RoaringBitmap/src/test/java/org/roaringbitmap/TestRoaringBitmap.java b/RoaringBitmap/src/test/java/org/roaringbitmap/TestRoaringBitmap.java
@@ -5360,6 +5360,23 @@ public void testPreviousValueRegression() {
         assertEquals(-1, RoaringBitmap.bitmapOf().previousValue(403042));
     }
 
+    @Test
+    public void testPreviousValue_AbsentTargetContainer() {
+        RoaringBitmap bitmap = RoaringBitmap.bitmapOf(-1, 2, 3, 131072);
+        assertEquals(3, bitmap.previousValue(65536));
+        assertEquals(131072, bitmap.previousValue(Integer.MAX_VALUE));
+        assertEquals(131072, bitmap.previousValue(-131072));
+
+        bitmap = RoaringBitmap.bitmapOf(131072);
+        assertEquals(-1, bitmap.previousValue(65536));
+    }
+
+    @Test
+    public void testPreviousValue_LastReturnedAsUnsignedLong() {
+        RoaringBitmap bitmap = RoaringBitmap.bitmapOf(-650002, -650001, -650000);
+        assertEquals(Util.toUnsignedLong(-650000), bitmap.previousValue(-1));
+    }
+
     @Test
     public void testRangeCardinalityAtBoundary() {
         // See https://github.com/RoaringBitmap/RoaringBitmap/issues/285

diff --git a/RoaringBitmap/src/test/java/org/roaringbitmap/buffer/TestImmutableRoaringBitmap.java b/RoaringBitmap/src/test/java/org/roaringbitmap/buffer/TestImmutableRoaringBitmap.java
@@ -1497,6 +1497,23 @@ public void testPreviousValue() {
     }
   }
 
+  @Test
+  public void testPreviousValue_AbsentTargetContainer() {
+    MutableRoaringBitmap bitmap = MutableRoaringBitmap.bitmapOf(-1, 2, 3, 131072);
+    assertEquals(3, bitmap.previousValue(65536));
+    assertEquals(131072, bitmap.previousValue(Integer.MAX_VALUE));
+    assertEquals(131072, bitmap.previousValue(-131072));
+
+    bitmap = MutableRoaringBitmap.bitmapOf(131072);
+    assertEquals(-1, bitmap.previousValue(65536));
+  }
+
+  @Test
+  public void testPreviousValue_LastReturnedAsUnsignedLong() {
+    MutableRoaringBitmap bitmap = MutableRoaringBitmap.bitmapOf(-650002, -650001, -650000);
+    assertEquals(Util.toUnsignedLong(-650000), bitmap.previousValue(-1));
+  }
+
   @Test
   public void testRangeCardinalityAtBoundary() {
     // See https://github.com/RoaringBitmap/RoaringBitmap/issues/285