Docstore/memdocstore: nested query
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.
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.
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
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.
@jba I'd like to wait for your review/LGTM.
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.
I'll try to get to it this weekend/early next week.
@eqinox76 can you address the open comments and sync to head?
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!
@jba when you have a chance can you re-review, sounds like there have been non-trivial changes since your approval.
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.
Hi, I fixed a flaky test that your test pipeline correctly identified and pushed the changes.