From dd4227f5e365b1fe2351f7ee85d9a09f3a01e850 Mon Sep 17 00:00:00 2001
From: Ben Johnson <benbjohnson@yahoo.com>
Date: Mon, 13 May 2019 13:51:54 -0600
Subject: [PATCH] Improve TopN() errors

This commit improves field not found, integer field, and cache errors
for the `TopN()` command.
---
 executor.go      | 15 ++++++++----
 executor_test.go | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/executor.go b/executor.go
index 07a83496d..607840a42 100644
--- a/executor.go
+++ b/executor.go
@@ -766,11 +766,14 @@ func (e *executor) executeTopNShard(ctx context.Context, index string, c *pql.Ca
 	span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executeTopNShard")
 	defer span.Finish()
 
-	field, _ := c.Args["_field"].(string)
+	fieldName, _ := c.Args["_field"].(string)
 	n, _, err := c.UintArg("n")
 	if err != nil {
 		return nil, fmt.Errorf("executeTopNShard: %v", err)
+	} else if f := e.Holder.Field(index, fieldName); f != nil && f.Type() == FieldTypeInt {
+		return nil, fmt.Errorf("cannot compute TopN() on integer field: %q", fieldName)
 	}
+
 	attrName, _ := c.Args["attrName"].(string)
 	rowIDs, _, err := c.UintSliceArg("ids")
 	if err != nil {
@@ -799,13 +802,15 @@ func (e *executor) executeTopNShard(ctx context.Context, index string, c *pql.Ca
 	}
 
 	// Set default field.
-	if field == "" {
-		field = defaultField
+	if fieldName == "" {
+		fieldName = defaultField
 	}
 
-	f := e.Holder.fragment(index, field, viewStandard, shard)
+	f := e.Holder.fragment(index, fieldName, viewStandard, shard)
 	if f == nil {
 		return nil, nil
+	} else if f.CacheType == CacheTypeNone {
+		return nil, fmt.Errorf("cannot compute TopN(), field has no cache: %q", fieldName)
 	}
 
 	if minThreshold == 0 {
@@ -2623,7 +2628,7 @@ func (e *executor) translateResult(index string, idx *Index, call *pql.Call, res
 		if fieldName := callArgString(call, "_field"); fieldName != "" {
 			field := idx.Field(fieldName)
 			if field == nil {
-				return nil, ErrFieldNotFound
+				return nil, fmt.Errorf("field %q not found", fieldName)
 			}
 			if field.keys() {
 				other := make([]Pair, len(result))
diff --git a/executor_test.go b/executor_test.go
index 60a92db2f..3565c40fd 100644
--- a/executor_test.go
+++ b/executor_test.go
@@ -1058,6 +1058,65 @@ func TestExecutor_Execute_TopN(t *testing.T) {
 			t.Fatal(diff)
 		}
 	})
+
+	t.Run("ErrFieldNotFound", func(t *testing.T) {
+		c := test.MustRunCluster(t, 1)
+		defer c.Close()
+		hldr := test.Holder{Holder: c[0].Server.Holder()}
+
+		// Set data on the "f" field.
+		if idx, err := hldr.CreateIndex("i", pilosa.IndexOptions{}); err != nil {
+			t.Fatal(err)
+		} else if _, err := idx.CreateField("f", pilosa.OptFieldTypeDefault()); err != nil {
+			t.Fatal(err)
+		} else if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `
+			Set(0, f=0)
+			Set(0, f=1)
+		`}); err != nil {
+			t.Fatal(err)
+		} else if err := c[0].RecalculateCaches(); err != nil {
+			t.Fatalf("recalculating caches: %v", err)
+		}
+
+		// Attempt to query the "g" field.
+		if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `TopN(g, n=2)`}); err == nil || err.Error() != `executing: field "g" not found` {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+
+	t.Run("ErrBSIField", func(t *testing.T) {
+		c := test.MustRunCluster(t, 1)
+		defer c.Close()
+		hldr := test.Holder{Holder: c[0].Server.Holder()}
+
+		// Create BSI "f" field.
+		if idx, err := hldr.CreateIndex("i", pilosa.IndexOptions{}); err != nil {
+			t.Fatal(err)
+		} else if _, err := idx.CreateField("f", pilosa.OptFieldTypeInt(0, 100)); err != nil {
+			t.Fatal(err)
+		} else if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `TopN(f, n=2)`}); err == nil || err.Error() != `executing: finding top results: cannot compute TopN() on integer field: "f"` {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+
+	t.Run("ErrCacheNone", func(t *testing.T) {
+		c := test.MustRunCluster(t, 1)
+		defer c.Close()
+		hldr := test.Holder{Holder: c[0].Server.Holder()}
+
+		if idx, err := hldr.CreateIndex("i", pilosa.IndexOptions{}); err != nil {
+			t.Fatal(err)
+		} else if _, err := idx.CreateField("f", pilosa.OptFieldTypeSet(pilosa.CacheTypeNone, 0)); err != nil {
+			t.Fatal(err)
+		} else if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `
+			Set(0, f=0)
+			Set(0, f=1)
+		`}); err != nil {
+			t.Fatal(err)
+		} else if _, err := c[0].API.Query(context.Background(), &pilosa.QueryRequest{Index: "i", Query: `TopN(f, n=2)`}); err == nil || err.Error() != `executing: finding top results: cannot compute TopN(), field has no cache: "f"` {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
 }
 
 func TestExecutor_Execute_TopN_fill(t *testing.T) {