From f921c5ded076685399b587ff1bb93e466f589962 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kuba=20Podg=C3=B3rski?= Date: Tue, 30 Jun 2020 21:07:54 +0200 Subject: [PATCH 1/4] Add test for Rows on bool --- executor.go | 27 +++++++++---------- executor_internal_test.go | 56 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/executor.go b/executor.go index 57bc2abca..d89823b56 100644 --- a/executor.go +++ b/executor.go @@ -724,7 +724,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 +4040,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]) { diff --git a/executor_internal_test.go b/executor_internal_test.go index 86ff6cdde..20c4f19c4 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("seeting 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: From 58964947d48d1c7fe4fe9c38bce3e2104e21984f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kuba=20Podg=C3=B3rski?= Date: Wed, 1 Jul 2020 10:17:30 +0200 Subject: [PATCH 2/4] Update executor_internal_test.go Co-authored-by: Travis Turner --- executor_internal_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/executor_internal_test.go b/executor_internal_test.go index 20c4f19c4..68b0f1f4e 100644 --- a/executor_internal_test.go +++ b/executor_internal_test.go @@ -162,7 +162,7 @@ func TestExecutor_TranslateRowsOnBool(t *testing.T) { _, err2 := fb.SetBit(2, 2, nil) _, err3 := fb.SetBit(3, 3, nil) if err1 != nil || err2 != nil || err3 != nil { - t.Fatalf("seeting bit %v, %v, %v", err1, err2, err3) + t.Fatalf("setting bit %v, %v, %v", err1, err2, err3) } tests := []struct { From 5bbb3e2065efc3965da05a76b1d2a3078bcb6165 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kuba=20Podg=C3=B3rski?= Date: Wed, 1 Jul 2020 11:36:42 +0200 Subject: [PATCH 3/4] FieldValue - check if column arg exists --- executor.go | 12 ++++++++---- executor_test.go | 11 ++++++----- pilosa.go | 7 ++++--- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/executor.go b/executor.go index 57bc2abca..25dff738b 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 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") From 4bed1df1016fd40c6b6ded960ee25793e84a0193 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kuba=20Podg=C3=B3rski?= Date: Wed, 1 Jul 2020 14:08:20 +0200 Subject: [PATCH 4/4] Address the overflow issue with values outside the int64 range --- executor.go | 47 ++++++++++++++++++++++++--- executor_internal_test.go | 68 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 5 deletions(-) diff --git a/executor.go b/executor.go index c3adf0278..25f5c074e 100644 --- a/executor.go +++ b/executor.go @@ -4486,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 @@ -4512,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 68b0f1f4e..909f48f7a 100644 --- a/executor_internal_test.go +++ b/executor_internal_test.go @@ -495,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) + } + } +}