Docstore/memdocstore: nested query #3508
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
eqinox76
force-pushed
the
docstore/memdocstore-nested-query
branch
from
dd80187 to
29a9b7b
Compare
|
can you update your PR to remove the drivertest changes and add that option? You should be able to add it to the existing Options struct |
eqinox76
force-pushed
the
docstore/memdocstore-nested-query
branch
from
29a9b7b to
02ce845
Compare
renamed option to AllowNestedSliceQueries
|
@jba I'd like to wait for your review/LGTM. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3508 /- ##
==========================================
Coverage 73.24% 73.29% 0.04%
==========================================
Files 113 113
Lines 15052 15081 29
==========================================
Hits 11025 11053 28
Misses 3262 3261 -1
- Partials 765 767 2 ☔ View full report in Codecov by Sentry. |
|
I'll try to get to it this weekend/early next week. |
| // 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. |
There was a problem hiding this comment.
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.
| 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) { |
| t.Fatal(err) | ||
| } | ||
|
|
||
| got := count(coll.Query().Where("list.a", "=", "A").Get(ctx)) |
There was a problem hiding this comment.
I think the tests would be improved by actually comparing the results. Also, use table-driven style. See testGetQuery for an example.
| if v1.Kind() == reflect.Slice { | ||
| for i := 0; i < v1.Len(); i { | ||
| if c, ok := compare(x2, v1.Index(i).Interface()); ok { | ||
| if !ok { |
There was a problem hiding this comment.
We already know ok is true from the previous line.
Fixes #3506 docstore/memdocstore query for nested documents with slices does not work
Added a test to the drivertest. It is working fine with the fixed memdocstore implementation but i can't test it against the other cloud providers.