Merge pull request #595 from jaddr2line/fix-between-common-bits-backport

Fix BSI range queries with nonzero common upper bits and oversized BSI queries (v2.1.x backport)

fragment.go

@@ -1230,12 +1230,18 @@ func (f *fragment) rangeEQ(bitDepth uint, predicate int64) (*Row, error) {
 	// Start with set of columns with values set.
 	b := f.row(bsiExistsBit)
 
-	// Filter to only positive/negative numbers.
 	upredicate := absInt64(predicate)
+	if uint(bits.Len64(upredicate)) > bitDepth {
+		// Predicate is out of range.
+		return NewRow(), nil
+	}
+
+	// Filter to only positive/negative numbers.
+	r := f.row(bsiSignBit)
 	if predicate < 0 {
-		b = b.Intersect(f.row(bsiSignBit)) // only negatives
+		b = b.Intersect(r) // only negatives
 	} else {
-		b = b.Difference(f.row(bsiSignBit)) // only positives
+		b = b.Difference(r) // only positives
 	}
 
 	// Filter any bits that don't match the current bit value.
@@ -1308,18 +1314,10 @@ func (f *fragment) rangeLT(bitDepth uint, predicate int64, allowEquality bool) (
 	}
 }
 
-// msb gives the 1-indexed position (counting from lsb) of the most
-// significant bit. E.G. for 1 it would return 1, for 2 2, for 3 2,
-// for 4 3, for 8 4, etc.
-func msb(x uint64) uint {
-	lz := bits.LeadingZeros64(x)
-	return 64 - uint(lz)
-}
-
 // rangeLTUnsigned returns all bits LT/LTE the predicate without considering the sign bit.
 func (f *fragment) rangeLTUnsigned(filter *Row, bitDepth uint, predicate uint64, allowEquality bool) (*Row, error) {
 	switch {
-	case msb(predicate) > bitDepth:
+	case uint(bits.Len64(predicate)) > bitDepth:
 		fallthrough
 	case predicate == (1<<bitDepth)-1 && allowEquality:
 		// This query matches all possible values.
@@ -1395,6 +1393,7 @@ func (f *fragment) rangeGT(bitDepth uint, predicate int64, allowEquality bool) (
 }
 
 func (f *fragment) rangeGTUnsigned(filter *Row, bitDepth uint, predicate uint64, allowEquality bool) (*Row, error) {
+prep:
 	switch {
 	case predicate == 0 && allowEquality:
 		// This query matches all possible values.
@@ -1407,8 +1406,13 @@ func (f *fragment) rangeGTUnsigned(filter *Row, bitDepth uint, predicate uint64,
 			matches = matches.Union(filter.Intersect(row))
 		}
 		return matches, nil
+	case !allowEquality && uint(bits.Len64(predicate)) > bitDepth:
+		// The predicate is bigger than the BSI width, so nothing can be bigger.
+		return NewRow(), nil
 	case allowEquality:
 		predicate--
+		allowEquality = false
+		goto prep
 	}
 
 	// Compare intermediate bits.
@@ -1479,9 +1483,9 @@ func (f *fragment) rangeBetweenUnsigned(filter *Row, bitDepth uint, predicateMin
 	}
 
 	// Compare any upper bits which are equal.
-	firstDiff := int(msb(predicateMax^predicateMin)) - 1
+	diffLen := bits.Len64(predicateMax ^ predicateMin)
 	remaining := filter
-	for i := int(bitDepth - 1); i > firstDiff; i-- {
+	for i := int(bitDepth - 1); i >= diffLen; i-- {
 		row := f.row(uint64(bsiOffsetBit + i))
 		switch (predicateMin >> uint(i)) & 1 {
 		case 1:
@@ -1491,12 +1495,17 @@ func (f *fragment) rangeBetweenUnsigned(filter *Row, bitDepth uint, predicateMin
 		}
 	}
 
+	// Clear the bits we just compared.
+	equalMask := (^uint64(0)) << uint(diffLen)
+	predicateMin &^= equalMask
+	predicateMax &^= equalMask
+
 	var err error
-	remaining, err = f.rangeGTUnsigned(remaining, uint(firstDiff+1), predicateMin, true)
+	remaining, err = f.rangeGTUnsigned(remaining, uint(diffLen), predicateMin, true)
 	if err != nil {
 		return nil, err
 	}
-	remaining, err = f.rangeLTUnsigned(remaining, uint(firstDiff+1), predicateMax, true)
+	remaining, err = f.rangeLTUnsigned(remaining, uint(diffLen), predicateMax, true)
 	if err != nil {
 		return nil, err
 	}

fragment_internal_test.go

@@ -538,6 +538,30 @@ func TestFragment_Range(t *testing.T) {
 		}
 	})
 
+	t.Run("EQOversizeRegression", func(t *testing.T) {
+		f := mustOpenFragment("i", "f", viewStandard, 0, "")
+		defer f.Clean(t)
+
+		// Set values.
+		if _, err := f.setValue(1000, 1, 0); err != nil {
+			t.Fatal(err)
+		} else if _, err := f.setValue(2000, 1, 1); err != nil {
+			t.Fatal(err)
+		}
+
+		// Query for equality.
+		if b, err := f.rangeOp(pql.EQ, 1, 3); err != nil {
+			t.Fatal(err)
+		} else if !reflect.DeepEqual(b.Columns(), []uint64{}) {
+			t.Fatalf("unexpected columns: %+v", b.Columns())
+		}
+		if b, err := f.rangeOp(pql.EQ, 1, 4); err != nil {
+			t.Fatal(err)
+		} else if !reflect.DeepEqual(b.Columns(), []uint64{}) {
+			t.Fatalf("unexpected columns: %+v", b.Columns())
+		}
+	})
+
 	t.Run("NEQ", func(t *testing.T) {
 		f := mustOpenFragment("i", "f", viewStandard, 0, "")
 		defer f.Clean(t)
@@ -706,6 +730,23 @@ func TestFragment_Range(t *testing.T) {
 		}
 	})
 
+	t.Run("GTOversizeRegression", func(t *testing.T) {
+		f := mustOpenFragment("i", "f", viewStandard, 0, "")
+		defer f.Clean(t)
+
+		if _, err := f.setValue(1, 2, 0); err != nil {
+			t.Fatal(err)
+		} else if _, err := f.setValue(2, 2, 1); err != nil {
+			t.Fatal(err)
+		}
+
+		if b, err := f.rangeGTUnsigned(NewRow(1, 2), 2, 4, false); err != nil {
+			t.Fatal(err)
+		} else if !reflect.DeepEqual(b.Columns(), []uint64{}) {
+			t.Fatalf("unepxected coulmns: %+v", b.Columns())
+		}
+	})
+
 	t.Run("BETWEEN", func(t *testing.T) {
 		f := mustOpenFragment("i", "f", viewStandard, 0, "")
 		defer f.Clean(t)
@@ -753,6 +794,23 @@ func TestFragment_Range(t *testing.T) {
 			t.Fatalf("unexpected columns: %+v", b.Columns())
 		}
 	})
+
+	t.Run("BetweenCommonBitsRegression", func(t *testing.T) {
+		f := mustOpenFragment("i", "f", viewStandard, 0, "")
+		defer f.Clean(t)
+
+		if _, err := f.setValue(1, 64, 0xf0); err != nil {
+			t.Fatal(err)
+		} else if _, err := f.setValue(2, 64, 0xf1); err != nil {
+			t.Fatal(err)
+		}
+
+		if b, err := f.rangeBetweenUnsigned(NewRow(1, 2), 64, 0xf0, 0xf1); err != nil {
+			t.Fatal(err)
+		} else if !reflect.DeepEqual(b.Columns(), []uint64{1, 2}) {
+			t.Fatalf("unepxected coulmns: %+v", b.Columns())
+		}
+	})
 }
 
 // benchmarkSetValues is a helper function to explore, very roughly, the cost