Index-Free: Using readTime instead of updateTime #686
Conversation
| /** Underlying cache of documents. */ | ||
| private ImmutableSortedMap<DocumentKey, MaybeDocument> docs; | ||
| /** Underlying cache of documents and their read times. */ | ||
| private ImmutableSortedMap<DocumentKey, Pair<MaybeDocument, SnapshotVersion>> docs; |
There was a problem hiding this comment.
Note that the real only benefit of the changes in this file (and the additional memory-overhead) is that we can run the LocalStore tests with IndexFree queries both against the MemoryRemoteDocumentCache and the SQLiteRemoteDocumentCache. We need to match the filter logic to be able to compare row read counts.
...e-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java
Show resolved
Hide resolved
| } | ||
| return count; | ||
| } | ||
|
|
||
| /** An Iterator that iterates the current set of documents in the RemoteDocumentCache. */ | ||
| private class DocumentIterator implements Iterable<MaybeDocument> { |
There was a problem hiding this comment.
This isn't quite a DocumentIterator--the actual iterator is the anonymous class below. DocumentIterable?
Also the comment would need tweaking. Maybe: A proxy that creates an iterator over the current ...
However, before addressing this consider the higher-level question elsewhere.
There was a problem hiding this comment.
Changed to:
/** A proxy that exposes an iterator over the current set of documents in the RemoteDocumentCache. */
| */ | ||
| void add(MaybeDocument maybeDocument); | ||
| void add(MaybeDocument maybeDocument, SnapshotVersion readTime); |
There was a problem hiding this comment.
Dumb question: why not make readTime a part of the MaybeDocument?
There was a problem hiding this comment.
That was the road I went down on at first - it generates a significant amount of changes to plumb through a value that is only ever read directly at source.
There was a problem hiding this comment.
I'm sort of surprised there are that many changes. Did you happen to commit it so I can see?
For example: all our tests could use the same snapshot for both versions by default and only specify a second timestamp when the difference really matters. That means nearly all tests would be unaffected.
Oh, maybe the issue is that the read_time comes from the TargetChange and is not intrinsically a part of the document?
There was a problem hiding this comment.
There are at least two additional challenges:
- If I add it to MaybeDocument, I have to change it for Document, UnknownDocument and NoDocument. At least for NoDocuments it is throwaway work as these are never written to the cache.
- MaybeDoocument currently uses the generic term
version. This works well with all subtypes, but it doesn't quite work if introduce another kind of version.
I never actually got as far as to see how we would construct the individual documents. By exposing the read time only as an input to the RemoteDocumentCache there are only two callsites that need changing, which won me over.
| @@ -134,7 +136,7 @@ public void testSetAndReadLotsOfDocuments() { | |||
| public void testSetAndReadDeletedDocument() { | |||
| String path = "a/b"; | |||
| NoDocument deletedDoc = deletedDoc(path, 42); | |||
| add(deletedDoc); | |||
| add(deletedDoc, version(42)); | |||
There was a problem hiding this comment.
All of these tests use identical versions for the update_time and read_time which means we could have gotten these reversed and won't see it. Could you add a test that verifies read time doesn't get crossed with update time?
There was a problem hiding this comment.
I think it will become easier to add this test when #617 lands (which should be the next PR). I added a TODO for now and will resolve it when we gain the ability to exclude documents by read time.
This PR updates index-free queries to use readTime instead of updateTime, which allows us to pick up documents that matched a given query at the Query's snapshot version but were not part of the query's limit (see spec test: "Resumed limit queries use updated documents")