From a6ba7e339cb907bf7004e2a7ed79b702ce4b7a31 Mon Sep 17 00:00:00 2001 From: Shaquille Wyan Que Date: Thu, 20 Jun 2019 20:50:58 -0500 Subject: [PATCH 1/2] fix container iteration bugs in roaring --- roaring/roaring.go | 83 ++++++++++++++++++++++++---- roaring/roaring_internal_test.go | 92 ++++++++++++++++++++++++++++++++ roaring/roaring_test.go | 72 ++++++++++++++++++++----- 3 files changed, 223 insertions(+), 24 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 00ca1ed4f..520a5ffe8 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -452,6 +452,16 @@ func (b *Bitmap) CountRange(start, end uint64) (n uint64) { // TODO remove once we've validated this stuff works panic("should be impossible for k to be less than skey") } + if roaringParanoia { + if start > end { + panic(fmt.Sprintf("counting in range but %v > %v", start, end)) + } + } + // k > ekey handles the case when start > end and where start and end + // are in different containers. Same container case is already handled above. + if k > ekey { + break + } if k == skey { n += uint64(c.countRange(int32(lowbits(start)), maxContainerVal+1)) continue @@ -464,9 +474,6 @@ func (b *Bitmap) CountRange(start, end uint64) (n uint64) { n += uint64(c.countRange(0, int32(lowbits(end)))) break } - if k > ekey { - break - } } return n } @@ -485,6 +492,11 @@ func (b *Bitmap) Slice() []uint64 { // SliceRange returns a slice of integers between [start, end). func (b *Bitmap) SliceRange(start, end uint64) []uint64 { + if roaringParanoia { + if start > end { + panic(fmt.Sprintf("getting slice in range but %v > %v", start, end)) + } + } var a []uint64 itr := b.Iterator() itr.Seek(start) @@ -934,6 +946,11 @@ func (b *Bitmap) Shift(n int) (*Bitmap, error) { lastKey := uint64(0) for iiter.Next() { ki, ci := iiter.Value() + if lastCarry && ki > lastKey+1 { + extra := NewContainerArray([]uint16{0}) + output.Containers.Put(lastKey+1, extra) + lastCarry = false + } o, carry := shift(ci) if lastCarry { o.add(0) @@ -1249,6 +1266,11 @@ func (b *Bitmap) Check() error { // Flip performs a logical negate of the bits in the range [start,end]. func (b *Bitmap) Flip(start, end uint64) *Bitmap { + if roaringParanoia { + if start > end { + panic(fmt.Sprintf("flipping in range but %v > %v", start, end)) + } + } result := NewBitmap() itr := b.Iterator() v, eof := itr.Next() @@ -1310,8 +1332,14 @@ func (itr *Iterator) Seek(seek uint64) { } // Move to the correct value index inside the container. - lb := lowbits(seek) + lb, hb := lowbits(seek), highbits(seek) if itr.c.isArray() { + // Seek is smaller than min(itr.c). + if itr.key > hb { + itr.j = -1 + return + } + // Find index in the container. itr.j = search32(itr.c.array(), lb) if itr.j < 0 { @@ -1333,24 +1361,37 @@ func (itr *Iterator) Seek(seek uint64) { } if itr.c.isRun() { - if seek == 0 { - itr.j, itr.k = 0, -1 + // Seek is smaller than min(itr.c). + if itr.key > hb { + itr.j = 0 + itr.k = -1 + return } j, contains := binSearchRuns(lb, itr.c.runs()) if contains { itr.j = j itr.k = int32(lb) - int32(itr.c.runs()[j].start) - 1 - } else { - // Set iterator to next value in the Bitmap. - itr.j = j - itr.k = -1 + return } - + // If seek is larger than all elements, return. + if j >= int32(len(itr.c.runs())) { + if !itr.citer.Next() { + itr.c = nil + return + } + } + // Set iterator to next value in the Bitmap. + itr.j = j + itr.k = -1 return } // If it's a bitmap container then move to index before the value and call next(). + if itr.key > hb { + itr.j = -1 + return + } itr.j = int32(lb) - 1 } @@ -1502,6 +1543,11 @@ func (c *Container) countRange(start, end int32) (n int32) { if c == nil { return 0 } + if roaringParanoia { + if start > end { + panic(fmt.Sprintf("counting in range but %v > %v", start, end)) + } + } if c.isArray() { return c.arrayCountRange(start, end) } else if c.isRun() { @@ -1511,6 +1557,11 @@ func (c *Container) countRange(start, end int32) (n int32) { } func (c *Container) arrayCountRange(start, end int32) (n int32) { + if roaringParanoia { + if start > end { + panic(fmt.Sprintf("counting in range but %v > %v", start, end)) + } + } array := c.array() i := int32(sort.Search(len(array), func(i int) bool { return int32(array[i]) >= start })) for ; i < int32(len(array)); i++ { @@ -1524,6 +1575,11 @@ func (c *Container) arrayCountRange(start, end int32) (n int32) { } func (c *Container) bitmapCountRange(start, end int32) int32 { + if roaringParanoia { + if start > end { + panic(fmt.Sprintf("counting in range but %v > %v", start, end)) + } + } var n uint64 i, j := start/64, end/64 // Special case when start and end fall in the same word. @@ -1555,6 +1611,11 @@ func (c *Container) bitmapCountRange(start, end int32) int32 { } func (c *Container) runCountRange(start, end int32) (n int32) { + if roaringParanoia { + if start > end { + panic(fmt.Sprintf("counting in range but %v > %v", start, end)) + } + } runs := c.runs() for _, iv := range runs { // iv is before range diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 95c8d1b0b..d064d864f 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -2025,6 +2025,15 @@ func TestIteratorArray(t *testing.T) { t.Fatalf("iterator did not next correctly across containers: %v\n", itr) } + itr.Seek(65535) + if !(itr.key == 1 && itr.j == -1) { + t.Fatalf("iterator did not seek missing value in previous container correctly: %v\n", itr) + } + val, eof = itr.Next() + if !(val == 90000 && !eof) { + t.Fatalf("iterator did not next from missing value in previous container correctly: %d, %v\n", val, eof) + } + itr.Seek(80000) if !(itr.key == 1 && itr.j == -1) { t.Fatalf("iterator did not seek missing value correctly: %v\n", itr) @@ -2044,6 +2053,24 @@ func TestIteratorArray(t *testing.T) { if !(val == 0 && eof) { t.Fatalf("iterator did not eof correctly: %d, %v\n", val, eof) } + + // Test for seeking value not in bitmap, where next container that the iterator should + // go to has values with low bits smaller than the low bits of seek. + b = NewBitmap(65537, 65538, 65539, 65541, 65542) + itr = b.Iterator() + // Both 65536+5-1 and 5 are not in b. + itr.Seek(5) + if !(itr.key == 1 && itr.j == -1) { + t.Fatalf("iterator did not seek correctly in next container: %v\n", itr) + } + val, eof = itr.Next() + if !(val == 65537 && !eof) { + t.Fatalf("iterator did not next corrrectly to next container: %d, %v\n", val, eof) + } + val, eof = itr.Next() + if !(val == 65538 && !eof) { + t.Fatalf("iterator did not next correctly: %d, %v\n", val, eof) + } } func TestIteratorBitmap(t *testing.T) { @@ -2102,6 +2129,39 @@ func TestIteratorBitmap(t *testing.T) { if !(val == 0 && eof) { t.Fatalf("iterator did not eof correctly: %d, %v\n", val, eof) } + + // Test for seeking value not in bitmap, where next container that the iterator should + // go to has values with low bits smaller than the low bits of seek. + for i := uint64(65536*3 + 2); i < 65536*3+7; i++ { + if i != 65536*3+5 { + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bit: %v", err) + } + } + } + for i := uint64(65536*3 + 8); i < 65536*3+4110; i++ { + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bit: %v", err) + } + } + + if !b.Containers.Get(3).isBitmap() { + t.Fatalf("wrong container type") + } + + // Both 65536*2+5 and 65536*3+5 are not in b. + itr.Seek(65536*2 + 5) + if !(itr.key == 3 && itr.j == -1) { + t.Fatalf("iterator did not seek correctly in next container: %v\n", itr) + } + val, eof = itr.Next() + if !((val == 65536*3+2) && !eof) { + t.Fatalf("iterator did not next correctly to next container: %d, %v\n", val, eof) + } + val, eof = itr.Next() + if !((val == 65536*3+3) && !eof) { + t.Fatalf("iterator did not next correctly to next container: %d, %v\n", val, eof) + } } func TestIteratorRuns(t *testing.T) { @@ -2164,6 +2224,38 @@ func TestIteratorRuns(t *testing.T) { if !(val == 0 && eof) { t.Fatalf("iterator did not eof correctly: %d, %v\n", val, eof) } + + // Test for seeking value not in bitmap, where next container that the iterator should + // go to has values with low bits smaller than the low bits of seek. + for i := uint64(65536*3 + 1); i <= 65536*3+8; i++ { + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bit: %v", err) + } + } + for i := uint64(65536*3 + 10); i <= 65536*3+20; i++ { + if _, err := b.Add(i); err != nil { + t.Fatalf("adding bit: %v", err) + } + } + b.Optimize() + + if !b.Containers.Get(3).isRun() { + t.Fatalf("wrong container type") + } + + // Both 65536*2+9 and 65536*3+9 are not in b. + itr.Seek(65536*2 + 9) + if !(itr.key == 3 && itr.j == 0 && itr.k == -1) { + t.Fatalf("iterator did not seek correctly in next container: %v\n", itr) + } + val, eof = itr.Next() + if !((val == 65536*3+1) && !eof) { + t.Fatalf("iterator did not next correctly to next container: %d, %v\n", val, eof) + } + val, eof = itr.Next() + if !((val == 65536*3+2) && !eof) { + t.Fatalf("iterator did not next correctly to next container: %d, %v\n", val, eof) + } } func TestIteratorVarious(t *testing.T) { diff --git a/roaring/roaring_test.go b/roaring/roaring_test.go index fe966be17..dc3fc4c76 100644 --- a/roaring/roaring_test.go +++ b/roaring/roaring_test.go @@ -146,6 +146,20 @@ func TestCountRange(t *testing.T) { end: 6 * 65536, exp: 1, }, + { + name: "start < end in different containers", + bitmap: []uint64{65537, 65538, 65539, 65540}, + start: 65536, + end: 2, + exp: 0, + }, + { + name: "start == end", + bitmap: []uint64{65537, 65538, 65539, 65540}, + start: 65537, + end: 65537, + exp: 0, + }, } for _, test := range tests { @@ -390,6 +404,11 @@ func TestBitmap_BitmapCountRange(t *testing.T) { if n := bm0.CountRange(10000000, 10000001); n != 0 { t.Fatalf("unexpected n: %d", n) } + + // Test the case where start < end and bitmap contains value in the same container as start. + if n := bm0.CountRange(65536, 2); n != 0 { + t.Fatalf("unexpected n: %d", n) + } } func TestBitmap_ArrayCountRange(t *testing.T) { @@ -397,6 +416,11 @@ func TestBitmap_ArrayCountRange(t *testing.T) { if n := bm0.CountRange(1, 2683313); n != 1 { t.Fatalf("unexpected n: %d", n) } + + // Test the case where start < end and bitmap contains value in the same container as start. + if n := bm0.CountRange(2621440, 2); n != 0 { + t.Fatalf("unexpected n: %d", n) + } } func TestBitmap_DirectAdd(t *testing.T) { @@ -427,6 +451,12 @@ func TestBitmap_RunCountRange(t *testing.T) { if n := bm1.CountRange(5, 12); n != 7 { t.Fatalf("unexpected n: %d", n) } + + bm2 := roaring.NewFileBitmap(65536, 65537, 65538, 65539, 65540, 65541, 65542, 65543, 65544, 65545, 65546, 65547, 65548, 65549, 65550, 65551, 65552, 65553) + bm2.Optimize() // convert to runs + if n := bm2.CountRange(3, 2); n != 0 { + t.Fatalf("unexpected n: %d", n) + } } func TestBitmap_Intersection(t *testing.T) { @@ -1063,6 +1093,22 @@ func TestBitmap_Shift(t *testing.T) { } else if !reflect.DeepEqual(got.Slice(), bm2.Slice()) { t.Fatalf("unexpected bitmap: expected %v, but got %v", bm2.Slice(), got.Slice()) } + + bm1 = roaring.NewFileBitmap(65535, 131073) + bm2 = roaring.NewFileBitmap(65536, 131074) + if got, err := bm1.Shift(1); err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual(got.Slice(), bm2.Slice()) { + t.Fatalf("unexpected bitmap: expected %v, but got %v", bm2.Slice(), got.Slice()) + } + + bm1 = roaring.NewFileBitmap(65535, 131073, 65536*5-1, 65536*10, 65536*15-1) + bm2 = roaring.NewFileBitmap(65536, 131074, 65536*5, 65536*10+1, 65536*15) + if got, err := bm1.Shift(1); err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual(got.Slice(), bm2.Slice()) { + t.Fatalf("unexpected bitmap: expected %v, but got %v", bm2.Slice(), got.Slice()) + } } func TestBitmap_Quick_Array1(t *testing.T) { testBitmapQuick(t, 1000, 1000, 2000) } @@ -1078,17 +1124,17 @@ func testBitmapQuick(t *testing.T, n int, min, max uint64) { m := make(map[uint64]struct{}) // Add values to the bitmap and set. - manual_count := uint64(0) + manualCount := uint64(0) for _, v := range a { - new_bit, _ := bm.Add(v) - if new_bit { - manual_count++ + newBit, _ := bm.Add(v) + if newBit { + manualCount++ } m[v] = struct{}{} } //check count - if manual_count != bm.Count() { - t.Fatalf("expected bitmap Add count to be: %d got: %d", manual_count, bm.Count()) + if manualCount != bm.Count() { + t.Fatalf("expected bitmap Add count to be: %d got: %d", manualCount, bm.Count()) } // Verify existence. @@ -1116,12 +1162,12 @@ func testBitmapQuick(t *testing.T, n int, min, max uint64) { for i, item := range rand.Perm(len(a)) { removed, _ := bm.Remove(a[item]) if removed { - manual_count-- + manualCount-- } //check count - if manual_count != bm.Count() { + if manualCount != bm.Count() { t.Fatalf("removing %d/%d [%d] from bitmap: expected bitmap Remove count to be %d, got %d", - i, len(a), a[item], manual_count, bm.Count()) + i, len(a), a[item], manualCount, bm.Count()) } } @@ -1241,13 +1287,13 @@ func TestIterator(t *testing.T) { t.Run("run", func(t *testing.T) { bm1 := roaring.NewFileBitmap() - for i := uint64(0); i < 11; i += 1 { + for i := uint64(0); i < 11; i++ { _, _ = bm1.Add(i) } bm1.Optimize() bm2 := roaring.NewFileBitmap() - for i := uint64(0); i < 12; i += 1 { + for i := uint64(0); i < 12; i++ { _, _ = bm2.Add(i) } bm2.Optimize() @@ -1290,11 +1336,11 @@ func testBM() *roaring.Bitmap { _, _ = bm.Add((2 << 16) + i) } //small run - for i := uint64(0); i < 1024; i += 1 { + for i := uint64(0); i < 1024; i++ { _, _ = bm.Add((3 << 16) + i) } //large run - for i := uint64(0); i < 65535; i += 1 { + for i := uint64(0); i < 65535; i++ { _, _ = bm.Add((4 << 16) + i) } bm.Optimize() From 86e703637b9836803dd0b07b53fa046773aeca58 Mon Sep 17 00:00:00 2001 From: shaqque Date: Mon, 24 Jun 2019 15:18:03 -0500 Subject: [PATCH 2/2] fixed seeking end of run container iteration bug when next container exists and ensure roaringparanoia panics before other ops --- roaring/roaring.go | 23 ++++++++++++++--------- roaring/roaring_internal_test.go | 20 +++++++++++++------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 520a5ffe8..5cb592881 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -431,6 +431,12 @@ func (b *Bitmap) Size() int { // CountRange returns the number of bits set between [start, end). func (b *Bitmap) CountRange(start, end uint64) (n uint64) { + if roaringParanoia { + if start > end { + panic(fmt.Sprintf("counting in range but %v > %v", start, end)) + } + } + if b.Containers.Size() == 0 { return } @@ -452,11 +458,7 @@ func (b *Bitmap) CountRange(start, end uint64) (n uint64) { // TODO remove once we've validated this stuff works panic("should be impossible for k to be less than skey") } - if roaringParanoia { - if start > end { - panic(fmt.Sprintf("counting in range but %v > %v", start, end)) - } - } + // k > ekey handles the case when start > end and where start and end // are in different containers. Same container case is already handled above. if k > ekey { @@ -1380,6 +1382,9 @@ func (itr *Iterator) Seek(seek uint64) { itr.c = nil return } + itr.key, itr.c = itr.citer.Value() + itr.j = -1 + return } // Set iterator to next value in the Bitmap. itr.j = j @@ -1387,7 +1392,7 @@ func (itr *Iterator) Seek(seek uint64) { return } - // If it's a bitmap container then move to index before the value and call next(). + // If it's a bitmap container then move to index before the value. if itr.key > hb { itr.j = -1 return @@ -1540,14 +1545,14 @@ func (c *Container) count() (n int32) { // countRange counts the number of bits set between [start, end). func (c *Container) countRange(start, end int32) (n int32) { - if c == nil { - return 0 - } if roaringParanoia { if start > end { panic(fmt.Sprintf("counting in range but %v > %v", start, end)) } } + if c == nil { + return 0 + } if c.isArray() { return c.arrayCountRange(start, end) } else if c.isRun() { diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index d064d864f..12dd73d26 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -2132,19 +2132,16 @@ func TestIteratorBitmap(t *testing.T) { // Test for seeking value not in bitmap, where next container that the iterator should // go to has values with low bits smaller than the low bits of seek. - for i := uint64(65536*3 + 2); i < 65536*3+7; i++ { - if i != 65536*3+5 { + for i := uint64(65536*3 + 2); i < 65536*3+4110; i++ { + if i != 65536*3+5 && i != 65536*3+7 { if _, err := b.Add(i); err != nil { t.Fatalf("adding bit: %v", err) } } } - for i := uint64(65536*3 + 8); i < 65536*3+4110; i++ { - if _, err := b.Add(i); err != nil { - t.Fatalf("adding bit: %v", err) - } - } + // We expect this to be a bitmap container because more than + // 4096 bits have been set, but Optimize() has not been called. if !b.Containers.Get(3).isBitmap() { t.Fatalf("wrong container type") } @@ -2214,6 +2211,15 @@ func TestIteratorRuns(t *testing.T) { t.Fatalf("iterator did not seek correctly to end of run: %v\n", itr) } + itr.Seek(1007) + if !(itr.key == 1 && itr.j == -1 && itr.k == -1) { + t.Fatalf("iterator did not seek correctly to end of run: %v\n", itr) + } + val, eof = itr.Next() + if !(val == 100000 && !eof) { + t.Fatalf("iterator did not next correctly across containers: %v, %v", val, itr) + } + itr.Seek(100005) if !(itr.key == 1 && itr.j == 0 && itr.k == 4) { t.Fatalf("iterator did not seek correctly in multiple containers: %v\n", itr)