feat: Add support for branchable collection time-traveling #3260
Conversation
AndrewSisley
added
bug
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3260 +/- ##
===========================================
+ Coverage 77.47% 77.62% +0.14%
===========================================
Files 382 382
Lines 35294 35290 -4
===========================================
+ Hits 27344 27391 +47
+ Misses 6317 6278 -39
+ Partials 1633 1621 -12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
AndrewSisley
force-pushed
the
3257-col-level-tt
branch
from
d8d6839 to
142b2a4
Compare
internal/planner/select.go
Outdated
| if len(n.selectReq.DocIDs.Value()) > 0 { | ||
| docID = n.selectReq.DocIDs.Value()[0] |
There was a problem hiding this comment.
thought: Passing the docID for a timetravel query is unnecessary as it will never be used. Even worst, it's confusing the reader by pretending to be useful for the query. One can pass an invalid docID and still get a valid response. We should probably remove references to docID in this block.
tought: It might be more appropriate to allow setting CID or docID but not both in the query params.
There was a problem hiding this comment.
Oh I forgot to test that, sorry.
There is a small chance it is still used within the planner nodes, and I actually want it to be so, so that a docID can be provided alongside a collection cid - otherwise there is a gap in the query syntax like you said. Also, IIRC John really likes being able to set the docID and cid in the same query (for doc-cids), even though the docID has always been somewhat pointless from an external perspective.
That said, even then the docID in Spans is almost certainly irrelevant (I didn't notice I'd removed it completely from the versionedFetcher) - and so this block deserves a revisit for being highly misleading like you said.
I'll have a play, if it looks like it'll take a bit more effort to get collection-cids with docIDs working I might add a temp error and we can add support whenever it becomes high enough priority.
- col-cid/docID interplay stuff
There was a problem hiding this comment.
It worked with the prod code unchanged, test is TestQuerySimpleWithCidOfBranchableCollectionAndDocID.
I think I remember actually thinking this through, and saw that the span is passed on to the backing document fetcher. But looking through the code I don't see that atm. It might be that the split logic refuses to split the filter as the first fetcher is not a doc-fetcher, leaving the span/select node to handle the docID filter.
Where this happens is not really important now though, I'll see if I can remove the block here without stuff breaking :)
- Remove docID from prefix (it isn't actually a prefix either)
There was a problem hiding this comment.
Removing it was problem free. I've documented the oddness in the code, and will merge this PR once the CI is done.
| if err != nil { | ||
| return err | ||
| } | ||
| var mcrdt merklecrdt.MerkleCRDT |
There was a problem hiding this comment.
praise: I really like the changes in this method 🙂
This is a bug I accidentally fixed (originally in another commit), we should probably have more tests that cover this, but I'm not interested in adding them in this PR.
The contents are very cheap, possibly more so than the cache.
AndrewSisley
force-pushed
the
3257-col-level-tt
branch
from
142b2a4 to
9bd23f2
Compare
…work#3260) ## Relevant issue(s) Resolves sourcenetwork#3257 ## Description Add support for branchable collection time-traveling. Also fixes the docID param which was misbehaving (see commit `Create document with actual docID not user provided value`).
Relevant issue(s)
Resolves #3257
Description
Add support for branchable collection time-traveling.
Also fixes the docID param which was misbehaving (see commit
Create document with actual docID not user provided value).Suggest reviewing commit by commit, it should be easier to follow.
Do not review the first commit here - this branch is based of off #3256 - please review that PR first.