From f7fb9f386ee92261b3639da40dff6ae31fb4f352 Mon Sep 17 00:00:00 2001 From: Todd Gruben Date: Tue, 15 Mar 2022 01:52:06 -0500 Subject: [PATCH 1/4] decimal wowes --- ctl/restore.go | 9 +++++++++ http_handler.go | 17 +++++++++++------ index.go | 14 ++++++++++++++ 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/ctl/restore.go b/ctl/restore.go index 0c8cb51b0..2a88a8eca 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -19,8 +19,10 @@ import ( pilosa "github.com/molecula/featurebase/v3" "github.com/molecula/featurebase/v3/logger" + "github.com/molecula/featurebase/v3/pql" "github.com/molecula/featurebase/v3/server" "github.com/molecula/featurebase/v3/topology" + "github.com/molecula/featurebase/v3/vprint" "github.com/pkg/errors" "golang.org/x/sync/errgroup" ) @@ -192,6 +194,13 @@ func (cmd *RestoreCommand) restoreSchema(ctx context.Context, primary *topology. } for _, field := range index.Fields { logger.Printf("Create Field %v", field.Name) + if field.Options.Type == pilosa.FieldTypeDecimal { + min, max := pql.MinMax(field.Options.Scale) + scale := field.Options.Scale + vprint.VV("setting MinMax %v %s %s", scale, min, max) + field.Options.Max = max + field.Options.Min = min + } err = cmd.client.CreateFieldWithOptions(ctx, index.Name, field.Name, field.Options) if err != nil { return err diff --git a/http_handler.go b/http_handler.go index 9dc5319ae..5ef030f50 100644 --- a/http_handler.go +++ b/http_handler.go @@ -1576,14 +1576,19 @@ func fieldOptionsToFunctionalOpts(opt fieldOptions) []FieldOption { scale := int64(0) if opt.Scale != nil { scale = *opt.Scale - } - if opt.Min == nil { - min := pql.NewDecimal(int64(math.MinInt64), scale) + min, max := pql.MinMax(scale) opt.Min = &min - } - if opt.Max == nil { - max := pql.NewDecimal(int64(math.MaxInt64), scale) opt.Max = &max + } else { + + if opt.Min == nil { + min := pql.NewDecimal(int64(math.MinInt64), scale) + opt.Min = &min + } + if opt.Max == nil { + max := pql.NewDecimal(int64(math.MaxInt64), scale) + opt.Max = &max + } } var minmax []pql.Decimal if opt.Min != nil { diff --git a/index.go b/index.go index 12e0aee68..08a4a6a4a 100644 --- a/index.go +++ b/index.go @@ -4,6 +4,7 @@ package pilosa import ( "context" "fmt" + "math" "os" "path/filepath" "sort" @@ -11,6 +12,7 @@ import ( "sync" "github.com/molecula/featurebase/v3/disco" + "github.com/molecula/featurebase/v3/pql" "github.com/molecula/featurebase/v3/roaring" "github.com/molecula/featurebase/v3/stats" "github.com/molecula/featurebase/v3/testhook" @@ -613,6 +615,18 @@ func (i *Index) CreateFieldIfNotExistsWithOptions(name string, opt *FieldOptions if f := i.fields[name]; f != nil { return f, nil } + // added for backward compatablity with old schemas + if opt != nil && opt.Type == FieldTypeDecimal { + min, max := pql.MinMax(opt.Scale) + opt.Max = max + opt.Min = min + } + if opt != nil && opt.Type == FieldTypeDecimal { + min := pql.NewDecimal(int64(math.MinInt64), 0) + max := pql.NewDecimal(int64(math.MaxInt64), 0) + opt.Max = max + opt.Min = min + } cfm := &CreateFieldMessage{ Index: i.name, From 739fd9b04beccfb270f9cfe4740adbfd53c476f8 Mon Sep 17 00:00:00 2001 From: Todd Gruben Date: Tue, 15 Mar 2022 09:33:27 -0500 Subject: [PATCH 2/4] ensure provided min/max are valid --- http_handler.go | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/http_handler.go b/http_handler.go index 5ef030f50..2815cdbf8 100644 --- a/http_handler.go +++ b/http_handler.go @@ -1577,18 +1577,29 @@ func fieldOptionsToFunctionalOpts(opt fieldOptions) []FieldOption { if opt.Scale != nil { scale = *opt.Scale min, max := pql.MinMax(scale) - opt.Min = &min - opt.Max = &max - } else { - - if opt.Min == nil { - min := pql.NewDecimal(int64(math.MinInt64), scale) - opt.Min = &min - } - if opt.Max == nil { - max := pql.NewDecimal(int64(math.MaxInt64), scale) + // ensure the provided bounds are valid + if opt.Max != nil && max.LessThan(*opt.Max) { opt.Max = &max } + if opt.Max == nil { + opt.Max = &max + } + + if opt.Min != nil && min.GreaterThan(*opt.Min) { + opt.Min = &min + } + if opt.Min == nil { + opt.Min = &min + } + } + + if opt.Min == nil { + min := pql.NewDecimal(int64(math.MinInt64), scale) + opt.Min = &min + } + if opt.Max == nil { + max := pql.NewDecimal(int64(math.MaxInt64), scale) + opt.Max = &max } var minmax []pql.Decimal if opt.Min != nil { From 8c3c774492ee94882578985b5af559b522c12bfc Mon Sep 17 00:00:00 2001 From: Todd Gruben Date: Tue, 15 Mar 2022 10:29:48 -0500 Subject: [PATCH 3/4] ensure provided min/max are valid on int fields --- http_handler.go | 15 +++++++++++---- index.go | 22 +++++++++++++++------- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/http_handler.go b/http_handler.go index 2815cdbf8..07385f342 100644 --- a/http_handler.go +++ b/http_handler.go @@ -1563,14 +1563,21 @@ func fieldOptionsToFunctionalOpts(opt fieldOptions) []FieldOption { case FieldTypeSet: fos = append(fos, OptFieldTypeSet(*opt.CacheType, *opt.CacheSize)) case FieldTypeInt: - if opt.Min == nil { - min := pql.NewDecimal(int64(math.MinInt64), 0) - opt.Min = &min + min, max := pql.MinMax(0) + // ensure the provided bounds are valid + if opt.Max != nil && max.LessThan(*opt.Max) { + opt.Max = &max } if opt.Max == nil { - max := pql.NewDecimal(int64(math.MaxInt64), 0) opt.Max = &max } + + if opt.Min != nil && min.GreaterThan(*opt.Min) { + opt.Min = &min + } + if opt.Min == nil { + opt.Min = &min + } fos = append(fos, OptFieldTypeInt(opt.Min.ToInt64(0), opt.Max.ToInt64(0))) case FieldTypeDecimal: scale := int64(0) diff --git a/index.go b/index.go index 08a4a6a4a..21686b04b 100644 --- a/index.go +++ b/index.go @@ -4,7 +4,6 @@ package pilosa import ( "context" "fmt" - "math" "os" "path/filepath" "sort" @@ -618,14 +617,23 @@ func (i *Index) CreateFieldIfNotExistsWithOptions(name string, opt *FieldOptions // added for backward compatablity with old schemas if opt != nil && opt.Type == FieldTypeDecimal { min, max := pql.MinMax(opt.Scale) - opt.Max = max - opt.Min = min + // ensure the provided bounds are valid + if max.LessThan(opt.Max) { + opt.Max = max + } + if min.GreaterThan(opt.Min) { + opt.Min = min + } } if opt != nil && opt.Type == FieldTypeDecimal { - min := pql.NewDecimal(int64(math.MinInt64), 0) - max := pql.NewDecimal(int64(math.MaxInt64), 0) - opt.Max = max - opt.Min = min + min, max := pql.MinMax(0) + // ensure the provided bounds are valid + if max.LessThan(opt.Max) { + opt.Max = max + } + if min.GreaterThan(opt.Min) { + opt.Min = min + } } cfm := &CreateFieldMessage{ From dfe052aa1a7ce5bec6527bb48e769bdf4c17dbf2 Mon Sep 17 00:00:00 2001 From: Todd Gruben Date: Tue, 15 Mar 2022 11:01:03 -0500 Subject: [PATCH 4/4] increase test coverage --- ctl/restore.go | 9 --------- server/handler_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/ctl/restore.go b/ctl/restore.go index 2a88a8eca..0c8cb51b0 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -19,10 +19,8 @@ import ( pilosa "github.com/molecula/featurebase/v3" "github.com/molecula/featurebase/v3/logger" - "github.com/molecula/featurebase/v3/pql" "github.com/molecula/featurebase/v3/server" "github.com/molecula/featurebase/v3/topology" - "github.com/molecula/featurebase/v3/vprint" "github.com/pkg/errors" "golang.org/x/sync/errgroup" ) @@ -194,13 +192,6 @@ func (cmd *RestoreCommand) restoreSchema(ctx context.Context, primary *topology. } for _, field := range index.Fields { logger.Printf("Create Field %v", field.Name) - if field.Options.Type == pilosa.FieldTypeDecimal { - min, max := pql.MinMax(field.Options.Scale) - scale := field.Options.Scale - vprint.VV("setting MinMax %v %s %s", scale, min, max) - field.Options.Max = max - field.Options.Min = min - } err = cmd.client.CreateFieldWithOptions(ctx, index.Name, field.Name, field.Options) if err != nil { return err diff --git a/server/handler_test.go b/server/handler_test.go index 2b17d7bd3..1f8225bba 100644 --- a/server/handler_test.go +++ b/server/handler_test.go @@ -886,6 +886,37 @@ func TestHandler_Endpoints(t *testing.T) { } } }) + t.Run("Query decimal field scale only", func(t *testing.T) { + w := httptest.NewRecorder() + fieldName := "f-decimal-scale-only" + h.ServeHTTP(w, test.MustNewHTTPRequest("POST", fmt.Sprintf("/index/i0/field/%s", fieldName), + strings.NewReader(`{"options":{"type":"decimal", "scale": 2}}`))) + if w.Code != gohttp.StatusOK { + fmt.Println(w.Body.String()) + t.Fatalf("unexpected status code: %d", w.Code) + } + w = httptest.NewRecorder() + h.ServeHTTP(w, test.MustNewHTTPRequest("GET", "/schema", strings.NewReader(""))) + if w.Code != gohttp.StatusOK { + t.Fatalf("unexpected status code: %d", w.Code) + } + rsp := getSchemaResponse{} + if err := json.Unmarshal(w.Body.Bytes(), &rsp); err != nil { + t.Fatalf("json decode: %s", err) + } + field := rsp.findField("i0", fieldName) + if field == nil { + t.Fatalf("field not found: %s", fieldName) + } + if field != nil { // happy linter + if !reflect.DeepEqual(pql.NewDecimal(math.MinInt64, 2), field.Options.Min) { + t.Fatalf("field min %d != %d", pql.NewDecimal(math.MinInt64, 1), field.Options.Min) + } + if !reflect.DeepEqual(pql.NewDecimal(math.MaxInt64, 2), field.Options.Max) { + t.Fatalf("field min %d != %d", pql.NewDecimal(math.MaxInt64, 2), field.Options.Max) + } + } + }) // Ensure that decimal fields error when scale is not provided. t.Run("Query decimal field scale error", func(t *testing.T) {