diff --git a/executor.go b/executor.go index 97f42acf1..c8ea9e544 100644 --- a/executor.go +++ b/executor.go @@ -1109,17 +1109,6 @@ func (e *Executor) executeSetBitView(ctx context.Context, index string, c *pql.C // executeSetFieldValue executes a SetFieldValue() call. func (e *Executor) executeSetFieldValue(ctx context.Context, index string, c *pql.Call, opt *ExecOptions) error { - frameName, ok := c.Args["frame"].(string) - if !ok { - return errors.New("SetFieldValue() frame required") - } - - // Retrieve frame. - frame := e.Holder.Frame(index, frameName) - if frame == nil { - return ErrFrameNotFound - } - // Parse labels. columnID, ok, err := c.UintArg(columnLabel) if err != nil { @@ -1130,23 +1119,28 @@ func (e *Executor) executeSetFieldValue(ctx context.Context, index string, c *pq // Copy args and remove reserved fields. args := pql.CopyArgs(c.Args) - delete(args, "frame") // While frame could technically work as a ColumnAttr argument, we are treating it as a reserved word primarily to avoid confusion. // Also, if we ever need to make ColumnAttrs frame-specific, then having this reserved word prevents backward incompatibility. delete(args, columnLabel) // Set values. for name, value := range args { + // Retrieve frame. + frame := e.Holder.Frame(index, name) + if frame == nil { + return ErrFrameNotFound + } + switch value := value.(type) { case int64: - if _, err := frame.SetFieldValue(columnID, name, value); err != nil { + if _, err := frame.SetValue(columnID, value); err != nil { return err } default: return ErrInvalidFieldValueType } + frame.Stats.Count("SetFieldValue", 1, 1.0) } - frame.Stats.Count("SetFieldValue", 1, 1.0) // Do not forward call if this is already being forwarded. if opt.Remote { diff --git a/executor_test.go b/executor_test.go index fbce4d52d..0b70a9e16 100644 --- a/executor_test.go +++ b/executor_test.go @@ -283,9 +283,9 @@ func TestExecutor_Execute_SetFieldValue(t *testing.T) { // Set field values. e := test.NewExecutor(hldr.Holder, test.NewCluster(1)) - if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(col=10, frame=f, f=25)`), nil, nil); err != nil { + if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(col=10, f=25)`), nil, nil); err != nil { t.Fatal(err) - } else if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(col=100, frame=f, f=10)`), nil, nil); err != nil { + } else if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(col=100, f=10)`), nil, nil); err != nil { t.Fatal(err) } @@ -319,30 +319,23 @@ func TestExecutor_Execute_SetFieldValue(t *testing.T) { t.Fatal(err) } - t.Run("ErrFrameRequired", func(t *testing.T) { - e := test.NewExecutor(hldr.Holder, test.NewCluster(1)) - if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(col=10, f=100)`), nil, nil); err == nil || err.Error() != `SetFieldValue() frame required` { - t.Fatalf("unexpected error: %s", err) - } - }) - t.Run("ErrColumnFieldRequired", func(t *testing.T) { e := test.NewExecutor(hldr.Holder, test.NewCluster(1)) - if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(invalid_column_name=10, frame=f, f=100)`), nil, nil); err == nil || err.Error() != `SetFieldValue() column field 'col' required` { + if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(invalid_column_name=10, f=100)`), nil, nil); err == nil || err.Error() != `SetFieldValue() column field 'col' required` { t.Fatalf("unexpected error: %s", err) } }) t.Run("ErrColumnFieldValue", func(t *testing.T) { e := test.NewExecutor(hldr.Holder, test.NewCluster(1)) - if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(invalid_column_name="bad_column", frame=f, f=100)`), nil, nil); err == nil || err.Error() != `SetFieldValue() column field 'col' required` { + if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(invalid_column_name="bad_column", f=100)`), nil, nil); err == nil || err.Error() != `SetFieldValue() column field 'col' required` { t.Fatalf("unexpected error: %s", err) } }) t.Run("ErrInvalidFieldValueType", func(t *testing.T) { e := test.NewExecutor(hldr.Holder, test.NewCluster(1)) - if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(col=10, frame=f, f="hello")`), nil, nil); err == nil || err.Error() != `invalid field value type` { + if _, err := e.Execute(context.Background(), "i", test.MustParse(`SetFieldValue(col=10, f="hello")`), nil, nil); err == nil || err.Error() != `invalid field value type` { t.Fatalf("unexpected error: %s", err) } }) @@ -598,14 +591,14 @@ func TestExecutor_Execute_MinMax(t *testing.T) { SetBit(frame=x, row=1, col=1) SetBit(frame=x, row=2, col=`+strconv.Itoa(SliceWidth+2)+`) - SetFieldValue(frame=f, f=20, col=0) - SetFieldValue(frame=f, f=-5, col=1) - SetFieldValue(frame=f, f=-5, col=2) - SetFieldValue(frame=f, f=10, col=3) - SetFieldValue(frame=f, f=30, col=`+strconv.Itoa(SliceWidth)+`) - SetFieldValue(frame=f, f=40, col=`+strconv.Itoa(SliceWidth+2)+`) - SetFieldValue(frame=f, f=50, col=`+strconv.Itoa((5*SliceWidth)+100)+`) - SetFieldValue(frame=f, f=60, col=`+strconv.Itoa(SliceWidth+1)+`) + SetFieldValue(f=20, col=0) + SetFieldValue(f=-5, col=1) + SetFieldValue(f=-5, col=2) + SetFieldValue(f=10, col=3) + SetFieldValue(f=30, col=`+strconv.Itoa(SliceWidth)+`) + SetFieldValue(f=40, col=`+strconv.Itoa(SliceWidth+2)+`) + SetFieldValue(f=50, col=`+strconv.Itoa((5*SliceWidth)+100)+`) + SetFieldValue(f=60, col=`+strconv.Itoa(SliceWidth+1)+`) `), nil, nil); err != nil { t.Fatal(err) } @@ -706,13 +699,13 @@ func TestExecutor_Execute_Sum(t *testing.T) { SetBit(frame=x, row=0, col=0) SetBit(frame=x, row=0, col=`+strconv.Itoa(SliceWidth+1)+`) - SetFieldValue(frame=foo, foo=20, col=0) - SetFieldValue(frame=bar, bar=2000, col=0) - SetFieldValue(frame=foo, foo=30, col=`+strconv.Itoa(SliceWidth)+`) - SetFieldValue(frame=foo, foo=40, col=`+strconv.Itoa(SliceWidth+2)+`) - SetFieldValue(frame=foo, foo=50, col=`+strconv.Itoa((5*SliceWidth)+100)+`) - SetFieldValue(frame=foo, foo=60, col=`+strconv.Itoa(SliceWidth+1)+`) - SetFieldValue(frame=other, other=1000, col=0) + SetFieldValue(foo=20, col=0) + SetFieldValue(bar=2000, col=0) + SetFieldValue(foo=30, col=`+strconv.Itoa(SliceWidth)+`) + SetFieldValue(foo=40, col=`+strconv.Itoa(SliceWidth+2)+`) + SetFieldValue(foo=50, col=`+strconv.Itoa((5*SliceWidth)+100)+`) + SetFieldValue(foo=60, col=`+strconv.Itoa(SliceWidth+1)+`) + SetFieldValue(other=1000, col=0) `), nil, nil); err != nil { t.Fatal(err) } @@ -827,15 +820,15 @@ func TestExecutor_Execute_FieldRange(t *testing.T) { SetBit(frame=f, row=0, col=0) SetBit(frame=f, row=0, col=`+strconv.Itoa(SliceWidth+1)+`) - SetFieldValue(frame=foo, foo=20, col=50) - SetFieldValue(frame=bar, bar=2000, col=50) - SetFieldValue(frame=foo, foo=30, col=`+strconv.Itoa(SliceWidth)+`) - SetFieldValue(frame=foo, foo=10, col=`+strconv.Itoa(SliceWidth+2)+`) - SetFieldValue(frame=foo, foo=20, col=`+strconv.Itoa((5*SliceWidth)+100)+`) - SetFieldValue(frame=foo, foo=60, col=`+strconv.Itoa(SliceWidth+1)+`) - SetFieldValue(frame=other, other=1000, col=0) - SetFieldValue(frame=edge, edge=100, col=0) - SetFieldValue(frame=edge, edge=-100, col=1) + SetFieldValue(foo=20, col=50) + SetFieldValue(bar=2000, col=50) + SetFieldValue(foo=30, col=`+strconv.Itoa(SliceWidth)+`) + SetFieldValue(foo=10, col=`+strconv.Itoa(SliceWidth+2)+`) + SetFieldValue(foo=20, col=`+strconv.Itoa((5*SliceWidth)+100)+`) + SetFieldValue(foo=60, col=`+strconv.Itoa(SliceWidth+1)+`) + SetFieldValue(other=1000, col=0) + SetFieldValue(edge=100, col=0) + SetFieldValue(edge=-100, col=1) `), nil, nil); err != nil { t.Fatal(err) } diff --git a/frame.go b/frame.go index 1307df32d..f460a1224 100644 --- a/frame.go +++ b/frame.go @@ -726,10 +726,10 @@ func (f *Frame) FieldValue(columnID uint64, name string) (value int64, exists bo return int64(v) + field.Min, true, nil } -// SetFieldValue sets a field value for a column. -func (f *Frame) SetFieldValue(columnID uint64, name string, value int64) (changed bool, err error) { +// SetValue sets a field value for a column. +func (f *Frame) SetValue(columnID uint64, value int64) (changed bool, err error) { // Fetch field and validate value. - field := f.Field(name) + field := f.Field(f.name) if field == nil { return false, ErrFieldNotFound } else if value < field.Min { @@ -739,7 +739,7 @@ func (f *Frame) SetFieldValue(columnID uint64, name string, value int64) (change } // Fetch target view. - view, err := f.CreateViewIfNotExists(ViewFieldPrefix + name) + view, err := f.CreateViewIfNotExists(ViewFieldPrefix + f.name) if err != nil { return false, errors.Wrap(err, "creating view") } diff --git a/frame_test.go b/frame_test.go index 8ad9b020a..8000886c0 100644 --- a/frame_test.go +++ b/frame_test.go @@ -87,7 +87,7 @@ func TestFrame_SetFieldValue(t *testing.T) { } // Set value on field. - if changed, err := f.SetFieldValue(100, "f", 21); err != nil { + if changed, err := f.SetValue(100, 21); err != nil { t.Fatal(err) } else if !changed { t.Fatal("expected change") @@ -103,7 +103,7 @@ func TestFrame_SetFieldValue(t *testing.T) { } // Setting value should return no change. - if changed, err := f.SetFieldValue(100, "f", 21); err != nil { + if changed, err := f.SetValue(100, 21); err != nil { t.Fatal(err) } else if changed { t.Fatal("expected no change") @@ -124,14 +124,14 @@ func TestFrame_SetFieldValue(t *testing.T) { } // Set value. - if changed, err := f.SetFieldValue(100, "f", 21); err != nil { + if changed, err := f.SetValue(100, 21); err != nil { t.Fatal(err) } else if !changed { t.Fatal("expected change") } // Set different value. - if changed, err := f.SetFieldValue(100, "f", 23); err != nil { + if changed, err := f.SetValue(100, 23); err != nil { t.Fatal(err) } else if !changed { t.Fatal("expected change") @@ -152,16 +152,14 @@ func TestFrame_SetFieldValue(t *testing.T) { defer idx.Close() f, err := idx.CreateFrame("f", pilosa.FrameOptions{ - Type: pilosa.FrameTypeInt, - Min: 0, - Max: 30, + Type: pilosa.FrameTypeSet, }) if err != nil { t.Fatal(err) } // Set value. - if _, err := f.SetFieldValue(100, "no_such_field", 21); err != pilosa.ErrFieldNotFound { + if _, err := f.SetValue(100, 21); err != pilosa.ErrFieldNotFound { t.Fatalf("unexpected error: %s", err) } }) @@ -180,7 +178,7 @@ func TestFrame_SetFieldValue(t *testing.T) { } // Set value. - if _, err := f.SetFieldValue(100, "f", 15); err != pilosa.ErrFieldValueTooLow { + if _, err := f.SetValue(100, 15); err != pilosa.ErrFieldValueTooLow { t.Fatalf("unexpected error: %s", err) } }) @@ -199,7 +197,7 @@ func TestFrame_SetFieldValue(t *testing.T) { } // Set value. - if _, err := f.SetFieldValue(100, "f", 31); err != pilosa.ErrFieldValueTooHigh { + if _, err := f.SetValue(100, 31); err != pilosa.ErrFieldValueTooHigh { t.Fatalf("unexpected error: %s", err) } }) diff --git a/test/cluster.go b/test/cluster.go index 77cf496a8..9089089a7 100644 --- a/test/cluster.go +++ b/test/cluster.go @@ -150,6 +150,7 @@ func (t *TestCluster) SetBit(index, frame, view string, rowID, colID uint64, x * return nil } +// TODO: remove `name` from this function signature func (t *TestCluster) SetFieldValue(index, frame string, columnID uint64, name string, value int64) error { // Determine which node should receive the SetFieldValue. c0 := t.Clusters[0] // use the first node's cluster to determine slice location. @@ -165,7 +166,7 @@ func (t *TestCluster) SetFieldValue(index, frame string, columnID uint64, name s if f == nil { return fmt.Errorf("index/frame does not exist: %s/%s", index, frame) } - _, err := f.SetFieldValue(columnID, name, value) + _, err := f.SetValue(columnID, value) if err != nil { return err } diff --git a/utils_test.go b/utils_test.go index 1732522d4..26fe06890 100644 --- a/utils_test.go +++ b/utils_test.go @@ -141,6 +141,7 @@ func (t *ClusterCluster) SetBit(index, frame, view string, rowID, colID uint64, return nil } +// TODO: remove `name` from this function signature func (t *ClusterCluster) SetFieldValue(index, frame string, columnID uint64, name string, value int64) error { // Determine which node should receive the SetFieldValue. c0 := t.Clusters[0] // use the first node's cluster to determine slice location. @@ -156,7 +157,7 @@ func (t *ClusterCluster) SetFieldValue(index, frame string, columnID uint64, nam if f == nil { return fmt.Errorf("index/frame does not exist: %s/%s", index, frame) } - _, err := f.SetFieldValue(columnID, name, value) + _, err := f.SetValue(columnID, value) if err != nil { return err }