Skip to content

Docstore/memdocstore: nested query#3508

Merged
jba merged 7 commits intogoogle:masterfrom
eqinox76:docstore/memdocstore-nested-query
Jul 9, 2025
Merged

Docstore/memdocstore: nested query#3508
jba merged 7 commits intogoogle:masterfrom
eqinox76:docstore/memdocstore-nested-query

Conversation

@eqinox76
Copy link
Contributor

@eqinox76 eqinox76 commented Nov 19, 2024

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.

@google-cla
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
docstore/memdocstore: google#3508 nested query
69656fd
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Nov 19, 2024
@eqinox76
docstore: google#3508 added test for nested query
29a9b7b
@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
@eqinox76 eqinox76 mentioned this pull request Nov 19, 2024
Closed
@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 added a commit to eqinox76/go-cloud that referenced this pull request Dec 10, 2024
@eqinox76
docstore/memdocstore: google#3508 allow nested slices query
02ce845
@eqinox76 eqinox76 force-pushed the docstore/memdocstore-nested-query branch from 29a9b7b to 02ce845 Compare December 10, 2024 08:14
vangent
vangent requested changes Dec 11, 2024
View reviewed changes
jba
jba requested changes Dec 11, 2024
View reviewed changes
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Dec 12, 2024
@eqinox76
docstore/memdocstore: google#3508 first review
e16b6c9 
renamed option to AllowNestedSliceQueries
@eqinox76
Copy link
Contributor Author

eqinox76 commented Dec 12, 2024

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.

@codecov
Copy link

codecov bot commented Dec 13, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 70.63%. Comparing base (01e0447) to head (cd9090a).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
docstore/memdocstore/mem.go 88.46% 2 Missing and 1 partial ⚠️
docstore/memdocstore/query.go 92.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3508      +/-   ##
==========================================
+ Coverage   70.60%   70.63%   +0.02%     
==========================================
  Files         113      115       +2     
  Lines       14384    14521     +137     
==========================================
+ Hits        10156    10257     +101     
- Misses       3501     3536      +35     
- Partials      727      728       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jba
Copy link
Contributor

jba commented Dec 13, 2024

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

jba
jba requested changes Dec 14, 2024
View reviewed changes
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!

docstore/memdocstore/mem_test.go Outdated
t.Fatal(err)
}

got := count(coll.Query().Where("list.a", "=", "A").Get(ctx))
Copy link
Contributor

@jba jba Dec 14, 2024

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.

Copy link
Contributor Author

@eqinox76 eqinox76 Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, i rewrote it to a table-driven style. Please check if its more readable

jba added a commit that referenced this pull request Dec 14, 2024
@jba
all: rename interface{} to any
5cd47b9 
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).
@jba jba mentioned this pull request Dec 14, 2024
Merged
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Jun 6, 2025
@eqinox76
docstore/memdocstore: google#3508 fixed >= and <= operators not worki…
952abc3 
…ng when comparing in nested structures
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Jun 6, 2025
@eqinox76
docstore/memdocstore: google#3508 allow to enable AllowNestedSliceQue…
4f4f9e1 
…ries per url parameter
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Jun 6, 2025
@eqinox76
docstore/memdocstore: google#3508 fixed >= and <= operators not worki…
e3436b8 
…ng when comparing in nested structures
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Jun 6, 2025
@eqinox76
docstore/memdocstore: google#3508 allow to enable AllowNestedSliceQue…
464a9d1 
…ries per url parameter
@eqinox76 eqinox76 force-pushed the docstore/memdocstore-nested-query branch from 4f4f9e1 to 464a9d1 Compare June 6, 2025 08:24
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Jun 6, 2025
@eqinox76
docstore/memdocstore: google#3508 fixed slices in slices are now foun…
3691c80 
…d with operators
@vangent
Copy link
Contributor

vangent commented Jun 28, 2025

@eqinox76 can you address the open comments and sync to head?

@eqinox76 eqinox76 force-pushed the docstore/memdocstore-nested-query branch from 3691c80 to be1bdae Compare July 1, 2025 10:09
@eqinox76
Copy link
Contributor Author

eqinox76 commented Jul 1, 2025

Hi @vangent , @jba ,
sorry for the long delay. I missed the notifications. On the plus side we found a few omissions in the feature. Especially when the nesting is deeper than one layer of slices or for more complex operations like '>='.
I updated to head, fixed the bugs and amended unit tests. Sadly the change got a bit bigger and especially in getAtFieldPath (mem.go:410) a bit unwiedly for the nested branch.

Please let me know if something else is missing. And many thanks for your time and work!

@vangent
Copy link
Contributor

vangent commented Jul 3, 2025

@jba when you have a chance can you re-review, sounds like there have been non-trivial changes since your approval.

jba
jba requested changes Jul 3, 2025
View reviewed changes
eqinox76 added a commit to eqinox76/go-cloud that referenced this pull request Jul 4, 2025
@eqinox76
docstore/memdocstore: google#3508 third review
8f0ede7 
fixed comment style
@eqinox76
Copy link
Contributor Author

eqinox76 commented Jul 4, 2025

Thanks for the review. Shadowing the variable name makes it actually more readable. I commit the changes and fixed the comment style. Please let me know when there is something else that i missed.

jba
jba approved these changes Jul 9, 2025
View reviewed changes
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.

Thank you! Sorry my reviewing has taken so long.

@eqinox76 eqinox76 force-pushed the docstore/memdocstore-nested-query branch from 8f0ede7 to cd9090a Compare July 9, 2025 12:35
@eqinox76
Copy link
Contributor Author

eqinox76 commented Jul 9, 2025

Hi, I fixed a flaky test that your test pipeline correctly identified and pushed the changes.

@jba jba merged commit 85b41d1 into google:master Jul 9, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vangent vangent vangent requested changes

@jba jba jba approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@eqinox76 @vangent @jba