Merge branch 'master' into roaring-cleanup-3

executor.go

@@ -699,7 +699,12 @@ func (e *executor) executeIncludesColumnCall(ctx context.Context, index string,
 func (e *executor) executeFieldValueCall(ctx context.Context, index string, c *pql.Call, shards []uint64, opt *execOptions) (ValCount, error) {
 	fieldName, ok := c.Args["field"].(string)
 	if !ok || fieldName == "" {
-		return ValCount{}, errors.New("FieldValue(): field required")
+		return ValCount{}, ErrFieldRequired
+	}
+
+	colKey, ok := c.Args["column"]
+	if !ok || colKey == "" {
+		return ValCount{}, ErrColumnRequired
 	}
 
 	// Fetch index.
@@ -715,8 +720,8 @@ func (e *executor) executeFieldValueCall(ctx context.Context, index string, c *p
 	}
 
 	var colID uint64
-	if colKey, ok := c.Args["column"].(string); ok && idx.Keys() {
-		id, err := e.Cluster.translateIndexKey(ctx, index, colKey)
+	if key, ok := colKey.(string); ok && idx.Keys() {
+		id, err := e.Cluster.translateIndexKey(ctx, index, key)
 		if err != nil {
 			return ValCount{}, errors.Wrap(err, "getting column id")
 		}
@@ -724,7 +729,6 @@ func (e *executor) executeFieldValueCall(ctx context.Context, index string, c *p
 	} else {
 		id, ok, err := c.UintArg("column")
 		if !ok || err != nil {
-			// TODO: this error is getting swallowed somewhere (via curl)
 			return ValCount{}, errors.Wrap(err, "getting column argument")
 		}
 		colID = id
@@ -4041,20 +4045,20 @@ func (e *executor) translateCall(ctx context.Context, indexName string, c *pql.C
 		// are only two possible values. Instead, they are handled
 		// directly.
 		if field.Type() == FieldTypeBool {
-			// TODO: This code block doesn't make sense for a `Rows()`
-			// queries on a `bool` field. Need to review this better,
-			// include it in tests, and probably back-port it to Pilosa.
-			if c.Name != "Rows" {
-				boolVal, err := callArgBool(c, rowKey)
-				if err != nil {
-					return errors.Wrap(err, "getting bool key")
-				}
-				rowID := falseRowID
-				if boolVal {
-					rowID = trueRowID
-				}
-				c.Args[rowKey] = rowID
+			if c.Name == "Rows" {
+				// TranslateInfo for Rows returns "previous" as rowKey,
+				// so for bool fields we would get "missing bool argument" error
+				return nil
 			}
+			boolVal, err := callArgBool(c, rowKey)
+			if err != nil {
+				return errors.Wrapf(err, "getting bool key (%+v)", rowKey)
+			}
+			rowID := falseRowID
+			if boolVal {
+				rowID = trueRowID
+			}
+			c.Args[rowKey] = rowID
 		} else if field.Keys() {
 			foreignIndexName := field.ForeignIndex()
 			if c.Args[rowKey] != nil && isCondition(c.Args[rowKey]) {
@@ -4482,25 +4486,37 @@ func (s SignedRow) ToTable() (*pb.TableResponse, error) {
 
 // ToRows implements the ToRowser interface.
 func (s SignedRow) ToRows(callback func(*pb.RowResponse) error) error {
-	// TODO: address the overflow issue with values outside the int64 range
+
 	ci := []*pb.ColumnInfo{{Name: s.Field(), Datatype: "int64"}}
 	negs := s.Neg.Columns()
 	for i := len(negs) - 1; i >= 0; i-- {
+		val, err := toNegInt64(negs[i])
+		if err != nil {
+			return errors.Wrap(err, "converting uint64 to int64 (negative)")
+		}
+
 		if err := callback(&pb.RowResponse{
 			Headers: ci,
 			Columns: []*pb.ColumnResponse{
-				&pb.ColumnResponse{ColumnVal: &pb.ColumnResponse_Int64Val{Int64Val: -1 * int64(negs[i])}},
-			}}); err != nil {
+				&pb.ColumnResponse{ColumnVal: &pb.ColumnResponse_Int64Val{Int64Val: val}},
+			},
+		}); err != nil {
 			return errors.Wrap(err, "calling callback")
 		}
 		ci = nil
 	}
 	for _, id := range s.Pos.Columns() {
+		val, err := toInt64(id)
+		if err != nil {
+			return errors.Wrap(err, "converting uint64 to int64 (positive)")
+		}
+
 		if err := callback(&pb.RowResponse{
 			Headers: ci,
 			Columns: []*pb.ColumnResponse{
-				&pb.ColumnResponse{ColumnVal: &pb.ColumnResponse_Int64Val{Int64Val: int64(id)}},
-			}}); err != nil {
+				&pb.ColumnResponse{ColumnVal: &pb.ColumnResponse_Int64Val{Int64Val: val}},
+			},
+		}); err != nil {
 			return errors.Wrap(err, "calling callback")
 		}
 		ci = nil
@@ -4508,6 +4524,31 @@ func (s SignedRow) ToRows(callback func(*pb.RowResponse) error) error {
 	return nil
 }
 
+func toNegInt64(n uint64) (int64, error) {
+	const absMinInt64 = uint64(1 << 63)
+
+	if n > absMinInt64 {
+		return 0, errors.Errorf("value %d overflows int64", n)
+	}
+
+	if n == absMinInt64 {
+		return int64(-1 << 63), nil
+	}
+
+	// n < 1 << 63
+	return -int64(n), nil
+}
+
+func toInt64(n uint64) (int64, error) {
+	const maxInt64 = uint64(1<<63) - 1
+
+	if n > maxInt64 {
+		return 0, errors.Errorf("value %d overflows int64", n)
+	}
+
+	return int64(n), nil
+}
+
 func (sr *SignedRow) union(other SignedRow) SignedRow {
 	ret := SignedRow{&Row{}, &Row{}, ""}
 

executor_internal_test.go

@@ -133,6 +133,62 @@ func TestExecutor_TranslateGroupByCall(t *testing.T) {
 	}
 }
 
+func TestExecutor_TranslateRowsOnBool(t *testing.T) {
+	holder := NewHolder(DefaultPartitionN)
+	defer holder.Close()
+
+	e := &executor{
+		Holder:  holder,
+		Cluster: NewTestCluster(1),
+	}
+	e.Holder.Path, _ = ioutil.TempDir(*TempDir, "")
+	err := e.Holder.Open()
+	if err != nil {
+		t.Fatalf("opening holder: %v", err)
+	}
+
+	idx, err := e.Holder.CreateIndex("i", IndexOptions{})
+	if err != nil {
+		t.Fatalf("creating index: %v", err)
+	}
+
+	fb, errb := idx.CreateField("b", OptFieldTypeBool())
+	_, errbk := idx.CreateField("bk", OptFieldTypeBool(), OptFieldKeys())
+	if errb != nil || errbk != nil {
+		t.Fatalf("creating fields %v, %v", errb, errbk)
+	}
+
+	_, err1 := fb.SetBit(1, 1, nil)
+	_, err2 := fb.SetBit(2, 2, nil)
+	_, err3 := fb.SetBit(3, 3, nil)
+	if err1 != nil || err2 != nil || err3 != nil {
+		t.Fatalf("setting bit %v, %v, %v", err1, err2, err3)
+	}
+
+	tests := []struct {
+		pql string
+	}{
+		{pql: "Rows(b)"},
+		{pql: "GroupBy(Rows(b))"},
+		{pql: "Set(4, b=true)"},
+	}
+
+	for _, test := range tests {
+		t.Run(test.pql, func(t *testing.T) {
+			query, err := pql.ParseString(test.pql)
+			if err != nil {
+				t.Fatalf("parsing query: %v", err)
+			}
+
+			c := query.Calls[0]
+			err = e.translateCall(context.Background(), "i", c, make(map[string]map[string]uint64))
+			if err != nil {
+				t.Fatalf("translating call: %v", err)
+			}
+		})
+	}
+}
+
 func isInt(a interface{}) bool {
 	switch a.(type) {
 	case int, int64, uint, uint64:
@@ -439,3 +495,71 @@ func TestValCountComparisons(t *testing.T) {
 		})
 	}
 }
+
+func TestToNegInt64(t *testing.T) {
+	tests := []struct {
+		u64      uint64
+		i64      int64
+		overflow bool
+	}{
+		{
+			u64: uint64(1 << 63),
+			i64: int64(-1 << 63),
+		},
+		{
+			u64: uint64(1<<63) - 1,
+			i64: int64(-1<<63) + 1,
+		},
+		{
+			u64:      uint64(1<<63) + 1,
+			overflow: true,
+		},
+	}
+
+	for _, tc := range tests {
+		val, err := toNegInt64(tc.u64)
+		if err != nil && !tc.overflow {
+			t.Fatalf("error: %+v, expected: %+v", err, tc)
+		}
+
+		if val != tc.i64 {
+			t.Fatalf("Expected: %+v, Got: %+v", tc.i64, val)
+		}
+	}
+}
+
+func TestToInt64(t *testing.T) {
+	tests := []struct {
+		u64      uint64
+		i64      int64
+		overflow bool
+	}{
+		{
+			u64: uint64(1<<63) - 1,
+			i64: 1<<63 - 1,
+		},
+		{
+			u64: uint64(0),
+			i64: 0,
+		},
+		{
+			u64:      uint64(1 << 63),
+			overflow: true,
+		},
+		{
+			u64:      1<<64 - 1,
+			overflow: true,
+		},
+	}
+
+	for _, tc := range tests {
+		val, err := toInt64(tc.u64)
+		if err != nil && !tc.overflow {
+			t.Fatalf("error: %+v, expected: %+v", err, tc)
+		}
+
+		if val != tc.i64 {
+			t.Fatalf("Expected: %+v, Got: %+v", tc.i64, val)
+		}
+	}
+}

executor_test.go

@@ -3522,7 +3522,6 @@ func TestExecutor_Execute_Not(t *testing.T) {
 func TestExecutor_Execute_FieldValue(t *testing.T) {
 	c := test.MustRunCluster(t, 2)
 	defer c.Close()
-	//hldr := test.Holder{Holder: c[0].Server.Holder()}
 
 	node0 := c[0]
 	node1 := c[1]
@@ -3535,8 +3534,8 @@ func TestExecutor_Execute_FieldValue(t *testing.T) {
 			Set(1, f=3)
 			Set(2, f=-4)
 			Set(` + strconv.Itoa(ShardWidth+1) + `, f=3)
-            Set(1, dec=12.985)
-            Set(2, dec=-4.234)
+			Set(1, dec=12.985)
+			Set(2, dec=-4.234)
 		`}); err != nil {
 		t.Fatal(err)
 	}
@@ -3548,8 +3547,8 @@ func TestExecutor_Execute_FieldValue(t *testing.T) {
 	if _, err := node0.API.Query(context.Background(), &pilosa.QueryRequest{Index: "ik", Query: `
 			Set("one", f=3)
 			Set("two", f=-4)
-            Set("one", dec=12.985)
-            Set("two", dec=-4.234)
+			Set("one", dec=12.985)
+			Set("two", dec=-4.234)
 		`}); err != nil {
 		t.Fatal(err)
 	}
@@ -3577,6 +3576,8 @@ func TestExecutor_Execute_FieldValue(t *testing.T) {
 
 		// Errors
 		{index: "i", qry: "FieldValue()", expErr: pilosa.ErrFieldRequired.Error()},
+		{index: "i", qry: "FieldValue(field=dec)", expErr: pilosa.ErrColumnRequired.Error()},
+		{index: "ik", qry: "FieldValue(field=f)", expErr: pilosa.ErrColumnRequired.Error()},
 	}
 	for n, node := range []*test.Command{node0, node1} {
 		for i, test := range tests {

pilosa.go

@@ -33,9 +33,10 @@ var (
 	ErrForeignIndexNotFound = errors.New("foreign index not found")
 
 	// ErrFieldRequired is returned when no field is specified.
-	ErrFieldRequired = errors.New("field required")
-	ErrFieldExists   = errors.New("field already exists")
-	ErrFieldNotFound = errors.New("field not found")
+	ErrFieldRequired  = errors.New("field required")
+	ErrColumnRequired = errors.New("column required")
+	ErrFieldExists    = errors.New("field already exists")
+	ErrFieldNotFound  = errors.New("field not found")
 
 	ErrBSIGroupNotFound         = errors.New("bsigroup not found")
 	ErrBSIGroupExists           = errors.New("bsigroup already exists")