-
Notifications
You must be signed in to change notification settings - Fork 578
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
Add option to allow SDK create cache indexes automatically to improve query execution locally #4987
Conversation
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>
|
Coverage Report 1Affected Products
Test Logs |
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. |
Size Report 1Affected Products
Test Logs |
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Notes
Startup Times
|
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.
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
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. |
79b1fd2
to
8aed796
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.
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
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
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. |
firebase-firestore/src/main/java/com/google/firebase/firestore/model/TargetIndexMatcher.java
Show resolved
Hide resolved
e5b8859
to
1e25c19
Compare
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
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. |
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. |
d48504b
to
ad7fc61
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.
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
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
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.
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
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 with some nits.
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
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.
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
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.
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
… 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
Preparing work for index auto creation feature.
This PR was ported to the web sdk in firebase/firebase-js-sdk#7542.