Merge branch 'master' into fix-delete-field-panic

cluster.go

@@ -1356,9 +1356,8 @@ func (c *cluster) findFieldKeys(ctx context.Context, field *Field, keys ...strin
 		// Therefore, the field keys are actually column keys on a different index.
 		return c.findIndexKeys(ctx, idx, keys...)
 	}
-
 	if !field.Keys() {
-		return nil, errors.Wrap(ErrTranslatingKeyNotFound, "field is not keyed")
+		return nil, errors.Errorf("cannot find keys on unkeyed field %q", field.Name())
 	}
 
 	// Attempt to find the keys locally.
@@ -1421,7 +1420,7 @@ func (c *cluster) createFieldKeys(ctx context.Context, field *Field, keys ...str
 	}
 
 	if !field.Keys() {
-		return nil, errors.Wrap(ErrTranslatingKeyNotFound, "field is not keyed")
+		return nil, errors.Errorf("cannot create keys on unkeyed field %q", field.Name())
 	}
 
 	// The primary is the only node that can create field keys, since it owns the authoritative copy.
@@ -1621,6 +1620,9 @@ func (c *cluster) findIndexKeys(ctx context.Context, indexName string, keys ...s
 	if idx == nil {
 		return nil, ErrIndexNotFound
 	}
+	if !idx.Keys() {
+		return nil, errors.Errorf("cannot find keys on unkeyed index %q", indexName)
+	}
 
 	// Create a snapshot of the cluster to use for node/partition calculations.
 	snap := topology.NewClusterSnapshot(c.noder, c.Hasher, c.ReplicaN)
@@ -1728,7 +1730,7 @@ func (c *cluster) createIndexKeys(ctx context.Context, indexName string, keys ..
 	}
 
 	if !idx.keys {
-		return nil, errors.Errorf("can't create index keys on unkeyed index %s", indexName)
+		return nil, errors.Errorf("cannot create keys on unkeyed index %q", indexName)
 	}
 
 	// Create a snapshot of the cluster to use for node/partition calculations.

executor.go

@@ -1125,9 +1125,9 @@ func (e *executor) executeSum(ctx context.Context, qcx *Qcx, index string, c *pq
 	span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executeSum")
 	defer span.Finish()
 
-	fieldName, ok := c.Args["field"].(string)
-	if !ok || fieldName == "" {
-		return ValCount{}, errors.New("Sum(): field required")
+	fieldName, err := c.FirstStringArg("field", "_field")
+	if err != nil {
+		return ValCount{}, errors.Wrap(err, "Sum(): field required")
 	}
 
 	if len(c.Children) > 1 {
@@ -1229,8 +1229,9 @@ func (e *executor) executeDistinct(ctx context.Context, qcx *Qcx, index string,
 func (e *executor) executeMin(ctx context.Context, qcx *Qcx, index string, c *pql.Call, shards []uint64, opt *execOptions) (_ ValCount, err error) {
 	span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executeMin")
 	defer span.Finish()
-	if field := c.Args["field"]; field == "" {
-		return ValCount{}, errors.New("Min(): field required")
+
+	if _, err := c.FirstStringArg("field", "_field"); err != nil {
+		return ValCount{}, errors.Wrap(err, "Min(): field required")
 	}
 
 	if len(c.Children) > 1 {
@@ -1265,8 +1266,8 @@ func (e *executor) executeMax(ctx context.Context, qcx *Qcx, index string, c *pq
 	span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executeMax")
 	defer span.Finish()
 
-	if field := c.Args["field"]; field == "" {
-		return ValCount{}, errors.New("Max(): field required")
+	if _, err := c.FirstStringArg("field", "_field"); err != nil {
+		return ValCount{}, errors.Wrap(err, "Max(): field required")
 	}
 
 	if len(c.Children) > 1 {
@@ -1889,7 +1890,10 @@ func (e *executor) executeSumCountShard(ctx context.Context, qcx *Qcx, index str
 		filter = row
 	}
 
-	fieldName, _ := c.Args["field"].(string)
+	fieldName, err := c.FirstStringArg("field", "_field")
+	if err != nil {
+		return ValCount{}, errors.Wrap(err, "Sum(): field required")
+	}
 
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {
@@ -1940,7 +1944,10 @@ func (e *executor) executeMinShard(ctx context.Context, qcx *Qcx, index string,
 		filter = row
 	}
 
-	fieldName, _ := c.Args["field"].(string)
+	fieldName, err := c.FirstStringArg("field", "_field")
+	if err != nil {
+		return ValCount{}, errors.Wrap(err, "Min(): field required")
+	}
 
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {
@@ -1969,7 +1976,10 @@ func (e *executor) executeMaxShard(ctx context.Context, qcx *Qcx, index string,
 		filter = row
 	}
 
-	fieldName, _ := c.Args["field"].(string)
+	fieldName, err := c.FirstStringArg("field", "_field")
+	if err != nil {
+		return ValCount{}, errors.Wrap(err, "Max(): field required")
+	}
 
 	field := e.Holder.Field(index, fieldName)
 	if field == nil {

executor_test.go

@@ -587,7 +587,7 @@ func TestExecutor_Execute_Set(t *testing.T) {
 		})
 
 		t.Run("ErrInvalidRowValueType", func(t *testing.T) {
-			if _, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f="bar")`}); err == nil || !hasCause(err, pilosa.ErrTranslatingKeyNotFound) || !strings.Contains(err.Error(), "field is not keyed") {
+			if _, err := cmd.API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(2, f="bar")`}); err == nil || !strings.Contains(err.Error(), "cannot create keys on unkeyed field") {
 				t.Fatal(err)
 			}
 		})
@@ -990,26 +990,13 @@ func TestExecutor_Execute_SetValue(t *testing.T) {
 		})
 
 		t.Run("InvalidBSIGroupValueType", func(t *testing.T) {
-			if _, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(10, f="hello")`}); err == nil || !hasCause(err, pilosa.ErrTranslatingKeyNotFound) || !strings.Contains(err.Error(), "field is not keyed") {
+			if _, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Set(10, f="hello")`}); err == nil || !strings.Contains(err.Error(), "cannot create keys on unkeyed field") {
 				t.Fatalf("unexpected error: %s", err)
 			}
 		})
 	})
 }
 
-func hasCause(err, cause error) bool {
-	for err != cause {
-		innerErr := errors.Cause(err)
-		if innerErr == err {
-			// This is the innermost accessible error, and it does not have that cause.
-			return false
-		}
-		err = innerErr
-	}
-
-	return true
-}
-
 // Ensure a SetRowAttrs() query can be executed.
 func TestExecutor_Execute_SetRowAttrs(t *testing.T) {
 	c := test.MustRunCluster(t, 1)
@@ -1581,6 +1568,42 @@ func TestExecutor_Execute_MinMax(t *testing.T) {
 							t.Fatalf("unexpected max result, test %d: %s", i, spew.Sdump(result))
 						}
 					})
+
+					t.Run("Min", func(t *testing.T) {
+						pql = fmt.Sprintf(`Min(field="%s")`, fld)
+						if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: pql}); err != nil {
+							t.Fatal(err)
+						} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{Val: test.set, Count: 1}) {
+							t.Fatalf("unexpected min result, test %d: %s", i, spew.Sdump(result))
+						}
+					})
+
+					t.Run("Max", func(t *testing.T) {
+						pql = fmt.Sprintf(`Max(field="%s")`, fld)
+						if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: pql}); err != nil {
+							t.Fatal(err)
+						} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{Val: test.set, Count: 1}) {
+							t.Fatalf("unexpected max result, test %d: %s", i, spew.Sdump(result))
+						}
+					})
+
+					t.Run("Min", func(t *testing.T) {
+						pql = fmt.Sprintf(`Min(%s)`, fld)
+						if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: pql}); err != nil {
+							t.Fatal(err)
+						} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{Val: test.set, Count: 1}) {
+							t.Fatalf("unexpected min result, test %d: %s", i, spew.Sdump(result))
+						}
+					})
+
+					t.Run("Max", func(t *testing.T) {
+						pql = fmt.Sprintf(`Max(%s)`, fld)
+						if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: pql}); err != nil {
+							t.Fatal(err)
+						} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{Val: test.set, Count: 1}) {
+							t.Fatalf("unexpected max result, test %d: %s", i, spew.Sdump(result))
+						}
+					})
 				})
 			}
 		})
