go-cloud icon indicating copy to clipboard operation
go-cloud copied to clipboard

Docstore/memdocstore: nested query

Open eqinox76 opened this issue 1 year ago • 6 comments

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.

eqinox76 avatar Nov 19 '24 13:11 eqinox76

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.

google-cla[bot] avatar Nov 19 '24 13:11 google-cla[bot]

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

vangent avatar Dec 07 '24 04:12 vangent

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.

eqinox76 avatar Dec 12 '24 14:12 eqinox76

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

vangent avatar Dec 13 '24 20:12 vangent

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 :warning:
docstore/memdocstore/query.go 92.30% 1 Missing and 1 partial :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Dec 13 '24 20:12 codecov[bot]

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

jba avatar Dec 13 '24 21:12 jba

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

vangent avatar Jun 28 '25 19:06 vangent

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!

eqinox76 avatar Jul 01 '25 10:07 eqinox76

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

vangent avatar Jul 03 '25 17:07 vangent

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.

eqinox76 avatar Jul 04 '25 06:07 eqinox76

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

eqinox76 avatar Jul 09 '25 12:07 eqinox76