Merge the Index Engines #3208
Merged
Merge the Index Engines #3208
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
schmidt-sebastian
force-pushed
the
mrschmidt/onequeryengine
branch
from
0abaa11 to
efbd10c
Compare
Comment on lines
137
to
224
| if (lastLimboFreeSnapshotVersion.equals(SnapshotVersion.NONE)) { | ||
| // Queries that have never seen a snapshot without limbo free documents should be run as a | ||
| // full collection scan. | ||
| return null; | ||
| } | ||
|
|
||
| ImmutableSortedMap<DocumentKey, Document> documents = | ||
| localDocumentsView.getDocuments(remoteKeys); | ||
| ImmutableSortedSet<Document> previousResults = applyQuery(query, documents); | ||
|
|
||
| if ((query.hasLimitToFirst() || query.hasLimitToLast()) | ||
| && needsRefill( | ||
| query.getLimitType(), previousResults, remoteKeys, lastLimboFreeSnapshotVersion)) { | ||
| return null; | ||
| } | ||
|
|
||
| if (Logger.isDebugEnabled()) { | ||
| Logger.debug( | ||
| LOG_TAG, | ||
| "Re-using previous result from %s to execute query: %s", | ||
| lastLimboFreeSnapshotVersion.toString(), | ||
| query.toString()); | ||
| } | ||
|
|
||
| return mergeResults(previousResults, query, IndexOffset.create(lastLimboFreeSnapshotVersion)); | ||
| } | ||
|
|
||
| /** Applies the query filter and sorting to the provided documents. */ | ||
| private ImmutableSortedSet<Document> applyQuery( | ||
| Query query, ImmutableSortedMap<DocumentKey, Document> documents) { | ||
| // Sort the documents and re-apply the query filter since previously matching documents do not | ||
| // necessarily still match the query. | ||
| ImmutableSortedSet<Document> queryResults = | ||
| new ImmutableSortedSet<>(Collections.emptyList(), query.comparator()); | ||
| for (Map.Entry<DocumentKey, Document> entry : documents) { | ||
| Document document = entry.getValue(); | ||
| if (query.matches(document)) { | ||
| queryResults = queryResults.insert(document); | ||
| } | ||
| } | ||
| return queryResults; | ||
| } | ||
|
|
||
| /** | ||
| * Determines if a limit query needs to be refilled from cache, making it ineligible for | ||
| * index-free execution. | ||
| * | ||
| * @param limitType The type of limit query for refill calculation. | ||
| * @param sortedPreviousResults The documents that matched the query when it was last | ||
| * synchronized, sorted by the query's comparator. | ||
| * @param remoteKeys The document keys that matched the query at the last snapshot. | ||
| * @param limboFreeSnapshotVersion The version of the snapshot when the query was last | ||
| * synchronized. | ||
| */ | ||
| private boolean needsRefill( | ||
| Query.LimitType limitType, | ||
| ImmutableSortedSet<Document> sortedPreviousResults, | ||
| ImmutableSortedSet<DocumentKey> remoteKeys, | ||
| SnapshotVersion limboFreeSnapshotVersion) { | ||
| // The query needs to be refilled if a previously matching document no longer matches. | ||
| if (remoteKeys.size() != sortedPreviousResults.size()) { | ||
| return true; | ||
| } | ||
|
|
||
| // Limit queries are not eligible for index-free query execution if there is a potential that an | ||
| // older document from cache now sorts before a document that was previously part of the limit. | ||
| // This, however, can only happen if the document at the edge of the limit goes out of limit. If | ||
| // a document that is not the limit boundary sorts differently, the boundary of the limit itself | ||
| // did not change and documents from cache will continue to be "rejected" by this boundary. | ||
| // Therefore, we can ignore any modifications that don't affect the last document. | ||
| Document documentAtLimitEdge = | ||
| limitType == Query.LimitType.LIMIT_TO_FIRST | ||
| ? sortedPreviousResults.getMaxEntry() | ||
| : sortedPreviousResults.getMinEntry(); | ||
| if (documentAtLimitEdge == null) { | ||
| // We don't need to refill the query if there were already no documents. | ||
| return false; | ||
| } | ||
| return documentAtLimitEdge.hasPendingWrites() | ||
| || documentAtLimitEdge.getVersion().compareTo(limboFreeSnapshotVersion) > 0; | ||
| } | ||
|
|
||
| private ImmutableSortedMap<DocumentKey, Document> executeFullCollectionScan(Query query) { | ||
| if (Logger.isDebugEnabled()) { | ||
| Logger.debug(LOG_TAG, "Using full collection scan to execute query: %s", query.toString()); | ||
| } | ||
| return localDocumentsView.getDocumentsMatchingQuery(query, IndexOffset.NONE); | ||
| } |
There was a problem hiding this comment.
All of this code is moved from DefaultQueryEngine.
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (4f8b6e54) is created by Prow via merging commits: dfa2231 7d109b8. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (4f8b6e54) is created by Prow via merging commits: dfa2231 7d109b8. |
|
/test device-check-changed |
|
/test device-check-changed Checking if |
Doing the lords work. (These two tests have been failing a lot since the GRPC update) |
thebrianchen
approved these changes
Dec 8, 2021
|
|
||
| public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery( | ||
| Query query, | ||
| ImmutableSortedSet<DocumentKey> remoteKeys, |
There was a problem hiding this comment.
Any reason you switched remoteKeys and lastLimboFreeSnapshotVersion around? Would prefer having the signature the same to reduce merge conflicts.
There was a problem hiding this comment.
At some point during my iterations on this PR, it felt like the right thing to do. I don't think this is needed anymore and reducing merge conflicts is a good reason to revert this.
| @@ -504,6 +504,10 @@ private void deleteIndexEntry(Document document, IndexEntry indexEntry) { | |||
| public FieldIndex getFieldIndex(Target target) { | |||
There was a problem hiding this comment.
Optional: add @Nullable annotation.
There was a problem hiding this comment.
It already has one :)
| return localDocumentsView.getDocumentsMatchingQuery(query, IndexOffset.NONE); | ||
| } | ||
|
|
||
| private ImmutableSortedMap<DocumentKey, Document> mergeResults( |
There was a problem hiding this comment.
nit: Either add comment or change method name to include the fact that we're merging results with LDV's new documents since the offset
There was a problem hiding this comment.
Done (did both)
|
/test device-check-changed |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR removes all Index Engines in favor one of single index engine that does Indexed queries by default, then falls back to index-free if no index is available and then falls back to collection scans.