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

Conversation

eqinox76
Copy link

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.

Copy link

google-cla bot commented Nov 19, 2024

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 added a commit to eqinox76/go-cloud that referenced this pull request Nov 19, 2024
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Nov 19, 2024
@eqinox76 eqinox76 force-pushed the docstore/memdocstore-nested-query branch from dd80187 to 29a9b7b Compare November 19, 2024 13:55
@eqinox76 eqinox76 marked this pull request as ready for review November 19, 2024 13:56
@vangent
Copy link
Contributor

vangent commented Dec 7, 2024

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 eqinox76 force-pushed the docstore/memdocstore-nested-query branch from 29a9b7b to 02ce845 Compare December 10, 2024 08:14
docstore/memdocstore/mem.go Outdated Show resolved Hide resolved
docstore/memdocstore/mem.go Outdated Show resolved Hide resolved
docstore/memdocstore/mem.go Show resolved Hide resolved
docstore/memdocstore/mem_test.go Show resolved Hide resolved
docstore/memdocstore/query.go Show resolved Hide resolved
docstore/memdocstore/mem.go Outdated Show resolved Hide resolved
docstore/memdocstore/mem.go Outdated Show resolved Hide resolved
docstore/memdocstore/mem.go Show resolved Hide resolved
docstore/memdocstore/mem.go Show resolved Hide resolved
renamed option to AllowNestedSliceQueries
@eqinox76
Copy link
Author

Thank you @vangent and @jba for your reviews! I amended the pr and i think only the question about the removed function is open. Please let me know if i missed anything.

@vangent
Copy link
Contributor

vangent commented Dec 13, 2024

@jba I'd like to wait for your review/LGTM.

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.

Project coverage is 73.29%. Comparing base (5695484) to head (e16b6c9).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
docstore/memdocstore/mem.go 85.71% 2 Missing and 1 partial ⚠️
docstore/memdocstore/query.go 83.33% 1 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@jba
Copy link
Contributor

jba commented Dec 13, 2024

I'll try to get to it this weekend/early next week.

Copy link
Contributor

@jba jba left a comment

Choose a reason for hiding this comment

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

Thanks for doing this work. Sorry it took me a while to give it careful look. The only major change is improving the test, otherwise looks good!

// 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.

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.

t.Fatal(err)
}

got := count(coll.Query().Where("list.a", "=", "A").Get(ctx))
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 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.

jba added a commit that referenced this pull request Dec 14, 2024
All modules are on a Go version after 1.18, so we can rename
interface{} to any to modernize the code.

docstore is omitted, because there are PRs in flight that this might
conflict with (#3500, #3508).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docstore/memdocstore query for nested documents with slices does not work
3 participants