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

In rare cases, RBF can produce containers which have a recorded N value which
is incorrect. This rarely affects anything, but on some particular queries,
this can result in very strange outcomes, like array containers with more
than 1<<16 entries.

To fix this, we have toContainer specify that it doesn't know the correct
N for the bitmap containers it's creating, which costs extra time for counting,
and should be considered a temporary workaround.

Also, we add a CheckN() function which is controlled by the
roaringparanoia flag, and add a number of calls to it, for instance, as
deferred calls after every container operation when roaringparanoia is
enabled. This means that we get improved confidence that we've caught
the relevant errors, but is not suitable for production use.

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))
+	}
+}