From 0ba5b48fca0c07bafab572f7ae370bd6add37ae9 Mon Sep 17 00:00:00 2001 From: Travis Date: Thu, 16 Jan 2020 22:23:29 -0600 Subject: [PATCH 1/6] Field.ForeignIndex translation on ImportValue() --- api.go | 36 +++++++++++++++++++++++++++--------- executor.go | 2 +- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/api.go b/api.go index 2a74f3b11..666bb4692 100644 --- a/api.go +++ b/api.go @@ -1086,18 +1086,36 @@ func (api *API) ImportValue(ctx context.Context, req *ImportValueRequest, opts . req.Shard = math.MaxUint64 } + // Determine if foreign index is being used for translation. + useKeys := field.Keys() + foreignIndexName := field.ForeignIndex() + if foreignIndexName != "" { + foreignIndex := api.holder.indexes[foreignIndexName] + if foreignIndex == nil { + return errors.Errorf("foreign index not found: %q", foreignIndexName) + } + useKeys = foreignIndex.Keys() + } + // 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") + if useKeys { + keySet := make(map[string]struct{}) + for i := range req.StringValues { + keySet[req.StringValues[i]] = struct{}{} } - // Because the BSI field supports negative value, we have to - // convert the slice of uint64 keys to a slice of int64. - ints := make([]int64, len(uints)) - for i := range uints { - ints[i] = int64(uints[i]) + + // Perform a separate batch translation for each separate index used. + keyMap := make(map[string]uint64) + if keyMap, err = api.cluster.translateIndexKeySet(ctx, foreignIndexName, keySet); err != nil { + return err + } + + // Because the BSI field supports negative values, we have to + // convert the uint64 keys to a slice of int64. + ints := make([]int64, len(req.StringValues)) + for i := range req.StringValues { + ints[i] = int64(keyMap[req.StringValues[i]]) } req.Values = ints } diff --git a/executor.go b/executor.go index 7b79fd11c..b6120662e 100644 --- a/executor.go +++ b/executor.go @@ -3544,7 +3544,7 @@ func (e *executor) translateCalls(ctx context.Context, defaultIndexName string, for indexName, keySet := range keySets { idx := e.Holder.indexes[indexName] if idx == nil { - return fmt.Errorf("canot find index %q", indexName) + return fmt.Errorf("cannot find index %q", indexName) } if !idx.Keys() || len(keySets) == 0 { From 90a2e116a7cdfc9073eec7ede0467a60b7172d7c Mon Sep 17 00:00:00 2001 From: Travis Date: Fri, 17 Jan 2020 11:10:42 -0600 Subject: [PATCH 2/6] update translateResult to translate foreign index keys on SignedRow results --- executor.go | 57 +++++++++++++++++++++++++++++------------------- executor_test.go | 30 ++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/executor.go b/executor.go index b6120662e..e3a064754 100644 --- a/executor.go +++ b/executor.go @@ -3857,34 +3857,47 @@ func (e *executor) translateResult(index string, idx *Index, call *pql.Call, res // make the return type for an int field with a ForeignIndex be // a *Row instead (because it should always be positive). case SignedRow: - var store TranslateStore + sr, err := func() (*SignedRow, error) { + fieldName := callArgString(call, "field") + if fieldName == "" { + return nil, nil + } - if fieldName := callArgString(call, "field"); fieldName != "" { field := idx.Field(fieldName) - if field != nil && field.Keys() { - store = field.TranslateStore() + if field == nil { + return nil, nil } - } - // In the case where a field/foreignIndex doesn't exist, - // fall back to using the index translateStore. - if store == nil && idx.Keys() { - store = nil // TODO: this may need to be idx.TranslateStore(?) - } - - if store != nil { - rslt := result.Pos - other := &Row{Attrs: rslt.Attrs} - for _, segment := range rslt.Segments() { - for _, col := range segment.Columns() { - key, err := store.TranslateID(col) - if err != nil { - return nil, err - } - other.Keys = append(other.Keys, key) + // Determine if foreign index is being used for translation. + useKeys := field.Keys() + foreignIndexName := field.ForeignIndex() + if foreignIndexName != "" { + foreignIndex := e.Holder.indexes[foreignIndexName] + if foreignIndex == nil { + return nil, errors.Errorf("foreign index not found: %q", foreignIndexName) } + useKeys = foreignIndex.Keys() } - return SignedRow{Pos: other}, nil + + if useKeys { + rslt := result.Pos + other := &Row{Attrs: rslt.Attrs} + for _, segment := range rslt.Segments() { + keys, err := e.Cluster.translateIndexIDs(context.Background(), foreignIndexName, segment.Columns()) + if err != nil { + return nil, errors.Wrap(err, "translating index ids") + } + other.Keys = append(other.Keys, keys...) + } + return &SignedRow{Pos: other}, nil + } + + return nil, nil + }() + if err != nil { + return nil, err + } else if sr != nil { + return *sr, nil } case PairField: diff --git a/executor_test.go b/executor_test.go index afd322eb6..0ec94efe4 100644 --- a/executor_test.go +++ b/executor_test.go @@ -3898,7 +3898,7 @@ func TestExecutor_ForeignIndex(t *testing.T) { `) distinct := c.Query(t, "child", `Distinct(index="child", field="parent_id")`).Results[0].(pilosa.SignedRow) - if !reflect.DeepEqual(distinct.Pos.Keys, []string{"one", "two", "twenty-one"}) { + if !sameStringSlice(distinct.Pos.Keys, []string{"one", "two", "twenty-one"}) { t.Fatalf("unexpected keys: %v", distinct.Pos.Keys) } @@ -3918,6 +3918,34 @@ func TestExecutor_ForeignIndex(t *testing.T) { } } +// sameStringSlice is a helper function which compares two string +// slices without enforcing order. +func sameStringSlice(x, y []string) bool { + if len(x) != len(y) { + return false + } + // create a map of string -> int + diff := make(map[string]int, len(x)) + for _, _x := range x { + // 0 value for int is 0, so just increment a counter for the string + diff[_x]++ + } + for _, _y := range y { + // If the string _y is not in diff bail out early + if _, ok := diff[_y]; !ok { + return false + } + diff[_y] -= 1 + if diff[_y] == 0 { + delete(diff, _y) + } + } + if len(diff) == 0 { + return true + } + return false +} + func TestExecutor_Execute_GroupBy(t *testing.T) { groupByTest := func(t *testing.T, clusterSize int) { c := test.MustRunCluster(t, 1) From efffa39c2c0c751aa6eccff0ea02ea50fa766a3d Mon Sep 17 00:00:00 2001 From: Travis Date: Fri, 17 Jan 2020 11:42:21 -0600 Subject: [PATCH 3/6] check foreign index on field open --- field.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/field.go b/field.go index 149ae65c3..86cf50b64 100644 --- a/field.go +++ b/field.go @@ -526,6 +526,14 @@ func (f *Field) Open() error { return errors.Wrap(err, "applying translate store") } + // If the field has a foreign index, make sure the index + // exists. + if f.options.ForeignIndex != "" { + if err := f.holder.checkForeignIndex(f); err != nil { + return errors.Wrap(err, "checking foreign index") + } + } + return nil }(); err != nil { f.Close() @@ -548,6 +556,14 @@ func (f *Field) applyTranslateStore() error { return nil } +// applyForeignIndex used to set the field's translateStore to +// that of the foreign index, but since moving to partitioned +// translate stores on indexes, that doesn't happen anymore. +// So now all this method does is check that the foreign index +// actually exists. If we decided this was unnecessary (which +// it kind of is), we could remove the field.holder and all +// the logic which does this check on holder open after all +// indexes have opened. func (f *Field) applyForeignIndex() error { foreignIndex := f.holder.Index(f.options.ForeignIndex) if foreignIndex == nil { From f001ad199f60fff349b80be2f5da2b472f763729 Mon Sep 17 00:00:00 2001 From: Travis Date: Fri, 17 Jan 2020 11:58:15 -0600 Subject: [PATCH 4/6] use translateIndexKeys instead of translateIndexKeySet in ImportValue() --- api.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/api.go b/api.go index 666bb4692..2049d2abe 100644 --- a/api.go +++ b/api.go @@ -1100,22 +1100,17 @@ func (api *API) ImportValue(ctx context.Context, req *ImportValueRequest, opts . // Translate values when the field uses keys (for example, when // the field has a ForeignIndex with keys). if useKeys { - keySet := make(map[string]struct{}) - for i := range req.StringValues { - keySet[req.StringValues[i]] = struct{}{} - } - - // Perform a separate batch translation for each separate index used. - keyMap := make(map[string]uint64) - if keyMap, err = api.cluster.translateIndexKeySet(ctx, foreignIndexName, keySet); err != nil { + // Perform translation. + uints, err := api.cluster.translateIndexKeys(ctx, foreignIndexName, req.StringValues) + if err != nil { return err } // Because the BSI field supports negative values, we have to // convert the uint64 keys to a slice of int64. - ints := make([]int64, len(req.StringValues)) - for i := range req.StringValues { - ints[i] = int64(keyMap[req.StringValues[i]]) + ints := make([]int64, len(uints)) + for i := range uints { + ints[i] = int64(uints[i]) } req.Values = ints } From b620e37e51820c2345b0796dc92fd9368dc795bd Mon Sep 17 00:00:00 2001 From: Travis Date: Fri, 17 Jan 2020 14:41:41 -0600 Subject: [PATCH 5/6] move the foreign index key check into applyTranslateStore() --- api.go | 15 ++------------- executor.go | 29 ++++------------------------- field.go | 11 +++++++++-- 3 files changed, 15 insertions(+), 40 deletions(-) diff --git a/api.go b/api.go index 2049d2abe..ee24823a1 100644 --- a/api.go +++ b/api.go @@ -1086,22 +1086,11 @@ func (api *API) ImportValue(ctx context.Context, req *ImportValueRequest, opts . req.Shard = math.MaxUint64 } - // Determine if foreign index is being used for translation. - useKeys := field.Keys() - foreignIndexName := field.ForeignIndex() - if foreignIndexName != "" { - foreignIndex := api.holder.indexes[foreignIndexName] - if foreignIndex == nil { - return errors.Errorf("foreign index not found: %q", foreignIndexName) - } - useKeys = foreignIndex.Keys() - } - // Translate values when the field uses keys (for example, when // the field has a ForeignIndex with keys). - if useKeys { + if field.Keys() { // Perform translation. - uints, err := api.cluster.translateIndexKeys(ctx, foreignIndexName, req.StringValues) + uints, err := api.cluster.translateIndexKeys(ctx, field.ForeignIndex(), req.StringValues) if err != nil { return err } diff --git a/executor.go b/executor.go index e3a064754..d2c23da46 100644 --- a/executor.go +++ b/executor.go @@ -3656,17 +3656,6 @@ func (e *executor) translateCall(indexName string, c *pql.Call, keyMaps map[stri return nil } - // Determine if foreign index is being used for translation. - useKeys := field.Keys() - foreignIndexName := field.ForeignIndex() - if foreignIndexName != "" { - foreignIndex := e.Holder.indexes[foreignIndexName] - if foreignIndex == nil { - return errors.Errorf("foreign index not found: %q", foreignIndexName) - } - useKeys = foreignIndex.Keys() - } - // Bool field keys do not use the translator because there // are only two possible values. Instead, they are handled // directly. @@ -3685,7 +3674,8 @@ func (e *executor) translateCall(indexName string, c *pql.Call, keyMaps map[stri } c.Args[rowKey] = rowID } - } else if useKeys { + } else if field.Keys() { + foreignIndexName := field.ForeignIndex() if c.Args[rowKey] != nil && isCondition(c.Args[rowKey]) { // In the case where a field has a foreign index with keys, // allow `== "key"` or `!= "key"` to be used against the BSI @@ -3868,22 +3858,11 @@ func (e *executor) translateResult(index string, idx *Index, call *pql.Call, res return nil, nil } - // Determine if foreign index is being used for translation. - useKeys := field.Keys() - foreignIndexName := field.ForeignIndex() - if foreignIndexName != "" { - foreignIndex := e.Holder.indexes[foreignIndexName] - if foreignIndex == nil { - return nil, errors.Errorf("foreign index not found: %q", foreignIndexName) - } - useKeys = foreignIndex.Keys() - } - - if useKeys { + if field.Keys() { rslt := result.Pos other := &Row{Attrs: rslt.Attrs} for _, segment := range rslt.Segments() { - keys, err := e.Cluster.translateIndexIDs(context.Background(), foreignIndexName, segment.Columns()) + keys, err := e.Cluster.translateIndexIDs(context.Background(), field.ForeignIndex(), segment.Columns()) if err != nil { return nil, errors.Wrap(err, "translating index ids") } diff --git a/field.go b/field.go index 86cf50b64..32dc18c22 100644 --- a/field.go +++ b/field.go @@ -520,8 +520,7 @@ func (f *Field) Open() error { return errors.Wrap(err, "opening attrstore") } - // If the field has a foreign index, and that index uses keys, - // then use that index's translateStore instead. + // Apply the field-specific translateStore. if err := f.applyTranslateStore(); err != nil { return errors.Wrap(err, "applying translate store") } @@ -553,6 +552,14 @@ func (f *Field) applyTranslateStore() error { return errors.Wrap(err, "opening field translate store") } f.usesKeys = f.options.Keys + + // In the case where the field has a foreign index, set + // the usesKeys value accordingly. + if foreignIndexName := f.ForeignIndex(); foreignIndexName != "" { + if foreignIndex := f.holder.indexes[foreignIndexName]; foreignIndex != nil { + f.usesKeys = foreignIndex.Keys() + } + } return nil } From d41ee990520f6b108aeb828a3ae51a68764947a1 Mon Sep 17 00:00:00 2001 From: Travis Date: Fri, 17 Jan 2020 14:46:43 -0600 Subject: [PATCH 6/6] linter fix --- executor_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/executor_test.go b/executor_test.go index 0ec94efe4..7586da42f 100644 --- a/executor_test.go +++ b/executor_test.go @@ -3940,10 +3940,7 @@ func sameStringSlice(x, y []string) bool { delete(diff, _y) } } - if len(diff) == 0 { - return true - } - return false + return len(diff) == 0 } func TestExecutor_Execute_GroupBy(t *testing.T) {