Add option to allow SDK create cache indexes automatically to improve query execution locally #4987
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Contributor
Javadoc Changes:--- /Users/runner/diff/original/firebase-kotlindoc/android/com/google/firebase/firestore/FirebaseFirestore.html 2023-07-26 17:30:50.000000000 +0000 +++ /Users/runner/diff/modified/firebase-kotlindoc/android/com/google/firebase/firestore/FirebaseFirestore.html 2023-07-26 17:24:33.000000000 +0000 @@ -22,6 +22,27 @@ </colgroup> <thead> <tr> + <th colspan="100%"><h3>Public fields</h3></th> + </tr> + </thead> + <tbody class="list"> + <tr> + <td><code>@<a href="https://developer.android.com/reference/kotlin/androidx/annotation/Nullable.html">Nullable</a> <a href="/docs/reference/android/com/google/firebase/firestore/PersistentCacheIndexManager.html">PersistentCacheIndexManager</a></code></td> + <td> + <div><code><a href="/docs/reference/android/com/google/firebase/firestore/FirebaseFirestore.html#persistentCacheIndexManager()">persistentCacheIndexManager</a></code></div> + </td> + </tr> + </tbody> + </table> + </div> + <div class="devsite-table-wrapper"> + <table class="responsive"> + <colgroup> + <col width="40%"> + <col> + </colgroup> + <thead> + <tr> <th colspan="100%"><h3>Public methods</h3></th> </tr> </thead> @@ -233,6 +254,13 @@ </table> </div> <div class="list"> + <h2>Public fields</h2> + <div class="api-item"><a name="getPersistentCacheIndexManager()"></a><a name="setPersistentCacheIndexManager()"></a><a name="getPersistentCacheIndexManager--"></a><a name="setPersistentCacheIndexManager--"></a> + <h3 class="api-name" id="persistentCacheIndexManager()">persistentCacheIndexManager</h3> + <pre class="api-signature no-pretty-print">public @<a href="https://developer.android.com/reference/kotlin/androidx/annotation/Nullable.html">Nullable</a> <a href="/docs/reference/android/com/google/firebase/firestore/PersistentCacheIndexManager.html">PersistentCacheIndexManager</a> <a href="/docs/reference/android/com/google/firebase/firestore/FirebaseFirestore.html#persistentCacheIndexManager()">persistentCacheIndexManager</a></pre> + </div> + </div> + <div class="list"> <h2>Public methods</h2> <div class="api-item"><a name="addSnapshotsInSyncListener-java.lang.Runnable-"></a><a name="addsnapshotsinsynclistener"></a> <h3 class="api-name" id="addSnapshotsInSyncListener(java.lang.Runnable)">addSnapshotsInSyncListener</h3>--- /Users/runner/diff/original/firebase-kotlindoc/kotlin/com/google/firebase/firestore/FirebaseFirestore.html 2023-07-26 17:30:50.000000000 +0000 +++ /Users/runner/diff/modified/firebase-kotlindoc/kotlin/com/google/firebase/firestore/FirebaseFirestore.html 2023-07-26 17:24:32.000000000 +0000 @@ -232,6 +232,27 @@ </tbody> </table> </div> + <div class="devsite-table-wrapper"> + <table class="responsive"> + <colgroup> + <col width="40%"> + <col> + </colgroup> + <thead> + <tr> + <th colspan="100%"><h3>Public properties</h3></th> + </tr> + </thead> + <tbody class="list"> + <tr> + <td><code><a href="/docs/reference/kotlin/com/google/firebase/firestore/PersistentCacheIndexManager.html">PersistentCacheIndexManager</a>?</code></td> + <td> + <div><code><a href="/docs/reference/kotlin/com/google/firebase/firestore/FirebaseFirestore.html#persistentCacheIndexManager()">persistentCacheIndexManager</a></code></div> + </td> + </tr> + </tbody> + </table> + </div> <div class="list"> <h2>Public functions</h2> <div class="api-item"><a name="addSnapshotsInSyncListener-java.lang.Runnable-"></a><a name="addsnapshotsinsynclistener"></a> @@ -1211,6 +1232,13 @@ </div> </div> </div> + <div class="list"> + <h2>Public properties</h2> + <div class="api-item"><a name="getPersistentCacheIndexManager()"></a><a name="setPersistentCacheIndexManager()"></a><a name="getPersistentCacheIndexManager--"></a><a name="setPersistentCacheIndexManager--"></a> + <h3 class="api-name" id="persistentCacheIndexManager()">persistentCacheIndexManager</h3> + <pre class="api-signature no-pretty-print">val <a href="/docs/reference/kotlin/com/google/firebase/firestore/FirebaseFirestore.html#persistentCacheIndexManager()">persistentCacheIndexManager</a>: <a href="/docs/reference/kotlin/com/google/firebase/firestore/PersistentCacheIndexManager.html">PersistentCacheIndexManager</a>?</pre> + </div> + </div> </body> </html> |
Contributor
Coverage Report 1Affected Products
Test Logs |
Contributor
Unit Test Results 162 files + 108 162 suites +108 2m 44s ⏱️ - 4m 21s Results for commit fb12477. ± Comparison against base commit 4d1cc39. This pull request removes 470 and adds 1188 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Size Report 1Affected Products
Test Logs |
Contributor
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Notes
Startup Times
|
wu-hui reviewed May 9, 2023
Contributor
wu-hui left a comment
wu-hui left a comment There was a problem hiding this comment.
Some comments, I think you get the overall idea however.
While you are at it, maybe write a simple count-based heuristics: compare the doc reads from full collection scan, and indexed-doc reads from a imaginary perfect indexed query, and make a decision to turn on indexing?
firebase-firestore/src/main/java/com/google/firebase/firestore/local/AutoIndexing.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
...e-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java Outdated Show resolved Hide resolved
Contributor
| The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
cherylEnkidu force-pushed the cheryllin/autoindexing branch from 79b1fd2 to 8aed796 Compare wu-hui reviewed May 18, 2023
Contributor
wu-hui left a comment
wu-hui left a comment There was a problem hiding this comment.
Looks good for a prototype.
I want to see some tests for the change in TargetIndexMatcher, then we should be able to move forward.
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryContext.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/model/TargetIndexMatcher.java Outdated Show resolved Hide resolved
Contributor
| The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
1 similar comment
Contributor
| The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
wu-hui reviewed May 25, 2023
firebase-firestore/src/main/java/com/google/firebase/firestore/model/TargetIndexMatcher.java Show resolved Hide resolved
cherylEnkidu force-pushed the cheryllin/autoindexing branch 2 times, most recently from e5b8859 to 1e25c19 Compare 
Contributor
| The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
2 similar comments
Contributor
| The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Contributor
| The public api surface has changed for the subproject firebase-firestore: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
cherylEnkidu force-pushed the cheryllin/autoindexing branch 3 times, most recently from d48504b to ad7fc61 Compare wu-hui requested changes Jun 30, 2023
Contributor
wu-hui left a comment
wu-hui left a comment There was a problem hiding this comment.
Please add more tests to SQLiteLocalStoreTest.
This is a place where you can:
- setup a collection
- turn on auto-indexing
- run a query that should have added a field index
- verify field indexes are added
- call backfillIndexes()
- verify query result and document read count is as expected as an indexed query.
You should be able to find enough examples in that file.
firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/IndexingTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/androidTest/java/com/google/firebase/firestore/IndexingTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
...e-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Outdated Show resolved Hide resolved
dconeybe requested changes Jul 18, 2023
firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryContext.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryContext.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryContext.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/model/TargetIndexMatcher.java Outdated Show resolved Hide resolved
wu-hui reviewed Jul 20, 2023
Contributor
wu-hui left a comment
wu-hui left a comment There was a problem hiding this comment.
Some improvements are still needed, but I think we are very close now.
firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java Outdated Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java Show resolved Hide resolved
firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java Show resolved Hide resolved
wu-hui approved these changes Jul 24, 2023
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryContext.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
dconeybe requested changes Jul 24, 2023
Contributor
dconeybe left a comment
dconeybe left a comment There was a problem hiding this comment.
Thank you for addressing my previous comments. After re-reading, I have some more suggestions, mostly relating to readability rather than to functionality of the code.
firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
...e-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryContext.java Show resolved Hide resolved
dconeybe requested changes Jul 25, 2023
Contributor
dconeybe left a comment
dconeybe left a comment There was a problem hiding this comment.
Just a few final comments, but LGTM for the most part.
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryContext.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryContext.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryIndexManager.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java Outdated Show resolved Hide resolved
dconeybe approved these changes Jul 26, 2023
davidmotson pushed a commit that referenced this pull request Aug 3, 2023
… query execution locally (#4987) * Add counter * address feedback 1 * add copyright * fix concurrency bug * implement autoClientIndexing * Add tests and fix bugs for BuildTargetIndex * hide getter from public API * move the flag from IndexManager to QueryEngine * Address feedback * move auto index flag to runtime * Support old way to enable persistent for PersistentCacheManager * Polish Tests * Add hide and copyright * clean up unused function * Rename PersistentCacheManager to PersistentCacheIndexManager * Remove unused QueryContext * Address feedbacks other than adding tests and comments * Change the api to match the update * Add tests * Increase tests coverage * Add comments * add configurable min documents to create indexes * Address Denver's feedback * Address feedback * address more feedbacks * use the number getting from 100 ~ 1000 documents experiment * Address feedbacks * improve debug log
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Preparing work for index auto creation feature.
This PR was ported to the web sdk in firebase/firebase-js-sdk#7542.