@@ -1695,6 +1718,24 @@ func TestExecutor_Execute_MinMax(t *testing.T) {
 							t.Fatalf("unexpected max result, test %d: %s", i, spew.Sdump(result))
 						}
 					})
+
+					t.Run("Min", func(t *testing.T) {
+						pql = fmt.Sprintf(`Min(%s)`, fld)
+						if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: pql}); err != nil {
+							t.Fatal(err)
+						} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{DecimalVal: &test.exp, Count: 1}) {
+							t.Fatalf("unexpected min result, test %d: %s", i, spew.Sdump(result))
+						}
+					})
+
+					t.Run("Max", func(t *testing.T) {
+						pql = fmt.Sprintf(`Max(%s)`, fld)
+						if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: pql}); err != nil {
+							t.Fatal(err)
+						} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{DecimalVal: &test.exp, Count: 1}) {
+							t.Fatalf("unexpected max result, test %d: %s", i, spew.Sdump(result))
+						}
+					})
 				})
 			}
 		})
@@ -2024,6 +2065,22 @@ func TestExecutor_Execute_Sum(t *testing.T) {
 				}
 			})
 
+			t.Run("NoFilter", func(t *testing.T) {
+				if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Sum(field="foo")`}); err != nil {
+					t.Fatal(err)
+				} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{Val: 200, Count: 5}) {
+					t.Fatalf("unexpected result: %s", spew.Sdump(result))
+				}
+			})
+
+			t.Run("NoFilter", func(t *testing.T) {
+				if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Sum(foo)`}); err != nil {
+					t.Fatal(err)
+				} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{Val: 200, Count: 5}) {
+					t.Fatalf("unexpected result: %s", spew.Sdump(result))
+				}
+			})
+
 			t.Run("WithFilter", func(t *testing.T) {
 				if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Sum(Row(x=0), field=foo)`}); err != nil {
 					t.Fatal(err)
@@ -2031,6 +2088,14 @@ func TestExecutor_Execute_Sum(t *testing.T) {
 					t.Fatalf("unexpected result: %s", spew.Sdump(result))
 				}
 			})
+
+			t.Run("WithFilter", func(t *testing.T) {
+				if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Sum(foo, Row(x=0))`}); err != nil {
+					t.Fatal(err)
+				} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{Val: 80, Count: 2}) {
+					t.Fatalf("unexpected result: %s", spew.Sdump(result))
+				}
+			})
 		})
 
 		t.Run("Decimal", func(t *testing.T) {
@@ -2049,6 +2114,23 @@ func TestExecutor_Execute_Sum(t *testing.T) {
 					t.Fatalf("unexpected result: %s", spew.Sdump(result))
 				}
 			})
