-
Notifications
You must be signed in to change notification settings - Fork 885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RemoteDocumentCache APIs for Indexing #5988
Conversation
🦋 Changeset detectedLatest commit: 8ce57c0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Changeset File Check ✅
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (782,147 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
80573ca
to
5e1a25b
Compare
5e1a25b
to
a74af48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not reviewed tests yet, but want to provide some feedback first.
Main concern is some places are not very readable.
Also, it looks like we might need to do this for LevelDb as well?
return store.delete(key); | ||
const path = documentKey.path.toArray(); | ||
return store.delete([ | ||
path.slice(0, path.length - 2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are quite some places using length - n
to get what we want, maybe we should put all these in DocumentKey
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it will fit well in DocumentKey, as we don't really have good terminology for "path that leads up to the collection". I will figure out how to make this cleaner in place.
packages/firestore/src/local/indexeddb_remote_document_cache.ts
Outdated
Show resolved
Hide resolved
.changeset/seven-dolphins-breathe.md
Outdated
"@firebase/firestore": patch | ||
--- | ||
|
||
The format of some of the IndexedDB data changed. This increases the performance of document lookups after an initial migration. If you do not want to migrate data, you can call `clearIndexedDbPersistence()` before invoking `enablIndexedDbPersistence()`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in line 5, needs another 'e', should be enableIndexedDbPersistence().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@egilmorez ... Talked to sebastian, and these changes are internal-only, no user-breaking effects. Thanks for adding me to the review.
@schmidt-sebastian ... Please feel free to include me in Firestore PRs, all platforms, especially the PRs that touch public-facing reference docs. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This changes the schema of the Remote Document Cache to rewrite the primary keys. The keys are now:
[ [prefix path ], collection group, read time, document ]
This allows for efficient scanning of indexed and non-indexed queries using
getAll()
. The PR also addsgetDocumentsFromCollectionGroup
, which means that all functionality required by Indexing is now available.TODO: