Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docstore/memdocstore: nested query #3508

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docstore/memdocstore/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func decodeDoc(m storedDoc, ddoc driver.Document, fps [][]string) error {
// (We don't need the key field because ddoc must already have it.)
m2 = map[string]interface{}{}
for _, fp := range fps {
val, err := getAtFieldPath(m, fp)
val, err := getAtFieldPath(m, fp, false)
if err != nil {
if gcerrors.Code(err) == gcerrors.NotFound {
continue
Expand Down
47 changes: 31 additions & 16 deletions docstore/memdocstore/mem.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@
// When the collection is closed, its contents are saved to the file.
Filename string

// AllowNestedSliceQueries allows querying with nested slices.
// This makes the memdocstore more compatible with MongoDB,
// but other providers may not support this feature.
// See https://github.com/google/go-cloud/pull/3511 for more details.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't refer to a PR for documentation. Make the doc self-contained by explaining how this changes the behavior, that is, precisely what "querying with nested slices" means.

You can refer to the PR in an implementation comment so developers can understand the context.

AllowNestedSliceQueries bool

// Call this function when the collection is closed.
// For internal use only.
onClose func()
Expand Down Expand Up @@ -399,16 +405,33 @@

// getAtFieldPath gets the value of m at fp. It returns an error if fp is invalid
// (see getParentMap).
func getAtFieldPath(m map[string]interface{}, fp []string) (interface{}, error) {
m2, err := getParentMap(m, fp, false)
if err != nil {
return nil, err
func getAtFieldPath(m map[string]any, fp []string, nested bool) (result any, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add doc explaining nested.

var get func(m any, name string) any
get = func(m any, name string) any {
switch concrete := m.(type) {
case map[string]any:
return concrete[name]
case []any:
if !nested {
return nil

Check warning on line 416 in docstore/memdocstore/mem.go

View check run for this annotation

Codecov / codecov/patch

docstore/memdocstore/mem.go#L416

Added line #L416 was not covered by tests
}
var result []any
for _, e := range concrete {
result = append(result, get(e, name))
}
return result
eqinox76 marked this conversation as resolved.
Show resolved Hide resolved
}
return nil

Check warning on line 424 in docstore/memdocstore/mem.go

View check run for this annotation

Codecov / codecov/patch

docstore/memdocstore/mem.go#L424

Added line #L424 was not covered by tests
}
v, ok := m2[fp[len(fp)-1]]
if ok {
return v, nil
result = m
for _, k := range fp {
next := get(result, k)
if next == nil {
return nil, gcerr.Newf(gcerr.NotFound, nil, "field %s not found", strings.Join(fp, "."))
}
result = next
}
return nil, gcerr.Newf(gcerr.NotFound, nil, "field %s not found", fp)
return result, nil
}

// setAtFieldPath sets m's value at fp to val. It creates intermediate maps as
Expand All @@ -422,14 +445,6 @@
return nil
}

// Delete the value from m at the given field path, if it exists.
jba marked this conversation as resolved.
Show resolved Hide resolved
func deleteAtFieldPath(m map[string]interface{}, fp []string) {
m2, _ := getParentMap(m, fp, false) // ignore error
if m2 != nil {
delete(m2, fp[len(fp)-1])
}
}

// getParentMap returns the map that directly contains the given field path;
// that is, the value of m at the field path that excludes the last component
// of fp. If a non-map is encountered along the way, an InvalidArgument error is
Expand Down
67 changes: 67 additions & 0 deletions docstore/memdocstore/mem_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package memdocstore

import (
"context"
"io"
"os"
"path/filepath"
"testing"
Expand Down Expand Up @@ -129,6 +130,72 @@ func TestUpdateAtomic(t *testing.T) {
}
}

func TestQueryNested(t *testing.T) {
ctx := context.Background()

count := func(iter *docstore.DocumentIterator) (c int) {
doc := docmap{}
for {
if err := iter.Next(ctx, doc); err != nil {
if err == io.EOF {
break
}
t.Fatal(err)
}
c++
}
return c
}

dc, err := newCollection(drivertest.KeyField, nil, &Options{AllowNestedSliceQueries: true})
if err != nil {
t.Fatal(err)
}
coll := docstore.NewCollection(dc)
defer coll.Close()

doc := docmap{drivertest.KeyField: "TestQueryNested",
"list": []any{docmap{"a": "A"}},
"map": docmap{"b": "B"},
"listOfMaps": []any{docmap{"id": "1"}, docmap{"id": "2"}, docmap{"id": "3"}},
"mapOfLists": docmap{"ids": []any{"1", "2", "3"}},
"deep": []any{docmap{"nesting": []any{docmap{"of": docmap{"elements": "yes"}}}}},
dc.RevisionField(): nil,
}
if err := coll.Put(ctx, doc); err != nil {
t.Fatal(err)
}

got := count(coll.Query().Where("list.a", "=", "A").Get(ctx))
vangent marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tests would be improved by actually comparing the results. Also, use table-driven style. See testGetQuery for an example.

if got != 1 {
t.Errorf("got %v docs when filtering by list.a, want 1", got)
}
got = count(coll.Query().Where("list.a", "=", "missing").Get(ctx))
if got != 0 {
t.Errorf("got %v docs when filtering by list.a, want 0", got)
}
got = count(coll.Query().Where("map.b", "=", "B").Get(ctx))
if got != 1 {
t.Errorf("got %v docs when filtering by map.b, want 1", got)
}
got = count(coll.Query().Where("listOfMaps.id", "=", "1").Get(ctx))
if got != 1 {
t.Errorf("got %v docs when filtering by listOfMaps.id, want 1", got)
}
got = count(coll.Query().Where("listOfMaps.id", "=", "123").Get(ctx))
if got != 0 {
t.Errorf("got %v docs when filtering by listOfMaps.id, want 0", got)
}
got = count(coll.Query().Where("mapOfLists.ids", "=", "1").Get(ctx))
if got != 1 {
t.Errorf("got %v docs when filtering by listOfMaps.1, want 1", got)
}
got = count(coll.Query().Where("deep.nesting.of.elements", "=", "yes").Get(ctx))
if got != 1 {
t.Errorf("got %v docs when filtering by deep.nesting.of.elements, want 1", got)
}
}

func TestSortDocs(t *testing.T) {
newDocs := func() []storedDoc {
return []storedDoc{
Expand Down
24 changes: 19 additions & 5 deletions docstore/memdocstore/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

var resultDocs []storedDoc
for _, doc := range c.docs {
if filtersMatch(q.Filters, doc) {
if filtersMatch(q.Filters, doc, c.opts.AllowNestedSliceQueries) {
resultDocs = append(resultDocs, doc)
}
}
Expand Down Expand Up @@ -74,17 +74,17 @@
}, nil
}

func filtersMatch(fs []driver.Filter, doc storedDoc) bool {
func filtersMatch(fs []driver.Filter, doc storedDoc, nested bool) bool {
for _, f := range fs {
if !filterMatches(f, doc) {
if !filterMatches(f, doc, nested) {
return false
}
}
return true
}

func filterMatches(f driver.Filter, doc storedDoc) bool {
docval, err := getAtFieldPath(doc, f.FieldPath)
func filterMatches(f driver.Filter, doc storedDoc, nested bool) bool {
docval, err := getAtFieldPath(doc, f.FieldPath, nested)
// missing or bad field path => no match
if err != nil {
return false
Expand Down Expand Up @@ -138,6 +138,20 @@
}
return -1, true
}
eqinox76 marked this conversation as resolved.
Show resolved Hide resolved
// for AllowNestedSliceQueries
// when querying for x2 in the document and x1 is a list of values we only need one value to match
if v1.Kind() == reflect.Slice {
for i := 0; i < v1.Len(); i++ {
if c, ok := compare(x2, v1.Index(i).Interface()); ok {
if !ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already know ok is true from the previous line.

return 0, false

Check warning on line 147 in docstore/memdocstore/query.go

View check run for this annotation

Codecov / codecov/patch

docstore/memdocstore/query.go#L147

Added line #L147 was not covered by tests
}
if c == 0 {
return 0, true
}
}
}
}
if v1.Kind() == reflect.String && v2.Kind() == reflect.String {
return strings.Compare(v1.String(), v2.String()), true
}
Expand Down
Loading