+
+			t.Run("NoFilter", func(t *testing.T) {
+				if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Sum(dec)`}); err != nil {
+					t.Fatal(err)
+				} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{DecimalVal: &pql.Decimal{Value: 700007, Scale: 3}, Count: 3}) {
+					t.Fatalf("unexpected result: %s", spew.Sdump(result))
+				}
+			})
+
+			t.Run("WithFilter", func(t *testing.T) {
+				if result, err := c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `Sum(dec, Row(x=0))`}); err != nil {
+					t.Fatal(err)
+				} else if !reflect.DeepEqual(result.Results[0], pilosa.ValCount{DecimalVal: &pql.Decimal{Value: 500005, Scale: 3}, Count: 2}) {
+					t.Fatalf("unexpected result: %s", spew.Sdump(result))
+				}
+			})
+
 		})
 	})
 

pql/ast.go

@@ -335,7 +335,8 @@ var stringOrInt64 stringOrInt64Type
 var allowField = callInfo{
 	allowUnknown: false,
 	prototypes: map[string]interface{}{
-		"field": "",
+		"_field": "",
+		"field":  "",
 	},
 }
 
@@ -722,6 +723,21 @@ func (c *Call) StringArg(key string) (string, bool, error) {
 	}
 }
 
+func (c *Call) FirstStringArg(keys ...string) (string, error) {
+	for _, k := range keys {
+		val, ok, err := c.StringArg(k)
+		if err != nil {
+			return "", err
+		}
+		if !ok {
+			continue
+		}
+		return val, nil
+	}
+
+	return "", fmt.Errorf("keys: %v not found", keys)
+}
+
 // CallArg is for reading the value at key from call.Args as a Call. If the
 // key is not in Call.Args, the value of the returned value will be nil, and
 // the error will be nil. An error is returned if the value is not a Call.

pql/pql.peg

@@ -14,8 +14,11 @@ Call <-  "Set" {p.startCall("Set")} open col comma args (comma timestamp)? close
        / "Store" {p.startCall("Store")} open Call comma arg close {p.endCall()}
        / "TopN" {p.startCall("TopN")} open posfield (comma allargs)? close {p.endCall()}
        / "TopK" {p.startCall("TopK")} open posfield (comma allargs)? close {p.endCall()}
-       / "Percentile" {p.startCall("Percentile")} open posfield (comma allargs)? close {p.endCall()}       
+       / "Percentile" {p.startCall("Percentile")} open posfield (comma allargs)? close {p.endCall()}
        / "Rows" {p.startCall("Rows")} open posfield (comma allargs)? close {p.endCall()}
+       / "Min" {p.startCall("Min")} open posfield (comma allargs)? close {p.endCall()}
+       / "Max" {p.startCall("Max")} open posfield (comma allargs)? close {p.endCall()}
+       / "Sum" {p.startCall("Sum")} open posfield (comma allargs)? close {p.endCall()}
        / "Range" {p.startCall("Range")} open field eq value comma 'from='? {p.addField("from")} timestampfmt {p.addVal(text)} comma 'to='? sp {p.addField("to")} timestampfmt {p.addVal(text)} close {p.endCall()}
        / < IDENT > { p.startCall(text) } open allargs comma? close { p.endCall() }
 allargs <- Call (comma Call)* (comma args)? / args / sp

pql/pqlpeg_test.go

@@ -710,13 +710,58 @@ func TestPQLDeepEquality(t *testing.T) {
 					},
 				},
 			}},
+		{
+			name: "Sum",
+			call: "Sum(f)",
+			exp: &Call{
+				Name: "Sum",
+				Args: map[string]interface{}{
+					"_field": "f",
+				},
+			}},
 		{
 			name: "Sum",
 			call: "Sum(field=f)",
 			exp: &Call{
 				Name: "Sum",
 				Args: map[string]interface{}{
-					"field": "f",
+					"_field": "f",
+				},
+			}},
+		{
+			name: "Max",
+			call: "Max(f)",
+			exp: &Call{
+				Name: "Max",
+				Args: map[string]interface{}{
+					"_field": "f",
+				},
+			}},
+		{
+			name: "Max",
+			call: "Max(field=f)",
+			exp: &Call{
+				Name: "Max",
+				Args: map[string]interface{}{
+					"_field": "f",
+				},
+			}},
+		{
+			name: "Min",
+			call: "Min(f)",
+			exp: &Call{
+				Name: "Min",
+				Args: map[string]interface{}{
+					"_field": "f",
+				},
+			}},
+		{
+			name: "Min",
+			call: "Min(field=f)",
+			exp: &Call{
+				Name: "Min",
+				Args: map[string]interface{}{
+					"_field": "f",
 				},
 			}},
 		{
@@ -740,6 +785,18 @@ func TestPQLDeepEquality(t *testing.T) {
 					{Name: "Row"},
 				},
 			}},
+		{
+			name: "SumChild",
+			call: "Sum(f, Row())",
+			exp: &Call{
+				Name: "Sum",
+				Args: map[string]interface{}{
+					"_field": "f",
+				},
+				Children: []*Call{
+					{Name: "Row"},
+				},
+			}},
 		{
 			name: "MinChild",
 			call: "Min(Row(), field=f)",

translator_test.go

@@ -879,6 +879,70 @@ func TestTranslation_Cluster_CreateFind(t *testing.T) {
 	})
 }
 
+func TestTranslation_Cluster_CreateFindUnkeyed(t *testing.T) {
+	c := test.MustRunCluster(t, 3)
+	defer c.Close()
+
+	c.CreateField(t, "i", pilosa.IndexOptions{}, "f")
+
+	t.Run("Index", func(t *testing.T) {
+		t.Run("Create", func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+
+			_, err := c.GetNonPrimary().API.CreateIndexKeys(ctx, "i", "foo")
+			if err == nil {
+				t.Fatal("unexpected success")
+			}
+			expect := `cannot create keys on unkeyed index "i"`
+			if got := err.Error(); got != expect {
+				t.Fatalf("expected error %q but got %q", expect, got)
+			}
+		})
+		t.Run("Find", func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+
+			_, err := c.GetNonPrimary().API.FindIndexKeys(ctx, "i", "foo")
+			if err == nil {
+				t.Fatal("unexpected success")
+			}
+			expect := `cannot find keys on unkeyed index "i"`
+			if got := err.Error(); got != expect {
+				t.Fatalf("expected error %q but got %q", expect, got)
+			}
+		})
+	})
+	t.Run("Field", func(t *testing.T) {
+		t.Run("Create", func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+
+			_, err := c.GetNonPrimary().API.CreateFieldKeys(ctx, "i", "f", "foo")
+			if err == nil {
+				t.Fatal("unexpected success")
+			}
+			expect := `cannot create keys on unkeyed field "f"`
+			if got := err.Error(); got != expect {
+				t.Fatalf("expected error %q but got %q", expect, got)
+			}
+		})
+		t.Run("Find", func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			defer cancel()
+
+			_, err := c.GetNonPrimary().API.FindFieldKeys(ctx, "i", "f", "foo")
+			if err == nil {
+				t.Fatal("unexpected success")
+			}
+			expect := `cannot find keys on unkeyed field "f"`
+			if got := err.Error(); got != expect {
+				t.Fatalf("expected error %q but got %q", expect, got)
+			}
+		})
+	})
+}
+
 func compareTranslations(expected, got map[string]uint64) error {
 	for key, id := range got {
 		if realID, ok := expected[key]; !ok {