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):