Merge pull request #487 from RoaringBitmap/fixing_issue_486

fixing issue 486

Author: lemire

roaring.go

@@ -1477,7 +1477,6 @@ main:
 				}
 				s2 = x2.highlowcontainer.getKeyAtIndex(pos2)
 			} else {
-
 				answer.highlowcontainer.appendContainer(s1, x1.highlowcontainer.getContainerAtIndex(pos1).or(x2.highlowcontainer.getContainerAtIndex(pos2)), false)
 				pos1++
 				pos2++
@@ -1516,6 +1515,7 @@ main:
 				if !C.isEmpty() {
 					answer.highlowcontainer.appendContainer(s1, C, false)
 				}
+
 				pos1++
 				pos2++
 				if (pos1 == length1) || (pos2 == length2) {

roaring_test.go

@@ -15,6 +15,54 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+var bm1Arr = []uint32{279981785, 279982923, 279988809, 279995913}
+
+const bm2Bytes = "OzAAAAGwEO2EbwQBdgoADXZFAFR2BABadgYAYnaiAAZ3GwAjdx0AQndaAJ533AB8eA4AjHgUAKJ4IQDFeA8B1nkLAON5FgD7eRQAEXoDABZ6AAAYeggAInpRAHV6BwB+egMAg3oFAIp6NgDCegYAynoVAOF6AQDkehYA/HpCAEF7ZACnewMArXsBALB7CQC7ewUAw3sJAM97DwDgewIA5HsQAPZ7BQD9exEAEHwDABV8CgAhfBMANnwFAD18EwBTfA0AY3w4AJ18HwC+fHwAPH0QAE99YgCzfQMAuH1DAP19sACvfhkAyn4dAOl+SQA0f3sAsX+lAFiAhgDggB0A/4AIAAmBBQAQgRQAJoEcAESBpgDsgQ8A/YGUAJOCNgDLgjUAAoNKAE6DJwB3g6gAIYQBACSEIQBHhAsAVIQSAGiEIQCLhNMAYIVVALeFBAC9hSQA44V8AGGGDwByhhAAhIYZAJ+GRgDnhhsABIdJAE+HGQBqhxgAhIcJAI+HOQDKhxYA4oc1ABmIDQAoiAQALogbAEuIcAC9iA0AzIgYAOaIEgD6iCoAJokaAEKJawKvizkA6ouLAHeMJQCejFIA8owGAPqMFAAQjRIAJI0MADKNFgBKjQEATY0IAFiNFwBxjQoAfY0EAIONYQDmjQAA6I06ACSOKgFQj8YAGJAzAE2QFQBkkHQA2pA7ABeRPwBYkQUAX5FdAL6RQwADkhYAG5IMACmSfgCpkhQAv5JlACaTEAA4k5wA1pMzAAuUBgATlEoAX5ROAa+VTQD+lRMAE5YzAEiWRACOlmwA/JYOAAyXAAAOlw8AH5cHACiXAAArlwkANpcBADmXBQBAlxYAWJcxAIuXEwCglxUAt5ccANWXAADXlxoA85cIAP2XMQAwmIAAsphyACaZdACdmU4A7ZkXAAaaVABcmhcAdZocAJOaFgCrmhYAw5osAPGaGgANmwEAEJsRACObBwAsmxwASpsDAE+bBQBWmw0AZZtmAM2bOQAInFIAXJwCAGCcOgCcnMkAZ50FAG6dGgCKnRQAoJ08AN6dAADgnQ0A750hABKeUABknhIAeJ42ALCebAAenzEAUZ8AAFOfCABdn4UA5J8AAOafQwAroCMAUKAEAFagQACYoBUAsKAxAOOgIgAHoQQADaEkADOhCQA+oQQARKEJAE+hPQCOoZ4ALqKMALyiKAHmo7oAoqQgAMSkUQAXpUEAWqW/ABumNgBTphkAbqZXAMemMwH8pwkAB6gMABWoTQBkqCgAjqg6AMqoHwDrqGwAWalEAJ+pEgCzqRIAx6kmAO+pPwAwqioAXKoSAHCqLQCfqkYA56pBACqrEgA+qyYAZqsrAJOrKAC9qwAAv6sdAN6rBwDnqxIA+6sCAP+rBQAGrBMAG6wRAC6sAAAwrA8AQawSAFWsEwBqrAQAcKwTAIWsJwCurBAAwKwMAM6sAADQrAMA1awLAOKsEwD3rAgAAa0oACutCwA5rQgAQ60qAG+tBAB1rQIAea0JAIStJQCrrQgAta0hANitCgDlrQsA8q0fABOuJgA8rhgAV64NAGauHQCFrioAsa4UAMeuGQDirhoA/q4PAA+vBQAWryYAP68TAFSvAwBZrwQAX68AAGGvGgB9rxAAj68DAJSvMADGrxQA3K8uAA2wBwAWsAsAI7AAACWwKABPsBUAZrABAGmwDQB4sCAAm7AIAKWwBwCusAEAsbATAMewIADpsBMA/rADAAOxGQAesQAAILEUADaxAAA4sRUAT7EfAHCxBgB4sRUAj7EXAKixCgC0sQEAt7EqAOOxGwAAsiEAI7ITADiyFgBQsgEAU7IOAGOyEwB4sgQAfrIAAIGyGgCdsgMAorIMALCyAwC1sgEAuLITAM2yFwDmshQA/LIEAAKzAQAFsxQAG7MNACqzCAA0swgAPrMAAEGzAABDswAARbMBAEizGQBjswYAa7MEAHGzCwB+swEAgbMEAIezEwCcsw0Aq7MHALSzCAC+swMAw7MDAMizAwDNswYA1bMUAOuzCQD2swEA+bMAAPuzDAAJtAcAErQAABS0AQAYtAsAJbQEACy0DQA7tBAATbQZAGi0IACKtAQAkLQCAJS0AQCXtAQAnbQAAJ+0BQCmtBMAu7QOAMu0GgDntAkA8rQCAPa0JQAdtQAAH7UHACi1FgBAtQoATbUKAFm1AABbtRgAdbUTAIq1GwCntQMArLUCALC1OQDrtXAAXbYAAF+2KACJtjUAwLYgAOK2OgAetxAAMLcWAEi3AABKt0cAk7cVAKq3KQDVtxIA6bcSAP23AAD/twcACLg0AD64RgCGuDEAubgkAN+4CQDquBQAALkKAAy5DwAduQAAH7kIACu5CAA3uQgAQbkCAEW5AABIuQkAVbkKAGG5AQBkuQcAbrkBAHO5FACJuQIAjbkAAJC5AwCVuQMAmrkAAJy5AgChuQMAqLkHALG5AAC0uQIAuLkHAMO5JQDquQIA8LkXAAm6AAALugkAFroDABu6BQAiugIAKLoIADS6AQA3ugUAProKAEq6AQBNugAAULoJAFy6AQBfugUAZroKAHK6AgB6ugAAfLoUAJK6AgCYugcAoboAAKS6FAC6ugEAvboAAL+6BQDGugEAyboBAMy6BwDVug8A5roAAOi6CADyug0AAbsBAAS7DgAUuxcALbsAADC7BAA2uwIAO7sCAEC7GwBduykAiLsqALS7gwA5vFAAi7wEAJG8JAC3vEIA+7wGAAO9EAAVvQUAHL0SADC9CgA8vTIAcL0KAHy9AQB/vRcAmL0TAK29GwDKvVoAJr4iAEq+DABYvlcCscDiAJXBeAAPwh0ALsInAFfCEABpwgQAb8IJAHrCBwCDwgUAisIRAJ3CEgCxwgQAt8IYANHCEADjwgcA7cIAAPDCKwAdwx8APsMAAEDDSACKwwMAj8MWAKfDCgCzwzMA6MP8AObEcABYxQEAW8UEAGHFDABvxQUAdsUQAIjFDQCXxQ0ApsUYAMDFAADCxSEA5cUAAOfFCgDzxQkA/sUFAAXGBQAMxgcAFcYNACTGKwBRxjIAhcYEAIvGNQDCxmwAMMfSAATIXQBjyAMAaMgVAH/IFwCYyCYAwMgGAMjICwDVyA4A5cggAAfJCAARyRcAKskSAD7JAQBByQoATckXAGbJGgCCyQAAhMkLAJLJFQCpyRMAvskEAMTJWAEeywAAIMsnAEnLAQBMyw8AXcsUAHPLLQCiyw8As8sAALXLAAC4yxoA1MsmAP3LAgABzCsAL8x8AK3MGADHzAMAzMwLANnMCwDnzBcAAM0QABLNGQAtzQoAOc0GAEHNEQBUzQkAX80TAHXNFQCMzQoAmc1JAOXNDADzzScAHM4qAEjOiwDVzgQA284NAOrOdwJj0Q0ActEBAHXRTQDE0QcAzdFQAB/SEAAx0joBbdOhABDUJwA51HMArtSSAELV6gAu1gcAN9YYAFHWIwB21gMAe9YAAH3WAwCC1hsAn9YKAKvWAQCu1gcAt9YBALrWBwDD1gEAxtYJANLWGwDv1gIA89YAAPXWFwAO1wAAENcCABTXFgAs1zoAaNdBAKvXnwBM2AEAT9gGAFfYJwCA2HkA+9idAJrZGgC22QEBudoVANDaOQAL22wAedu8ADfcLABl3CYAjdwcAKvcFgDD3EwAEd1fAHLdTQDB3R8A4t0fAAPenACh3jEA1N4SAOjehQBv3wAAcd9AALPfAQC23xsA098zAAjgEgAc4AIAIOADACXgBQAs4AAAL+ADADTgBAA64AAAPOAKAEjgCQBT4AcAXOAHAGXgHQCE4AAAhuAKAJLgFQCp4A0AuOAkAN7gAwDk4A4A9OAlABvhGQA24RcAT+ECAFPhGABt4QAAb+EbAIzhCwCZ4QUAoOEGAKjhFwDB4QAAw+EBAMbhDgDW4QsA4+EdAALiAwAH4gsAFOIDABniIgA94gEAQOIhAGPiEAB14i0ApOISALjiFADO4gwA3OITAPHiAQD04gYA/OIZABfjAQAa4wIAHuMBACHjBgAp4wQAL+MKADvjDABJ4wwAV+MOAGfjBQBu4wEAceMAAHPjBQB64woAhuMBAInjBwCT4w4Ao+MIAK3jAgCx4wcAuuMDAL/jCgDL4wAAzeMAAM/jBADW4wMA2+MEAOHjBADn4woA8+MCAPfjBQD+4wIAA+QEAArkEAAc5AAAIuQEACjkCgA05AAAOOQDAD7kDABM5AAATuQAAFDkAwBW5AMAW+QEAGHkBQBo5AoAdOQAAHjkAwB+5AwAjOQAAJDkAwCV5AQAnOQGAKTkAQCn5AQAreQDALTkCgDA5AkAy+QEANHkBQDY5AcA4eQHAOrkAgDu5AAA8OQCAPTkBgD85A4ADOUDABHlDQAg5TMAVeUdAHTlBAB65QIAfuUQAJDlFgCo5RAAuuUIAMTlCQDP5SsA/OUdABvmCQAm5hoAQuYSAFbmHAB05ikAn+YSALTmLgDl5g0A9OYxACfnAQAq5wIALuc+AG7nDgF+6BQAlOguAMToqQBv6QwAfekaAJnphwAj6rEA1uoeAPbqTwBH64YAz+sIANnrPgAZ7CYAQewnAGrsBABw7BwAjuwCAJLsCwCf7AIAo+xGAOvsNAAh7QAAI+07AGDtBQBn7S0Alu0GAJ7tLwDP7RYA5+0MAPXtJgAd7iIAQe4BAETuEQBY7g0AZ+4NAHbuLACk7jIA2O4UAO7uJgAW7xIAKu8zAF/vcQDS7yUA+e8RAA3wAgAR8BIAJfAqAFHwBABX8BAAafAfAIrwAQCN8BIAofALAK7wIwDT8A0A4vAHAOvwEgD/8AUABvETABvxCwAp8TsAZvEwAJjxHgC48QAAuvEiAN7xAwDj8Q0A8vEDAPfxBwAA8hsAHfISADHyBAA38hMATfIGAFXyEgBp8j4AqfICAK3yTgD98hsAGvNqAIbzCgCS8xMAp/MAAKrzAwCv8wkAuvMBAL7zAADA8wEAw/MBAMbzBQDN8wAA0fMAANTzAgDY8wEA2/MIAOXzAADn8wYA7/MAAPLzAwD38wMA/PMEAAT0AQAH9A0AGPQGACD0BgAo9AAALPQBAC/0AwA09AcAQPQDAEj0BgBQ9AEAU/QGAFv0CABn9AAAafQEAG/0AQB09AUAe/QEAIH0AQCF9AIAifQCAI/0AACS9AQAmPQCAJz0DgCt9AIAsfQFALj0AAC69AQAwvQEAMj0AgDM9AMA0fQCANb0AQDZ9AQA3/QBAOT0BQDr9AcA9PQHAP70AgAD9QEABvUCAAr1EQAf9Q8AMPUDADf1BAA99QUARPUGAE31AgBR9QYAWfULAGb1AQBq9RIAfvUNAI31BACV9Q8ApvUDAKz1AwCx9QYAufUGAMH1DADP9QkA2vUBAN31EADw9QMA9fUCAPn1AQD99QkACPYAAAr2BwAT9gYAG/YGACP2AAAl9gQAK/YCADD2AAAy9gcAO/YGAET2AABG9gMAS/YAAE32AwBS9gMAV/YIAGL2AABk9hEAd/YDAHz2AAB+9gMAg/YFAIr2BgCS9gEAlvYAAJj2EQCr9gEArvYHALj2AAC69gYAwvYNANH2DADg9gsA7fYBAPD2AgD09gAA9vYBAPn2AQD89goACPcDAA33CgAZ9wEAHPcBAB/3AQAi9wsAL/cCADP3AAA29wwARPcBAEf3AQBK9wwAWPcBAFv3AQBe9wkAafcAAGv3BABy9wkAffcAAH/3DACN9wEAkPcEAJb3CQCh9wAApPcDAKn3DAC39xAAyfcBAMz3EQDf9xIA9PcYAA74AAAQ+AsAHvgGACf4EQA8+AQAQvgOAFL4AABU+BYAbfgBAHD4DgCA+AsAjfgRAKD4AwCm+BEAuvgmAOL4FAD4+BQADvkJABr5CgAm+UIAavmEAPD5CwD9+TYANfoJAEH6LQBw+i0An/oLAKz6PwDt+hoACft4AIP7BgCL+xMBoPwYALr8UwAP/RIAI/0KAC/9BwA4/QkAQ/0kAGn9HgCJ/RsApv2yAFr+AQBd/gIAYf4IAGv+CgB3/gAAef5oAOP+EAD1/mIAWf+CAN3/BwDm/xkA"
+
+func TestRoaring_OrThenAnd(t *testing.T) {
+	bm1 := BitmapOf(bm1Arr...)
+	require.NoError(t, bm1.Validate())
+
+	bm2 := New()
+	_, err := bm2.FromBase64(bm2Bytes)
+	require.NoError(t, err)
+	require.NoError(t, bm2.Validate())
+
+	orBm := Or(bm1, bm2) // this can be FastOr, bm1.Or(bm2)
+	require.NoError(t, orBm.Validate())
+
+	mask := New()
+	mask.AddRange(279_900_001, 280_000_001)
+	mask.Or(orBm) // this can be roaring.And(mask, orBm)
+
+	require.NoError(t, mask.Validate()) // <-- fails here
+}
+
+func TestRoaring_OrThenAnd_RunOptimize(t *testing.T) {
+	bm1 := BitmapOf(bm1Arr...)
+	require.NoError(t, bm1.Validate())
+
+	fmt.Printf("%v\n", bm1)
+	bm2 := New()
+	bm2.FromBase64(bm2Bytes) // this shows that data is not corrupted
+	bm2 = BitmapOf(bm2.ToArray()...)
+	require.NoError(t, bm2.Validate())
+
+	bm2.RunOptimize()
+	require.NoError(t, bm2.Validate())
+
+	orBm := Or(bm1, bm2)
+	require.NoError(t, orBm.Validate())
+
+	mask := New()
+	mask.AddRange(279_900_001, 280_000_001)
+	require.NoError(t, mask.Validate())
+
+	mask.And(orBm)
+
+	require.NoError(t, mask.Validate())
+}
+
 func TestSplitter_BrokeBm(t *testing.T) {
 	bm1 := NewBitmap()
 	bm2 := NewBitmap()

runcontainer.go

@@ -1823,7 +1823,11 @@ func (rc *runContainer16) and(a container) container {
 	}
 	switch c := a.(type) {
 	case *runContainer16:
-		return rc.intersect(c)
+		// Important: there is no reason to believe that the
+		// result of intersecting two run containers is itself
+		// a run container. Hence we convert to efficient container.
+		// We only use run containers when they are efficient.
+		return rc.intersect(c).toEfficientContainer()
 	case *arrayContainer:
 		return rc.andArray(c)
 	case *bitmapContainer:
@@ -1885,7 +1889,11 @@ func (rc *runContainer16) iand(a container) container {
 	}
 	switch c := a.(type) {
 	case *runContainer16:
-		return rc.inplaceIntersect(c)
+		// Important: there is no reason to believe that the
+		// result of intersecting two run containers is itself
+		// a run container. Hence we convert to efficient container.
+		// We only use run containers when they are efficient.
+		return rc.inplaceIntersect(c).toEfficientContainer()
 	case *arrayContainer:
 		// inplace intersection with array is not supported
 		// It is likely not very useful either.
@@ -2198,7 +2206,7 @@ func (rc *runContainer16) ior(a container) container {
 	case *arrayContainer:
 		return rc.iorArray(c)
 	case *bitmapContainer:
-		return rc.iorBitmapContainer(c)
+		return rc.orBitmapContainer(c)
 	}
 	panic("unsupported container type")
 }
@@ -2213,13 +2221,14 @@ func (rc *runContainer16) inplaceUnion(rc2 *runContainer16) container {
 	return rc
 }
 
-func (rc *runContainer16) iorBitmapContainer(bc *bitmapContainer) container {
-	it := bc.getShortIterator()
-	for it.hasNext() {
-		rc.Add(it.next())
-	}
-	return rc
-}
+// Such code should not be used as it will not preserve the container invariants:
+//func (rc *runContainer16) iorBitmapContainer(bc *bitmapContainer) container {
+//	it := bc.getShortIterator()
+//	for it.hasNext() {
+//		rc.Add(it.next())
+//	}
+//	return rc
+//}
 
 func (rc *runContainer16) iorArray(ac *arrayContainer) container {
 	if rc.isEmpty() {

runcontainer_test.go

@@ -1058,10 +1058,6 @@ func TestRle16RandomInplaceUnionAgainstOtherContainers012(t *testing.T) {
 					b = append(b, uint16(r1))
 					mb[r1] = true
 				}
-
-				// showArray16(a, "a")
-				// showArray16(b, "b")
-
 				// hash version of union
 				hashi := make(map[int]bool)
 				for k := range ma {
@@ -1092,21 +1088,21 @@ func TestRle16RandomInplaceUnionAgainstOtherContainers012(t *testing.T) {
 				// vs runContainer
 				rcb := newRunContainer16FromVals(false, b...)
 
-				rcVsBcUnion.ior(bc)
-				rcVsAcUnion.ior(ac)
-				rcVsRcbUnion.ior(rcb)
+				rcVsBcUnionAnswer := rcVsBcUnion.ior(bc)
+				rcVsAcUnionAnswer := rcVsAcUnion.ior(ac)
+				rcVsRcbUnionAnswer := rcVsRcbUnion.ior(rcb)
 
 				for k := range hashi {
-					assert.True(t, rcVsBcUnion.contains(uint16(k)))
+					assert.True(t, rcVsBcUnionAnswer.contains(uint16(k)))
 
-					assert.True(t, rcVsAcUnion.contains(uint16(k)))
+					assert.True(t, rcVsAcUnionAnswer.contains(uint16(k)))
 
-					assert.True(t, rcVsRcbUnion.contains(uint16(k)))
+					assert.True(t, rcVsRcbUnionAnswer.contains(uint16(k)))
 				}
 
-				assert.Equal(t, len(hashi), rcVsBcUnion.getCardinality())
-				assert.Equal(t, len(hashi), rcVsAcUnion.getCardinality())
-				assert.Equal(t, len(hashi), rcVsRcbUnion.getCardinality())
+				assert.Equal(t, len(hashi), rcVsBcUnionAnswer.getCardinality())
+				assert.Equal(t, len(hashi), rcVsAcUnionAnswer.getCardinality())
+				assert.Equal(t, len(hashi), rcVsRcbUnionAnswer.getCardinality())
 			}
 		}