From a3a0de2b0a3b5422a832e504dcbb21a1efca9194 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 20 Mar 2023 10:37:59 -0500 Subject: [PATCH] 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. --- api.go | 45 ++++------------------------------------ field.go | 46 +++++++++++++++++++++++++++++++++++++++++ internal_client_test.go | 4 ++-- 3 files changed, 52 insertions(+), 43 deletions(-) diff --git a/api.go b/api.go index c8f792992..a46d8da36 100644 --- a/api.go +++ b/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 { diff --git a/field.go b/field.go index f8e3251e3..8ff7d3ed2 100644 --- a/field.go +++ b/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. diff --git a/internal_client_test.go b/internal_client_test.go index e5d22285c..fa8f8aa56 100644 --- a/internal_client_test.go +++ b/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) }