Merge pull request #386 from travisturner/bsigroup-edges

Fix edge case bugs in range queries

executor.go

@@ -4845,6 +4845,13 @@ func getCondIntSlice(f *Field, cond *pql.Condition) ([]int64, error) {
 		ret[i] = s
 	}
 
+	// In the case where one (or both) of the predicates is on the
+	// opposite edge, return early to avoid the increment/decrement
+	// logic below and prevent an overflow.
+	if ret[0] == math.MaxInt64 || ret[1] == math.MinInt64 {
+		return ret, nil
+	}
+
 	switch cond.Op {
 	case pql.BTWN_LT_LTE: // a < x <= b
 		ret[0]++

executor_test.go

@@ -2559,6 +2559,93 @@ func TestExecutor_Execute_Row_BSIGroup(t *testing.T) {
 	})
 }
 
+// Ensure a Row(bsiGroup) query can be executed (edge cases).
+func TestExecutor_Execute_Row_BSIGroupEdge(t *testing.T) {
+	c := test.MustRunCluster(t, 1)
+	defer c.Close()
+	hldr := test.Holder{Holder: c[0].Server.Holder()}
+
+	idx, err := hldr.CreateIndex("i", pilosa.IndexOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	t.Run("LT", func(t *testing.T) {
+		if _, err := idx.CreateField("f1", pilosa.OptFieldTypeInt(-2000, 2000)); err != nil {
+			t.Fatal(err)
+		}
+
+		// Set a value at the edge of bitDepth (i.e. 2^n-1; here, n=3).
+		// It must also be the max value in the field; in other words,
+		// set the value to bsiGroup.bitDepthMax().
+		if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `
+			Set(100, f1=7)
+		`}); err != nil {
+			t.Fatal(err)
+		}
+
+		if result, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Row(f1 < 10)`}); err != nil {
+			t.Fatal(err)
+		} else if got, exp := result.Results[0].(*pilosa.Row).Columns(), []uint64{100}; !reflect.DeepEqual(got, exp) {
+			t.Fatalf("unexpected result: got=%v, exp=%v", got, exp)
+		}
+	})
+
+	t.Run("GT", func(t *testing.T) {
+		if _, err := idx.CreateField("f2", pilosa.OptFieldTypeInt(-2000, 2000)); err != nil {
+			t.Fatal(err)
+		}
+
+		// Set a value at the negative edge of bitDepth (i.e. -(2^n-1); here, n=3).
+		// It must also be the min value in the field; in other words,
+		// set the value to bsiGroup.bitDepthMin().
+		if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `
+		Set(200, f2=-7)
+	`}); err != nil {
+			t.Fatal(err)
+		}
+
+		if result, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Row(f2 > -10)`}); err != nil {
+			t.Fatal(err)
+		} else if got, exp := result.Results[0].(*pilosa.Row).Columns(), []uint64{200}; !reflect.DeepEqual(got, exp) {
+			t.Fatalf("unexpected result: got=%v, exp=%v", got, exp)
+		}
+	})
+
+	t.Run("BTWN_LT_LT", func(t *testing.T) {
+		if _, err := idx.CreateField("f3", pilosa.OptFieldTypeInt(-2000, 2000)); err != nil {
+			t.Fatal(err)
+		}
+
+		// Set a value anywhere in range.
+		if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `
+		Set(300, f3=10)
+	`}); err != nil {
+			t.Fatal(err)
+		}
+
+		// Query INT_MAX < x < INT_MIN. Because that's an invalid range, we should
+		// get back an empty result set.
+		tests := []struct {
+			predA int64
+			predB int64
+		}{
+			{math.MaxInt64, math.MinInt64},
+			{math.MaxInt64, 1000},
+			{-1000, math.MinInt64},
+		}
+
+		for i, test := range tests {
+			pql := fmt.Sprintf("Row(%d < f3 < %d)", test.predA, test.predB)
+			if result, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: pql}); err != nil {
+				t.Fatal(err)
+			} else if got, exp := result.Results[0].(*pilosa.Row).Columns(), []uint64{}; !reflect.DeepEqual(got, exp) {
+				t.Fatalf("test %d unexpected result: got=%v, exp=%v", i, got, exp)
+			}
+		}
+	})
+}
+
 // Ensure a Range(bsiGroup) query can be executed. (Deprecated)
 func TestExecutor_Execute_Range_BSIGroup_Deprecated(t *testing.T) {
 	c := test.MustRunCluster(t, 1)

field.go

@@ -2189,11 +2189,6 @@ type bsiGroup struct {
 // ex: Field.Min = 0, Field.Max = 1023
 // baseValue(LT, 2000) returns 1023, which will perform "LT 1023" and effectively
 // exclude any columns with value = 1023.
-// Note that in this case (because the range uses the full BitDepth 0 to 1023),
-// we can't simply return 1024.
-// In order to make this work, we effectively need to change the operator to LTE.
-// Executor.executeBSIGroupRangeShard() takes this into account and returns
-// `frag.FieldNotNull(bsig.BitDepth())` in such instances.
 func (b *bsiGroup) baseValue(op pql.Token, value int64) (baseValue int64, outOfRange bool) {
 	min, max := b.bitDepthMin(), b.bitDepthMax()
 
@@ -2202,6 +2197,10 @@ func (b *bsiGroup) baseValue(op pql.Token, value int64) (baseValue int64, outOfR
 			return baseValue, true
 		} else if value < min {
 			baseValue = int64(min - b.Base)
+			// Address edge case noted in comments above.
+			if op == pql.GT {
+				baseValue--
+			}
 		} else {
 			baseValue = int64(value - b.Base)
 		}
@@ -2210,6 +2209,10 @@ func (b *bsiGroup) baseValue(op pql.Token, value int64) (baseValue int64, outOfR
 			return baseValue, true
 		} else if value > max {
 			baseValue = int64(max - b.Base)
+			// Address edge case noted in comments above.
+			if op == pql.LT {
+				baseValue++
+			}
 		} else {
 			baseValue = int64(value - b.Base)
 		}

field_internal_test.go

@@ -69,12 +69,12 @@ func TestBSIGroup_BaseValue(t *testing.T) {
 			{b0, pql.LT, 5, 105, false},
 			{b0, pql.LT, -8, 92, false},
 			{b0, pql.LT, -108, -8, false},
-			{b0, pql.LT, 1005, 1023, false},
+			{b0, pql.LT, 1005, 1024, false},
 			{b0, pql.LT, 0, 100, false},
 
 			{b1, pql.LT, 5, 5, false},
 			{b1, pql.LT, -8, -8, false},
-			{b1, pql.LT, 1005, 255, false},
+			{b1, pql.LT, 1005, 256, false},
 			{b1, pql.LT, 0, 0, false},
 
 			{b2, pql.LT, 5, -95, false},
@@ -92,7 +92,7 @@ func TestBSIGroup_BaseValue(t *testing.T) {
 			{b1, pql.GT, -8, -8, false},
 			{b1, pql.GT, 1005, 0, true},
 			{b1, pql.GT, 0, 0, false},
-			{b1, pql.GT, -300, -255, false},
+			{b1, pql.GT, -300, -256, false},
 
 			{b2, pql.GT, 5, -95, false},
 			{b2, pql.GT, -8, -108, false},

fragment.go

@@ -1242,7 +1242,6 @@ func (f *fragment) rangeLTUnsigned(filter *Row, bitDepth uint, predicate uint64,
 	// if the predicate is larger than all representable numbers given
 	// our bitDepth... then just return everything.
 	if msb(predicate) > bitDepth {
-
 		return filter, nil
 	}