From 881d3bef06991f4590b696ef40e630ef4201b5ed Mon Sep 17 00:00:00 2001 From: Travis Date: Mon, 6 Jan 2020 23:19:38 -0600 Subject: [PATCH] Adjust the FieldOption logic to be in place prior to field.Open(). This commit changes the order of FieldOption application so that it's always set before field.Open() is called. This was required because field.Open() now uses some of the values from FieldOptions to determine if/when to use a particular translateStore. For example, when FieldOptions.ForeignIndex is set, the translateStore from the foreign index is retrieved during field.Open(). --- api.go | 11 +++---- ctl/import.go | 2 ++ executor.go | 12 -------- field.go | 68 +++++++++++++++++++++++++++++++++--------- field_internal_test.go | 2 +- index.go | 16 +++++----- 6 files changed, 68 insertions(+), 43 deletions(-) diff --git a/api.go b/api.go index a3fdc3b93..978f169f7 100644 --- a/api.go +++ b/api.go @@ -1084,13 +1084,10 @@ func (api *API) ImportValue(ctx context.Context, req *ImportValueRequest, opts . req.Shard = math.MaxUint64 } - // Translate values when the field has a ForeignIndex with keys. - if fidx := field.Options().ForeignIndex; fidx != "" { - foreignIndex := api.holder.Index(fidx) - if foreignIndex == nil { - return errors.Errorf("foreign index does not exist: %s", fidx) - } - uints, err := foreignIndex.translateStore.TranslateKeys(req.StringValues) + // Translate values when the field uses keys (for example, when + // the field has a ForeignIndex with keys). + if field.keys() { + uints, err := field.translateStore.TranslateKeys(req.StringValues) if err != nil { return errors.Wrap(err, "translating string values") } diff --git a/ctl/import.go b/ctl/import.go index 208eede07..32a49be49 100644 --- a/ctl/import.go +++ b/ctl/import.go @@ -108,6 +108,8 @@ func (cmd *ImportCommand) Run(ctx context.Context) error { cmd.FieldOptions.Type = pilosa.FieldTypeInt } else { cmd.FieldOptions.Type = pilosa.FieldTypeSet + cmd.FieldOptions.CacheType = pilosa.CacheTypeRanked + cmd.FieldOptions.CacheSize = pilosa.DefaultCacheSize } } err := cmd.ensureSchema(ctx) diff --git a/executor.go b/executor.go index a17897793..0b4d7d8e9 100644 --- a/executor.go +++ b/executor.go @@ -3623,18 +3623,6 @@ func (e *executor) translateCall(index string, idx *Index, c *pql.Call) error { if err := e.translateRowKey(c, field.translateStore, rowKey); err != nil { return errors.Wrap(err, "translating rowkey") } - } else if field.Options().ForeignIndex != "" { - // Get the foreign index. - fidx := field.Options().ForeignIndex - foreignIndex := e.Holder.Index(fidx) - if foreignIndex == nil { - return errors.Errorf("foreign index does not exist: %s", fidx) - } - if foreignIndex.Keys() { - if err := e.translateRowKey(c, foreignIndex.translateStore, rowKey); err != nil { - return errors.Wrap(err, "translating rowkey") - } - } } else { if isString(c.Args[rowKey]) { return errors.New("string 'row' value not allowed unless field 'keys' option enabled") diff --git a/field.go b/field.go index ffaacb39a..24249ee80 100644 --- a/field.go +++ b/field.go @@ -84,6 +84,15 @@ type Field struct { // Field options. options FieldOptions + // finalOptions is used with a final call to applyOptions. + // The initial call to applyOptions is made with options + // loaded from the meta file on disk (in the case when + // a field is being re-opened). If the field creator calls + // setOptions before calling Open(), then those options + // will be held in finalOptions, and applied instead of + // those from the meta file. + finalOptions *FieldOptions + bsiGroups []*bsiGroup // Shards with data on any node in the cluster, according to this node. @@ -94,6 +103,15 @@ type Field struct { snapshotQueue snapshotQueue // Instantiates new translation store on open. OpenTranslateStore OpenTranslateStoreFunc + + // Used for looking up a foreign index. + holder *Holder + + // Stores whether or not the field has keys enabled. + // This is most helpful for cases where the keys are + // based on a foreign index; this prevents having to + // call holder.index.Keys() every time. + usesKeys bool } // FieldOption is a functional option type for pilosa.fieldOptions. @@ -276,7 +294,7 @@ func newField(path, index, name string, opts FieldOption) (*Field, error) { broadcaster: NopBroadcaster, Stats: stats.NopStatsClient, - options: applyDefaultOptions(fo), + options: *applyDefaultOptions(&fo), remoteAvailableShards: roaring.NewBitmap(), @@ -473,7 +491,13 @@ func (f *Field) Open() error { return errors.Wrap(err, "loading available shards") } - // Apply the field options loaded from meta. + // If options were provided using setOptions(), then + // use those instead of the options from the meta file. + if f.finalOptions != nil { + f.options = *f.finalOptions + } + + // Apply the field options loaded from meta (or set via setOptions()). f.logger.Debugf("apply options for index/field: %s/%s", f.index, f.name) if err := f.applyOptions(f.options); err != nil { return errors.Wrap(err, "applying options") @@ -489,9 +513,22 @@ func (f *Field) Open() error { return errors.Wrap(err, "opening attrstore") } - // Instantiate & open translation store. - if f.translateStore, err = f.OpenTranslateStore(filepath.Join(f.path, "keys"), f.index, f.name); err != nil { - return errors.Wrap(err, "opening translate store") + // If the field has a foreign index, and that index uses keys, + // then use that index's translateStore instead. + if f.options.ForeignIndex != "" { + foreignIndex := f.holder.Index(f.options.ForeignIndex) + if foreignIndex == nil { + return errors.Errorf("foreign index does not exist: %s", f.options.ForeignIndex) + } else if foreignIndex.Keys() { + f.usesKeys = true + f.translateStore = foreignIndex.translateStore + } + } else { + // Instantiate & open translation store. + if f.translateStore, err = f.OpenTranslateStore(filepath.Join(f.path, "keys"), f.index, f.name); err != nil { + return errors.Wrap(err, "opening translate store") + } + f.usesKeys = f.options.Keys } return nil @@ -641,6 +678,11 @@ func (f *Field) saveMeta() error { return nil } +// setOptions saves options for final application during Open(). +func (f *Field) setOptions(opts *FieldOptions) { + f.finalOptions = applyDefaultOptions(opts) +} + // applyOptions configures the field based on opt. func (f *Field) applyOptions(opt FieldOptions) error { switch opt.Type { @@ -760,7 +802,7 @@ func (f *Field) Close() error { func (f *Field) keys() bool { f.mu.RLock() defer f.mu.RUnlock() - return f.options.Keys + return f.usesKeys } // bsiGroup returns a bsiGroup by name. @@ -1604,15 +1646,13 @@ type FieldOptions struct { ForeignIndex string `json:"foreignIndex"` } -// applyDefaultOptions returns a new FieldOptions object -// with default values if o does not contain a valid type. -func applyDefaultOptions(o FieldOptions) FieldOptions { +// applyDefaultOptions updates FieldOptions with the default +// values if o does not contain a valid type. +func applyDefaultOptions(o *FieldOptions) *FieldOptions { if o.Type == "" { - return FieldOptions{ - Type: DefaultFieldType, - CacheType: DefaultCacheType, - CacheSize: DefaultCacheSize, - } + o.Type = DefaultFieldType + o.CacheType = DefaultCacheType + o.CacheSize = DefaultCacheSize } return o } diff --git a/field_internal_test.go b/field_internal_test.go index 41cb5832b..fd8786246 100644 --- a/field_internal_test.go +++ b/field_internal_test.go @@ -516,7 +516,7 @@ func TestField_ApplyOptions(t *testing.T) { } { fld := &Field{} - fld.options = applyDefaultOptions(FieldOptions{}) + fld.options = *applyDefaultOptions(&FieldOptions{}) if err := fld.applyOptions(tt.opts); err != nil { t.Fatal(err) diff --git a/index.go b/index.go index a6a9e3070..ed05ca6bc 100644 --- a/index.go +++ b/index.go @@ -61,6 +61,7 @@ type Index struct { snapshotQueue snapshotQueue // Used for notifying holder when a field is added. + // Also passed to field for foreign-index lookup. holder *Holder // Instantiates new translation stores for fields. @@ -426,20 +427,17 @@ func (i *Index) createField(name string, opt FieldOptions) (*Field, error) { return nil, errors.Wrap(err, "initializing") } + // Pass holder through to the field for use in looking + // up a foreign index. + f.holder = i.holder + + f.setOptions(&opt) + // Open field. if err := f.Open(); err != nil { return nil, errors.Wrap(err, "opening") } - // Apply field options. - // This is already happening in f.Open() just before this, but in - // that case, the options being applied are those read from the meta - // file on disk. - if err := f.applyOptions(opt); err != nil { - f.Close() - return nil, errors.Wrap(err, "applying options") - } - if err := f.saveMeta(); err != nil { f.Close() return nil, errors.Wrap(err, "saving meta")