Merge pull request #976 from seebs/impossible

Check more carefully for, and also fix, containers with invalid N

rbf/cursor.go

@@ -1180,7 +1180,7 @@ func (c *Cursor) merge(key uint64, data *roaring.Container) (bool, error) {
 		if err != nil {
 			return false, errors.Wrap(err, "cursor.merge")
 		}
-		container = roaring.NewContainerBitmap(cell.BitN, d)
+		container = roaring.NewContainerBitmap(-1, d)
 	case ContainerTypeRLE:
 		d := toInterval16(cell.Data)
 		container = roaring.NewContainerRun(d)
@@ -1267,7 +1267,7 @@ func (c *Cursor) difference(key uint64, data *roaring.Container) (bool, error) {
 		if err != nil {
 			return false, errors.Wrap(err, "cursor.difference")
 		}
-		container = roaring.NewContainerBitmap(cell.N, d)
+		container = roaring.NewContainerBitmap(-1, d)
 	case ContainerTypeRLE:
 		d := toInterval16(cell.Data)
 		container = roaring.NewContainerRun(d)

rbf/cursorx.go

@@ -179,12 +179,16 @@ func toContainer(l leafCell, tx *Tx) (c *roaring.Container) {
 			cloneMaybe = make([]uint64, len(bm))
 			copy(cloneMaybe, bm)
 		}
-		c = roaring.NewContainerBitmap(l.N, cloneMaybe)
+		c = roaring.NewContainerBitmap(-1, cloneMaybe)
 	case ContainerTypeBitmap:
-		c = roaring.NewContainerBitmap(l.N, toArray64(cpMaybe))
+		c = roaring.NewContainerBitmap(-1, toArray64(cpMaybe))
 	case ContainerTypeRLE:
 		c = roaring.NewContainerRun(toInterval16(cpMaybe))
 	}
+	// Note: If the "roaringparanoia" build tag isn't set, this
+	// should be optimized away entirely. Otherwise it's moderately
+	// expensive.
+	c.CheckN()
 	c.SetMapped(mapped)
 	return c
 }

roaring/container_stash.go

@@ -146,6 +146,9 @@ func NewContainerBitmap(n int, bitmap []uint64) *Container {
 		c.bitmapRepair()
 	} else {
 		c.setN(int32(n))
+		if roaringParanoia {
+			c.CheckN()
+		}
 	}
 	return c
 }
@@ -164,6 +167,9 @@ func NewContainerBitmapN(bitmap []uint64, n int32) *Container {
 	} else {
 		c.setBitmap(bitmap)
 	}
+	if roaringParanoia {
+		c.CheckN()
+	}
 	return c
 }
 
