From fb4651cdb8092e1dfad3b0384b57d306ce351282 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Thu, 18 May 2017 12:04:37 -0500 Subject: [PATCH 1/5] fix 3 separate bugs in bitmapCountRange in order of the diff: 1. When the start and end of the range fall in the same word, special handling is needed to "mask" off the beginning and end of the word simultaneously to avoid counting bits at the beginning or end of the word that aren't in the range. 2. `i++` is needed at the end of the first partial word to avoid counting this word in the next block. 3. the shift amount for right shifts is 64 - (end % 64) rather than just end % 64. If end is (e.g.) 68, then 68 - 64 is 4 and we are only interested in the first 4 bits of the word, so we must right shift by 60 bits, not 4 bits. --- roaring/roaring.go | 10 +++++++++- roaring/roaring_internal_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 roaring/roaring_internal_test.go diff --git a/roaring/roaring.go b/roaring/roaring.go index e0810a6c2..1a4d8d486 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -904,9 +904,17 @@ func (c *container) bitmapCountRange(start, end uint32) int { var n uint64 i, j := start/64, end/64 + // Special case when start and end fall in the same word. + if i == j { + offi, offj := start%64, 64-end%64 + n += popcount((c.bitmap[i] << offi) >> (offj + offi)) + return int(n) + } + // Count partial starting word. if off := start % 64; off != 0 { n += popcount(c.bitmap[i] << off) + i++ } // Count words in between. @@ -916,7 +924,7 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Count partial ending word. if int(j) < len(c.bitmap) { - if off := end % 64; off != 0 { + if off := 64 - (end % 64); off != 0 { n += popcount(c.bitmap[j] >> off) } } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go new file mode 100644 index 000000000..17a35c5e1 --- /dev/null +++ b/roaring/roaring_internal_test.go @@ -0,0 +1,31 @@ +package roaring + +import ( + "testing" +) + +func TestBitmapCountRange(t *testing.T) { + c := container{bitmap: []uint64{1}} + cnt := c.bitmapCountRange(63, 65) + if cnt != 1 { + t.Fatalf("count of %v from 63 to 65 should be 1, but got %v", c.bitmap, cnt) + } + + c = container{bitmap: []uint64{0, 0x8000000000000000}} + cnt = c.bitmapCountRange(65, 66) + if cnt != 0 { + t.Fatalf("count of %v from 65 to 66 should be 0, but got %v", c.bitmap, cnt) + } + + c = container{bitmap: []uint64{0, 0xF000000000000000}} + cnt = c.bitmapCountRange(65, 66) + if cnt != 1 { + t.Fatalf("count of %v from 65 to 66 should be 1, but got %v", c.bitmap, cnt) + } + + c = container{bitmap: []uint64{0x1, 0xFF00000000000000}} + cnt = c.bitmapCountRange(62, 66) + if cnt != 3 { + t.Fatalf("count of %v from 62 to 66 should be 3, but got %v", c.bitmap, cnt) + } +} From c0ddbe0e3fba41573cc46ee510153ac3d6f5799a Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 19 May 2017 15:15:23 -0500 Subject: [PATCH 2/5] fix bitmapCountRange and test had been thinking that index 0 was the most significant bit, but based on the bitmapAdd function, it must be the least significant bit --- roaring/roaring.go | 6 ++--- roaring/roaring_internal_test.go | 40 +++++++++++++++----------------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 1a4d8d486..57825a5b2 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -907,13 +907,13 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Special case when start and end fall in the same word. if i == j { offi, offj := start%64, 64-end%64 - n += popcount((c.bitmap[i] << offi) >> (offj + offi)) + n += popcount((c.bitmap[i] >> offi) << (offj + offi)) return int(n) } // Count partial starting word. if off := start % 64; off != 0 { - n += popcount(c.bitmap[i] << off) + n += popcount(c.bitmap[i] >> off) i++ } @@ -925,7 +925,7 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Count partial ending word. if int(j) < len(c.bitmap) { if off := 64 - (end % 64); off != 0 { - n += popcount(c.bitmap[j] >> off) + n += popcount(c.bitmap[j] << off) } } diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 17a35c5e1..5cff4a30f 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -5,27 +5,25 @@ import ( ) func TestBitmapCountRange(t *testing.T) { - c := container{bitmap: []uint64{1}} - cnt := c.bitmapCountRange(63, 65) - if cnt != 1 { - t.Fatalf("count of %v from 63 to 65 should be 1, but got %v", c.bitmap, cnt) + c := container{} + tests := []struct { + start uint32 + end uint32 + bitmap []uint64 + exp int + }{ + {start: 0, end: 1, bitmap: []uint64{1}, exp: 1}, + {start: 2, end: 7, bitmap: []uint64{0xFFFFFFFFFFFFFF18}, exp: 2}, + {start: 67, end: 68, bitmap: []uint64{0, 0x8}, exp: 1}, + {start: 1, end: 68, bitmap: []uint64{0x3, 0x8, 0xF}, exp: 2}, + {start: 1, end: 258, bitmap: []uint64{0xF, 0x8, 0xA, 0x4, 0xFFFFFFFFFFFFFFFF}, exp: 9}, + {start: 66, end: 71, bitmap: []uint64{0xF, 0xFFFFFFFFFFFFFF18}, exp: 2}, + {start: 63, end: 64, bitmap: []uint64{0x8000000000000000}, exp: 1}, } - - c = container{bitmap: []uint64{0, 0x8000000000000000}} - cnt = c.bitmapCountRange(65, 66) - if cnt != 0 { - t.Fatalf("count of %v from 65 to 66 should be 0, but got %v", c.bitmap, cnt) - } - - c = container{bitmap: []uint64{0, 0xF000000000000000}} - cnt = c.bitmapCountRange(65, 66) - if cnt != 1 { - t.Fatalf("count of %v from 65 to 66 should be 1, but got %v", c.bitmap, cnt) - } - - c = container{bitmap: []uint64{0x1, 0xFF00000000000000}} - cnt = c.bitmapCountRange(62, 66) - if cnt != 3 { - t.Fatalf("count of %v from 62 to 66 should be 3, but got %v", c.bitmap, cnt) + for i, test := range tests { + c.bitmap = test.bitmap + if ret := c.bitmapCountRange(test.start, test.end); ret != test.exp { + t.Fatalf("test #%v count of %v from %v to %v should be %v but got %v", i, test.bitmap, test.start, test.end, test.exp, ret) + } } } From a5918d01d9d41b6a2512d1eeba8aa7b60d8b6012 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Fri, 19 May 2017 17:09:22 -0500 Subject: [PATCH 3/5] fix offset check to make sense --- roaring/roaring.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 57825a5b2..ee46622f1 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -924,7 +924,7 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Count partial ending word. if int(j) < len(c.bitmap) { - if off := 64 - (end % 64); off != 0 { + if off := 64 - (end % 64); off != 64 { n += popcount(c.bitmap[j] << off) } } From 6b3b459659aedeae9fb6ba3dc9e05cf2ff0db454 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Mon, 22 May 2017 09:46:52 -0500 Subject: [PATCH 4/5] add license to roaring_internal_test --- roaring/roaring_internal_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/roaring/roaring_internal_test.go b/roaring/roaring_internal_test.go index 5cff4a30f..41d0d167e 100644 --- a/roaring/roaring_internal_test.go +++ b/roaring/roaring_internal_test.go @@ -1,3 +1,17 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package roaring import ( From e90e1455db96773379ba6b74f07e4bee6dc4fe9f Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Mon, 22 May 2017 10:38:40 -0500 Subject: [PATCH 5/5] remove branch and always count last container --- roaring/roaring.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index ee46622f1..86d810450 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -924,9 +924,8 @@ func (c *container) bitmapCountRange(start, end uint32) int { // Count partial ending word. if int(j) < len(c.bitmap) { - if off := 64 - (end % 64); off != 64 { - n += popcount(c.bitmap[j] << off) - } + off := 64 - (end % 64) + n += popcount(c.bitmap[j] << off) } return int(n)