From 81a4d32cdddd4031d3fac0919a05f5bd35505071 Mon Sep 17 00:00:00 2001 From: Matt Jaffee Date: Wed, 27 Nov 2019 08:41:40 -0600 Subject: [PATCH] allow IDs to be passed even when keys enabled This change allows one to query Pilosa fields and indexes directly with integer row and column ids even when key translation is enabled. This was previously disallowed during query translation... I'm not sure why, but it can be quite useful for debugging and testing to be able to use IDs directly. I have a test in go-pilosa which uses this functionality. I also simplified a bunch of the test code which was of the form: ``` else { if blah { } } ``` to be: ``` else if blah { ``` which I think is pretty harmless. I also changed a snapshot log line that has been bugging me to be Debug level so that it isn't generating lots of useless logs for long running Pilosa instances. --- executor.go | 38 ++++++++++++++++++++++++++++------ executor_test.go | 53 ++++++++++++++++++++++++++++++++---------------- snapshotqueue.go | 2 +- 3 files changed, 68 insertions(+), 25 deletions(-) diff --git a/executor.go b/executor.go index 8d31c739b..89c371aff 100644 --- a/executor.go +++ b/executor.go @@ -3170,9 +3170,10 @@ func (e *executor) translateCall(index string, idx *Index, c *pql.Call) error { // Translate column key. if idx.Keys() { if c.Args[colKey] != nil && !isString(c.Args[colKey]) { - return errors.New("column value must be a string when index 'keys' option enabled") - } - if value := callArgString(c, colKey); value != "" { + if !isValidID(c.Args[colKey]) { + return errors.Errorf("column value must be a string or non-negative integer, but got: %v of %[1]T", c.Args[colKey]) + } + } else if value := callArgString(c, colKey); value != "" { id, err := idx.translateStore.TranslateKey(value) if err != nil { return err @@ -3216,9 +3217,11 @@ func (e *executor) translateCall(index string, idx *Index, c *pql.Call) error { } } else if field.keys() { if c.Args[rowKey] != nil && !isString(c.Args[rowKey]) { - return errors.New("row value must be a string when field 'keys' option enabled") - } - if value := callArgString(c, rowKey); value != "" { + // allow passing row id directly (this can come in handy, but make sure it is a valid row id) + if !isValidID(c.Args[rowKey]) { + return errors.Errorf("row value must be a string or non-negative integer, but got: %v of %[1]T", c.Args[rowKey]) + } + } else if value := callArgString(c, rowKey); value != "" { id, err := field.translateStore.TranslateKey(value) if err != nil { return err @@ -3626,6 +3629,29 @@ func isString(v interface{}) bool { return ok } +// isValidID returns whether v can be interpreted as a valid row or +// column ID. In short, is v a non-negative integer? I think the int64 +// and default cases are the only ones actually used since the PQL +// parser doesn't return any other integer types. +func isValidID(v interface{}) bool { + switch vt := v.(type) { + case uint, uint64, uint32, uint16, uint8: + return true + case int64: + return vt >= 0 + case int: + return vt >= 0 + case int32: + return vt >= 0 + case int16: + return vt >= 0 + case int8: + return vt >= 0 + default: + return false + } +} + // groupByIterator contains several slices. Each slice contains a number of // elements equal to the number of fields in the group by (the number of Rows // calls). diff --git a/executor_test.go b/executor_test.go index 834532fa8..ce40d56f0 100644 --- a/executor_test.go +++ b/executor_test.go @@ -526,10 +526,8 @@ func TestExecutor_Execute_Set(t *testing.T) { if res, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(1, f=11)`}); err != nil { t.Fatal(err) - } else { - if !res.Results[0].(bool) { - t.Fatalf("expected column changed") - } + } else if !res.Results[0].(bool) { + t.Fatalf("expected column changed") } if n := hldr.Row("i", "f", 11).Count(); n != 1 { @@ -537,10 +535,8 @@ func TestExecutor_Execute_Set(t *testing.T) { } if res, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(1, f=11)`}); err != nil { t.Fatal(err) - } else { - if res.Results[0].(bool) { - t.Fatalf("expected column unchanged") - } + } else if res.Results[0].(bool) { + t.Fatalf("expected column unchanged") } }) @@ -591,10 +587,8 @@ func TestExecutor_Execute_Set(t *testing.T) { if res, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set("foo", f=11)`}); err != nil { t.Fatal(err) - } else { - if !res.Results[0].(bool) { - t.Fatalf("expected column changed") - } + } else if !res.Results[0].(bool) { + t.Fatalf("expected column changed") } if n := hldr.Row("i", "f", 11).Count(); n != 1 { @@ -602,10 +596,20 @@ func TestExecutor_Execute_Set(t *testing.T) { } if res, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set("foo", f=11)`}); err != nil { t.Fatal(err) - } else { - if res.Results[0].(bool) { - t.Fatalf("expected column unchanged") - } + } else if res.Results[0].(bool) { + t.Fatalf("expected column unchanged") + } + + if res, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f=11)`}); err != nil { + t.Fatal(err) + } else if !res.Results[0].(bool) { + t.Fatalf("expected column changed with integer column key") + } + + if res, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f=11)`}); err != nil { + t.Fatal(err) + } else if res.Results[0].(bool) { + t.Fatalf("expected column unchanged with integer column key") } }) @@ -617,9 +621,15 @@ func TestExecutor_Execute_Set(t *testing.T) { t.Fatal(err) } - if _, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f=1)`}); err == nil || errors.Cause(err).Error() != `column value must be a string when index 'keys' option enabled` { + if _, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2.1, f=1)`}); err == nil || strings.Contains(err.Error(), `column value must be a string or non-negative integer`) { t.Fatal(err) } + + if res, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f=1)`}); err != nil { + t.Fatal(err) + } else if !res.Results[0].(bool) { + t.Fatalf("expected column changed with integer column key") + } }) t.Run("ErrInvalidRowValueType", func(t *testing.T) { @@ -627,9 +637,16 @@ func TestExecutor_Execute_Set(t *testing.T) { if _, err := index.CreateField("f", pilosa.OptFieldTypeDefault(), pilosa.OptFieldKeys()); err != nil { t.Fatal(err) } - if _, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "inokey", Query: `Set(2, f=1)`}); err == nil || errors.Cause(err).Error() != `row value must be a string when field 'keys' option enabled` { + if _, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "inokey", Query: `Set(2, f=1.2)`}); err == nil || !strings.Contains(err.Error(), "row value must be a string or non-negative integer") { t.Fatal(err) } + + if res, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f=9)`}); err != nil { + t.Fatal(err) + } else if !res.Results[0].(bool) { + t.Fatalf("expected column changed with integer column key") + } + }) }) } diff --git a/snapshotqueue.go b/snapshotqueue.go index d355824e3..9df79e26d 100644 --- a/snapshotqueue.go +++ b/snapshotqueue.go @@ -394,7 +394,7 @@ func (sq *prioritySnapshotQueue) scanHolderWorker(h *Holder, background chan sna sq.logger.Printf("background scan: %d fragments needed snapshots\n", hits) hits = 0 } else { - sq.logger.Printf("background scan: no fragments needed snapshots, waiting\n") + sq.logger.Debugf("background scan: no fragments needed snapshots, waiting\n") // No reason to be active if we're not finding anything. select { case <-time.After(60 * time.Second):