@@ -219,6 +225,9 @@ func NewContainerRunCopy(set []Interval16) *Container {
 func NewContainerRunN(set []Interval16, n int32) *Container {
 	c := &Container{typeID: ContainerRun, n: n}
 	c.setRuns(set)
+	if roaringParanoia {
+		c.CheckN()
+	}
 	return c
 }
 
@@ -624,61 +633,6 @@ func (c *Container) setRunsMaybeCopy(runs []Interval16, doCopy bool) {
 	c.pointer, c.len, c.cap = &runs[0].Start, int32(len(runs)), int32(cap(runs))
 }
 
-// UpdateOrMake updates the container, yielding a new container if necessary.
-func (c *Container) UpdateOrMake(typ byte, n int32, mapped bool) *Container {
-	if c == nil {
-		switch typ {
-		case ContainerRun:
-			c = NewContainerRunN(nil, n)
-		case ContainerBitmap:
-			c = NewContainerBitmapN(nil, n)
-		default:
-			c = NewContainerArrayN(nil, n)
-		}
-		c.flags |= flagMapped
-		return c
-	}
-	// ensure that we are allowed to modify this container
-	c = c.Thaw()
-	c.typeID = typ
-	c.n = n
-	// note: this probably shouldn't be happening, the decision should be getting
-	// made when we specify the storage.
-	c.setMapped(mapped)
-	// we don't know that any existing slice is usable, so let's ditch it
-	switch c.typeID {
-	case ContainerArray:
-		c.pointer, c.len, c.cap = &c.data[0], 0, stashedArraySize
-	case ContainerRun:
-		c.pointer, c.len, c.cap = &c.data[0], 0, stashedRunSize
-	default:
-		c.pointer, c.len, c.cap = nil, 0, 0
-	}
-	return c
-}
-
-// Update updates the container if possible. It is an error to
-// call Update on a frozen container.
-func (c *Container) Update(typ byte, n int32, mapped bool) {
-	if c == nil || c.frozen() {
-		panic("cannot Update a nil or frozen container")
-	}
-	c.typeID = typ
-	c.n = n
-	// note: this probably shouldn't be happening, the decision should be getting
-	// made when we specify the storage.
-	c.setMapped(mapped)
-	// we don't know that any existing slice is usable, so let's ditch it
-	switch c.typeID {
-	case ContainerArray:
-		c.pointer, c.len, c.cap = nil, 0, 0
-	case ContainerRun:
-		c.pointer, c.len, c.cap = nil, 0, 0
-	default:
-		c.pointer, c.len, c.cap = nil, 0, 0
-	}
-}
-
 // isArray returns true if the container is an array container.
 func (c *Container) isArray() bool {
 	if c == nil {

roaring/roaring.go

@@ -3465,8 +3465,11 @@ func (c *Container) bitmapToArray() *Container {
 }
 
 // arrayToBitmap converts from array format to bitmap format.
-func (c *Container) arrayToBitmap() *Container {
+func (c *Container) arrayToBitmap() (out *Container) {
 	statsHit("arrayToBitmap")
+	if roaringParanoia {
+		defer func() { out.CheckN() }()
+	}
 	if c == nil {
 		if roaringParanoia {
 			panic("nil container for arrayToBitmap")
@@ -3498,8 +3501,11 @@ func (c *Container) arrayToBitmap() *Container {
 }
 
 // runToBitmap converts from RLE format to bitmap format.
-func (c *Container) runToBitmap() *Container {
+func (c *Container) runToBitmap() (out *Container) {
 	statsHit("runToBitmap")
+	if roaringParanoia {
+		defer func() { c.CheckN() }()
+	}
 	if c == nil {
 		if roaringParanoia {
 			panic("nil container for runToBitmap")
@@ -3725,6 +3731,9 @@ func (c *Container) runToArray() *Container {
 
 // Clone returns a copy of c.
 func (c *Container) Clone() (out *Container) {
+	if roaringParanoia {
+		defer func() { out.CheckN() }()
+	}
 	statsHit("Container/Clone")
 	if c == nil {
 		return nil
@@ -3735,8 +3744,9 @@ func (c *Container) Clone() (out *Container) {
 		out = NewContainerArrayCopy(c.array())
 	case ContainerBitmap:
 		statsHit("Container/Clone/Bitmap")
-		other := NewContainerBitmapN(nil, c.N())
+		other := NewContainerBitmapN(nil, 0)
 		copy(other.bitmap(), c.bitmap())
+		other.n = c.n
 		out = other
 	case ContainerRun:
 		statsHit("Container/Clone/Run")
@@ -4098,7 +4108,10 @@ func intersectionCountBitmapBitmap(a, b *Container) (n int32) {
 	return int32(popcountAndSlice(a.bitmap(), b.bitmap()))
 }
 
-func intersect(a, b *Container) *Container {
+func intersect(a, b *Container) (c *Container) {
+	if roaringParanoia {
+		defer func() { c.CheckN() }()
+	}
 	if a.N() == MaxContainerVal+1 {
 		return b.Freeze()
 	}
@@ -4320,7 +4333,10 @@ func intersectBitmapBitmap(a, b *Container) *Container {
 	return output
 }
 
-func union(a, b *Container) *Container {
+func union(a, b *Container) (c *Container) {
+	if roaringParanoia {
+		defer func() { c.CheckN() }()
+	}
 	if a.N() == MaxContainerVal+1 || b.N() == MaxContainerVal+1 {
 		return fullContainer
 	}
@@ -5029,7 +5045,10 @@ func appendInterval16At(a []Interval16, val Interval16, off int) ([]Interval16,
 	return a, off
 }
 
-func difference(a, b *Container) *Container {
+func difference(a, b *Container) (c *Container) {
+	if roaringParanoia {
+		defer func() { c.CheckN() }()
+	}
 	if a.N() == 0 || b.N() == MaxContainerVal+1 {
 		return nil
 	}
@@ -5386,7 +5405,10 @@ func differenceBitmapBitmap(a, b *Container) *Container {
 	return output
 }
 
-func xor(a, b *Container) *Container {
+func xor(a, b *Container) (c *Container) {
+	if roaringParanoia {
+		defer func() { c.CheckN() }()
+	}
 	if a.N() == 0 {
 		return b.Freeze()
 	}

roaring/roaring_internal_test.go

@@ -1895,9 +1895,10 @@ func TestWriteReadArray(t *testing.T) {
 
 func TestWriteReadBitmap(t *testing.T) {
 	// create bitmap containing > 4096 bits
-	cb := NewContainerBitmapN(nil, 129*32)
+	cb := NewContainerBitmapN(nil, 0)
 	for i := 0; i < 129; i++ {
 		cb.bitmap()[i] = 0x5555555555555555
+		cb.n += 32
 	}
 	bb := NewFileBitmap()
 	bb.Containers.Put(0, cb)
@@ -1918,9 +1919,10 @@ func TestWriteReadBitmap(t *testing.T) {
 
 func TestWriteReadFullBitmap(t *testing.T) {
 	// create bitmap containing > 4096 bits
-	cb := NewContainerBitmapN(nil, 65536)
+	cb := NewContainerBitmapN(nil, 0)
 	for i := 0; i < bitmapN; i++ {
 		cb.bitmap()[i] = 0xffffffffffffffff
+		cb.n += 64
 	}
 	bb := NewFileBitmap()
 	bb.Containers.Put(0, cb)

roaring/roaring_nop_paranoia.go

@@ -17,3 +17,10 @@
 package roaring
 
 const roaringParanoia = false
+
+// CheckN verifies that a container's cached count is correct, but
+// there are two versions; this is the one which doesn't actually
+// do anything, because the check is expensive. Which one you get is
+// controlled by the roaringparanoia build tag.
+func (c *Container) CheckN() {
+}

roaring/roaring_paranoia.go

@@ -16,4 +16,19 @@
 
 package roaring
 
+import "fmt"
+
 const roaringParanoia = true
+
+// CheckN verifies that the container's cached count is correct. Note
+// that this has two definitions, depending on the presence of the
+// roaringparanoia build tag.
+func (c *Container) CheckN() {
+	if c == nil {
+		return
+	}
+	count := c.count()
+	if count != c.n {
+		panic(fmt.Sprintf("CheckN (%p): n %d, count %d", c, c.n, count))
+	}
+}