diff --git a/api.go b/api.go index 2a74f3b11..ee24823a1 100644 --- a/api.go +++ b/api.go @@ -1089,12 +1089,14 @@ 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 field.Keys() { - uints, err := field.translateStore.TranslateKeys(req.StringValues) + // Perform translation. + uints, err := api.cluster.translateIndexKeys(ctx, field.ForeignIndex(), req.StringValues) if err != nil { - return errors.Wrap(err, "translating string values") + return err } - // Because the BSI field supports negative value, we have to - // convert the slice of uint64 keys to a slice of int64. + + // Because the BSI field supports negative values, we have to + // convert the uint64 keys to a slice of int64. ints := make([]int64, len(uints)) for i := range uints { ints[i] = int64(uints[i]) diff --git a/executor.go b/executor.go index 7b79fd11c..d2c23da46 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 { @@ -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 @@ -3857,34 +3847,36 @@ 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 field.Keys() { + rslt := result.Pos + other := &Row{Attrs: rslt.Attrs} + for _, segment := range rslt.Segments() { + keys, err := e.Cluster.translateIndexIDs(context.Background(), field.ForeignIndex(), segment.Columns()) if err != nil { - return nil, err + return nil, errors.Wrap(err, "translating index ids") } - other.Keys = append(other.Keys, key) + other.Keys = append(other.Keys, keys...) } + return &SignedRow{Pos: other}, nil } - 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..7586da42f 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,31 @@ 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) + } + } + return len(diff) == 0 +} + func TestExecutor_Execute_GroupBy(t *testing.T) { groupByTest := func(t *testing.T, clusterSize int) { c := test.MustRunCluster(t, 1) diff --git a/field.go b/field.go index 149ae65c3..32dc18c22 100644 --- a/field.go +++ b/field.go @@ -520,12 +520,19 @@ 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") } + // 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() @@ -545,9 +552,25 @@ 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 } +// 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 {