From efed5ea3fce3b20f795d2dbdf9c649b2d7aed175 Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Wed, 3 Oct 2018 13:43:15 +0300 Subject: [PATCH 1/4] Fixes #1632 --- api.go | 64 +++------------------------------------------ executor.go | 75 +++++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 70 insertions(+), 69 deletions(-) diff --git a/api.go b/api.go index 44ea0a7b0..d910fbbe8 100644 --- a/api.go +++ b/api.go @@ -102,11 +102,9 @@ func (api *API) Query(ctx context.Context, req *QueryRequest) (QueryResponse, er return QueryResponse{}, errors.Wrap(err, "validating api method") } - resp := QueryResponse{} - q, err := pql.NewParser(strings.NewReader(req.Query)).Parse() if err != nil { - return resp, errors.Wrap(err, "parsing") + return QueryResponse{}, errors.Wrap(err, "parsing") } execOpts := &execOptions{ Remote: req.Remote, @@ -114,70 +112,14 @@ func (api *API) Query(ctx context.Context, req *QueryRequest) (QueryResponse, er ExcludeColumns: req.ExcludeColumns, // NOTE: Kept for Pilosa 1.x compat. ColumnAttrs: req.ColumnAttrs, // NOTE: Kept for Pilosa 1.x compat. } - results, err := api.server.executor.Execute(ctx, req.Index, q, req.Shards, execOpts) + resp, err := api.server.executor.Execute(ctx, req.Index, q, req.Shards, execOpts) if err != nil { - return resp, errors.Wrap(err, "executing") + return QueryResponse{}, errors.Wrap(err, "executing") } - resp.Results = results - // Fill column attributes if requested. - // execOpts.ColumnAttrs may be set by the Execute method if any of the Calls use Options(columnAttrs=true) - if execOpts.ColumnAttrs { - // Consolidate all column ids across all calls. - var columnIDs []uint64 - for _, result := range results { - bm, ok := result.(*Row) - if !ok { - continue - } - columnIDs = uint64Slice(columnIDs).merge(bm.Columns()) - } - - // Retrieve column attributes across all calls. - columnAttrSets, err := api.readColumnAttrSets(api.holder.Index(req.Index), columnIDs) - if err != nil { - return resp, errors.Wrap(err, "reading column attrs") - } - - // Translate column attributes, if necessary. - if api.holder.translateFile != nil { - for _, col := range resp.ColumnAttrSets { - v, err := api.holder.translateFile.TranslateColumnToString(req.Index, col.ID) - if err != nil { - return resp, err - } - col.Key, col.ID = v, 0 - } - } - - resp.ColumnAttrSets = columnAttrSets - } return resp, nil } -// readColumnAttrSets returns a list of column attribute objects by id. -func (api *API) readColumnAttrSets(index *Index, ids []uint64) ([]*ColumnAttrSet, error) { - if index == nil { - return nil, nil - } - - ax := make([]*ColumnAttrSet, 0, len(ids)) - for _, id := range ids { - // Read attributes for column. Skip column if empty. - attrs, err := index.ColumnAttrStore().Attrs(id) - if err != nil { - return nil, errors.Wrap(err, "getting attrs") - } else if len(attrs) == 0 { - continue - } - - // Append column with attributes. - ax = append(ax, &ColumnAttrSet{ID: id, Attrs: attrs}) - } - - return ax, nil -} - // CreateIndex makes a new Pilosa index. func (api *API) CreateIndex(_ context.Context, indexName string, options IndexOptions) (*Index, error) { if err := api.validate(apiCreateIndex); err != nil { diff --git a/executor.go b/executor.go index 8dd4db569..29da40b27 100644 --- a/executor.go +++ b/executor.go @@ -79,20 +79,21 @@ func newExecutor(opts ...executorOption) *executor { } // Execute executes a PQL query. -func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shards []uint64, opt *execOptions) ([]interface{}, error) { +func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shards []uint64, opt *execOptions) (QueryResponse, error) { + resp := QueryResponse{} // Verify that an index is set. if index == "" { - return nil, ErrIndexRequired + return resp, ErrIndexRequired } idx := e.Holder.Index(index) if idx == nil { - return nil, ErrIndexNotFound + return resp, ErrIndexNotFound } // Verify that the number of writes do not exceed the maximum. if e.MaxWritesPerRequest > 0 && q.WriteCallN() > e.MaxWritesPerRequest { - return nil, ErrTooManyWrites + return resp, ErrTooManyWrites } // Default options. @@ -105,27 +106,85 @@ func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shar if !opt.Remote { for i := range q.Calls { if err := e.translateCall(index, idx, q.Calls[i]); err != nil { - return nil, err + return resp, err } } } results, err := e.execute(ctx, index, q, shards, opt) if err != nil { - return nil, err + return resp, err } + resp.Results = results + // Translate response objects from ids to keys, if necessary. // No need to translate a remote call. if !opt.Remote { for i := range results { results[i], err = e.translateResult(index, idx, q.Calls[i], results[i]) if err != nil { - return nil, err + return resp, err } } } - return results, nil + + // Fill column attributes if requested. + if opt.ColumnAttrs { + // Consolidate all column ids across all calls. + var columnIDs []uint64 + for _, result := range results { + bm, ok := result.(*Row) + if !ok { + continue + } + columnIDs = uint64Slice(columnIDs).merge(bm.Columns()) + } + + // Retrieve column attributes across all calls. + columnAttrSets, err := e.readColumnAttrSets(e.Holder.Index(index), columnIDs) + if err != nil { + return resp, errors.Wrap(err, "reading column attrs") + } + + // Translate column attributes, if necessary. + if e.Holder.translateFile != nil { + for _, col := range resp.ColumnAttrSets { + v, err := e.Holder.translateFile.TranslateColumnToString(index, col.ID) + if err != nil { + return resp, err + } + col.Key, col.ID = v, 0 + } + } + + resp.ColumnAttrSets = columnAttrSets + } + + return resp, nil +} + +// readColumnAttrSets returns a list of column attribute objects by id. +func (e *executor) readColumnAttrSets(index *Index, ids []uint64) ([]*ColumnAttrSet, error) { + if index == nil { + return nil, nil + } + + ax := make([]*ColumnAttrSet, 0, len(ids)) + for _, id := range ids { + // Read attributes for column. Skip column if empty. + attrs, err := index.ColumnAttrStore().Attrs(id) + if err != nil { + return nil, errors.Wrap(err, "getting attrs") + } else if len(attrs) == 0 { + continue + } + + // Append column with attributes. + ax = append(ax, &ColumnAttrSet{ID: id, Attrs: attrs}) + } + + return ax, nil } func (e *executor) execute(ctx context.Context, index string, q *pql.Query, shards []uint64, opt *execOptions) ([]interface{}, error) { From bd48db1435456dd6eb0c283eb7022199803985b4 Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Fri, 5 Oct 2018 16:29:29 +0300 Subject: [PATCH 2/4] updated executor.Execute logic for columnAttrs with keys; added columnAttrs with keys test --- executor.go | 35 +++++++++++++++++++++-------------- executor_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/executor.go b/executor.go index 29da40b27..235000938 100644 --- a/executor.go +++ b/executor.go @@ -118,17 +118,6 @@ func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shar resp.Results = results - // Translate response objects from ids to keys, if necessary. - // No need to translate a remote call. - if !opt.Remote { - for i := range results { - results[i], err = e.translateResult(index, idx, q.Calls[i], results[i]) - if err != nil { - return resp, err - } - } - } - // Fill column attributes if requested. if opt.ColumnAttrs { // Consolidate all column ids across all calls. @@ -148,8 +137,8 @@ func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shar } // Translate column attributes, if necessary. - if e.Holder.translateFile != nil { - for _, col := range resp.ColumnAttrSets { + if idx.Keys() { + for _, col := range columnAttrSets { v, err := e.Holder.translateFile.TranslateColumnToString(index, col.ID) if err != nil { return resp, err @@ -161,6 +150,17 @@ func (e *executor) Execute(ctx context.Context, index string, q *pql.Query, shar resp.ColumnAttrSets = columnAttrSets } + // Translate response objects from ids to keys, if necessary. + // No need to translate a remote call. + if !opt.Remote { + for i := range results { + results[i], err = e.translateResult(index, idx, q.Calls[i], results[i]) + if err != nil { + return resp, err + } + } + } + return resp, nil } @@ -1765,9 +1765,16 @@ func (e *executor) mapperLocal(ctx context.Context, shards []uint64, mapFn mapFu } } +var translateCallCol = map[string]struct{}{ + "Set": struct{}{}, + "Clear": struct{}{}, + "Row": struct{}{}, + "SetColumnAttrs": struct{}{}, +} + func (e *executor) translateCall(index string, idx *Index, c *pql.Call) error { var colKey, rowKey, fieldName string - if c.Name == "Set" || c.Name == "Clear" || c.Name == "Row" { + if _, ok := translateCallCol[c.Name]; ok { // Positional args in new PQL syntax require special handling here. colKey = "_" + columnLabel fieldName, _ = c.FieldArg() diff --git a/executor_test.go b/executor_test.go index d6d529efe..5a5f68ff9 100644 --- a/executor_test.go +++ b/executor_test.go @@ -1509,6 +1509,36 @@ func TestExecutor_QueryCall(t *testing.T) { } }) + t.Run("columnAttrsWithKeys", func(t *testing.T) { + c := test.MustRunCluster(t, 1) + defer c.Close() + hldr := test.Holder{Holder: c[0].Server.Holder()} + + // Set columns for rows 0, 10, & 20 across two shards. + if idx, err := hldr.CreateIndex("i", pilosa.IndexOptions{Keys: true}); err != nil { + t.Fatal(err) + } else if _, err := idx.CreateField("f", pilosa.OptFieldKeys()); err != nil { + t.Fatal(err) + } else if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: ` + Set("one-hundred", f="ten") + SetColumnAttrs("one-hundred", foo="bar") + `}); err != nil { + t.Fatal(err) + } + + targetColAttrSets := []*pilosa.ColumnAttrSet{ + {Key: "one-hundred", Attrs: map[string]interface{}{"foo": "bar"}}, + } + + if res, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Options(Row(f="ten"), columnAttrs=true)`}); err != nil { + t.Fatal(err) + } else if keys := res.Results[0].(*pilosa.Row).Keys; !reflect.DeepEqual(keys, []string{"one-hundred"}) { + t.Fatalf("unexpected keys: %+v", keys) + } else if attrs := res.ColumnAttrSets; !reflect.DeepEqual(attrs, targetColAttrSets) { + t.Fatalf("unexpected attrs: %s", spew.Sdump(attrs)) + } + }) + t.Run("shards", func(t *testing.T) { c := test.MustRunCluster(t, 1) defer c.Close() From b6981930b142fd98bad4a17500709994f9d59baa Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Fri, 5 Oct 2018 16:47:53 +0300 Subject: [PATCH 3/4] ColumnAttrsSet omit empty ID --- pilosa.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pilosa.go b/pilosa.go index 7378eb628..9798ff315 100644 --- a/pilosa.go +++ b/pilosa.go @@ -117,7 +117,7 @@ var nameRegexp = regexp.MustCompile(`^[a-z][a-z0-9_-]{0,63}$`) // ColumnAttrSet represents a set of attributes for a vertical column in an index. // Can have a set of attributes attached to it. type ColumnAttrSet struct { - ID uint64 `json:"id"` + ID uint64 `json:"id,omitempty"` Key string `json:"key,omitempty"` Attrs map[string]interface{} `json:"attrs,omitempty"` } From b617db84eb6be6fd590a8c2e57d300cbc8dd34d1 Mon Sep 17 00:00:00 2001 From: Yuce Tekol Date: Fri, 5 Oct 2018 17:44:02 +0300 Subject: [PATCH 4/4] gfmt'ed --- executor.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/executor.go b/executor.go index 235000938..76f4d7b65 100644 --- a/executor.go +++ b/executor.go @@ -1766,10 +1766,10 @@ func (e *executor) mapperLocal(ctx context.Context, shards []uint64, mapFn mapFu } var translateCallCol = map[string]struct{}{ - "Set": struct{}{}, - "Clear": struct{}{}, - "Row": struct{}{}, - "SetColumnAttrs": struct{}{}, + "Set": {}, + "Clear": {}, + "Row": {}, + "SetColumnAttrs": {}, } func (e *executor) translateCall(index string, idx *Index, c *pql.Call) error {