diff --git a/executor.go b/executor.go index 57bc2abca..25f5c074e 100644 --- a/executor.go +++ b/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{}, ""} diff --git a/executor_internal_test.go b/executor_internal_test.go index 86ff6cdde..909f48f7a 100644 --- a/executor_internal_test.go +++ b/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) + } + } +} diff --git a/executor_test.go b/executor_test.go index bc990dff9..0bbf3bde8 100644 --- a/executor_test.go +++ b/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 { diff --git a/pilosa.go b/pilosa.go index 53733d9ce..481c8ff07 100644 --- a/pilosa.go +++ b/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")