rework and consolidate view name cleanup

We had two different versions of this, and a comment referring to a third
which doesn't exist, so I've consolidated them and made them slightly
pickier, to avoid problems like the one I ran into developing the existence
tracking where one of these optimistically transformed names it actually
shouldn't have. Now if we don't expect a view name, we yield an error,
rather than silently performing a transformation.

This also implies updating the ImportRoaring_MultiView test to use
two valid view names.

Author: seebs

api.go

@@ -499,22 +499,9 @@ func importWorker(importWork chan importJob) {
 	for j := range importWork {
 		err := func() (err0 error) {
 			for viewName, viewData := range j.req.Views {
-				// The logic here corresponds to the logic in fragment.cleanViewName().
-				// Unfortunately, the logic in that method is not completely exclusive
-				// (i.e. an "other" view named with format YYYYMMDD would be handled
-				// incorrectly). One way to address this would be to change the logic
-				// overall so there weren't conflicts. For now, we just
-				// rely on the field type to inform the intended view name.
-				// contrast with cleanupView, which is similar but unfortunately not quite identical
-				switch viewName {
-				case "":
-					viewName = viewStandard
-				case viewStandard, viewExistence:
-					// do nothing, these are fine
-				default: // possibly a time view
-					if j.field.Type() == FieldTypeTime && !strings.HasPrefix(viewName, viewStandard) {
-						viewName = viewStandard + "_" + viewName
-					}
+				viewName, err0 = j.field.cleanupViewName(viewName)
+				if err0 != nil {
+					return err0
 				}
 				if len(viewData) == 0 {
 					return fmt.Errorf("no data to import for view: %s", viewName)
@@ -1687,7 +1674,7 @@ func (api *API) ImportRoaringShard(ctx context.Context, indexName string, shard
 		}
 
 		fieldType := field.Options().Type
-		if err1 = cleanupView(fieldType, &viewUpdate); err1 != nil {
+		if viewUpdate.View, err1 = field.cleanupViewName(viewUpdate.View); err1 != nil {
 			return err1
 		}
 
@@ -1779,30 +1766,6 @@ func (api *API) ImportRoaringShard(ctx context.Context, indexName string, shard
 	return nil
 }
 
-func cleanupView(fieldType string, viewUpdate *RoaringUpdate) error {
-	// TODO wouldn't hurt to have consolidated logic somewhere for validating view names.
-	switch fieldType {
-	case FieldTypeSet, FieldTypeTime:
-		switch viewUpdate.View {
-		case "":
-			viewUpdate.View = viewStandard
-		case viewStandard, viewExistence:
-			// do nothing, these are fine
-		default:
-			if fieldType == FieldTypeTime && !strings.HasPrefix(viewUpdate.View, viewStandard) {
-				viewUpdate.View = viewStandard + "_" + viewUpdate.View
-			}
-		}
-	case FieldTypeInt, FieldTypeDecimal, FieldTypeTimestamp:
-		if viewUpdate.View == "" {
-			viewUpdate.View = "bsig_" + viewUpdate.Field
-		} else if viewUpdate.View != "bsig_"+viewUpdate.Field {
-			return NewBadRequestError(errors.Errorf("invalid view name (%s) for field %s of type %s", viewUpdate.View, viewUpdate.Field, fieldType))
-		}
-	}
-	return nil
-}
-
 // ImportValue is a wrapper around the common code in ImportValueWithTx, which
 // currently just translates req.Clear into a clear ImportOption.
 func (api *API) ImportValue(ctx context.Context, qcx *Qcx, req *ImportValueRequest, opts ...ImportOption) error {

field.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 	"sync"
 	"time"
+	"unicode"
 
 	"github.com/featurebasedb/featurebase/v3/pql"
 	"github.com/featurebasedb/featurebase/v3/roaring"
@@ -982,6 +983,51 @@ func (f *Field) hasBSIGroup(name string) bool {
 	return false
 }
 
+// cleanupViewName yields a "corrected" view name, handling some
+// idioms we used elsewhere in code. Given an empty string,
+// it yields a default view name (either "standard" or the BSI view
+// for BSI fields). Given a string starting with numbers, it
+// yields the corresponding time quantum view (prefixing "standard_").
+// It yields an error if the view name given does not correspond
+// to a view which should exist. For instance, the "standard" or
+// "existence" views for a BSI field, or a time quantum view for
+// a non-time field.
+func (f *Field) cleanupViewName(viewName string) (string, error) {
+	if viewName == "" {
+		switch f.options.Type {
+		case FieldTypeInt, FieldTypeDecimal, FieldTypeTimestamp:
+			return "bsig_" + f.name, nil
+		default:
+			return viewStandard, nil
+		}
+	}
+	switch f.options.Type {
+	case FieldTypeInt, FieldTypeDecimal, FieldTypeTimestamp:
+		if viewName == "bsig_"+f.name {
+			return viewName, nil
+		}
+		return viewName, fmt.Errorf("BSI-type field view should be named bsig_[fieldname], got %q", viewName)
+	case FieldTypeTime:
+		switch {
+		case viewName == viewStandard, viewName == viewExistence:
+			return viewName, nil
+		case strings.HasPrefix(viewName, viewStandard):
+			return viewName, nil
+		case unicode.IsDigit(rune(viewName[0])):
+			return viewStandard + "_" + viewName, nil
+		default:
+			return viewName, fmt.Errorf("time field views are %q, %q, or %q_[digits], got %q", viewStandard, viewExistence, viewStandard, viewName)
+		}
+	default:
+		switch viewName {
+		case viewStandard, viewExistence:
+			return viewName, nil
+		default:
+			return viewName, fmt.Errorf("unexpected view name %q, expecting %q or %q", viewName, viewStandard, viewExistence)
+		}
+	}
+}
+
 // createBSIGroup creates a new bsiGroup on the field.
 func (f *Field) createBSIGroup(bsig *bsiGroup) error {
 	// Append bsiGroup.

internal_client_test.go

@@ -664,8 +664,8 @@ func TestClient_ImportRoaring_MultiView(t *testing.T) {
 	host := cluster.GetNode(0).URL()
 	c := MustNewClient(host, pilosa.GetHTTPClient(nil))
 	req := &pilosa.ImportRoaringRequest{Views: map[string][]byte{}}
-	req.Views["a"], _ = hex.DecodeString("3B3001000100000900010000000100010009000100")
-	req.Views["b"], _ = hex.DecodeString("3B3001000100000900010000000100010009000100")
+	req.Views["standard"], _ = hex.DecodeString("3B3001000100000900010000000100010009000100")
+	req.Views["existence"], _ = hex.DecodeString("3B3001000100000900010000000100010009000100")
 	if err := c.ImportRoaring(context.Background(), &cluster.GetNode(0).API.Node().URI, cluster.Idx(), "f", 0, false, req); err != nil {
 		t.Fatal(err)
 	}