fix latency-compensated query #3363
Conversation
ghost
added
the
ghost
force-pushed
the
wuandy/FixLatencyCompPatchQuery
branch
from
649c8a0 to
47190a2
Compare
ghost
force-pushed
the
wuandy/FixLatencyCompPatchQuery
branch
from
cb9c572 to
aa85e8d
Compare
| * also has every document in `existingDocs`. | ||
| */ | ||
| model::DocumentMap AddMissingBaseDocument( | ||
| const std::vector<FSTMutationBatch*>& matchingBatches, |
There was a problem hiding this comment.
Nit: C++ uses snake_case for variable names (this can be a little confusing because Objective-C uses camelCase). We consider files under core/src to be C++ and files under Source to be Objective-C, so this file should follow C++ style rules. Some of the surrounding code gets this wrong (no need to fix anything you didn't add in this PR).
Applies throughout.
| * `existingDocs` add them to the returning `DocumentMap`. The returned map | ||
| * also has every document in `existingDocs`. | ||
| */ | ||
| model::DocumentMap AddMissingBaseDocument( |
There was a problem hiding this comment.
Nit: pluralize the name (AddMissingBaseDocuments).
| @@ -215,6 +217,38 @@ | |||
| return results; | |||
| } | |||
|
|
|||
| // It is possible that a `PatchMutation` can make a document match a query, even | |||
There was a problem hiding this comment.
Optional: I'm not against adding a new comment for this function in the header, but you could simply move this comment to the header (replacing the current one) as well.
| MaybeDocumentMap missingDocs = remote_document_cache_->GetAll(missingDocKeys); | ||
| for (const auto& kv : missingDocs) { | ||
| if (kv.second != nil && [kv.second isKindOfClass:[FSTDocument class]]) { | ||
| mergedDocs = mergedDocs.insert(kv.first, (FSTDocument*)kv.second); |
There was a problem hiding this comment.
Nit: please use static_cast to make casting more visible:
static_cast<FSTDocument*>(kv.second)Note that C++ casts work fine on Objective-C classes (in Objective-C++ mode).
| // if the version in the `RemoteDocumentCache` is not a match yet (waiting for | ||
| // server to ack). To handle this, we find all document keys affected by the | ||
| // `PatchMutation`s that are not in `existingDocs` yet, and back fill them via | ||
| // `remoteDocumentCache.getAll`, otherwise those `PatchMutation`s will be |
There was a problem hiding this comment.
Nit: remote_document_cache.GetAll().
| // for the query. | ||
| DocumentMap LocalDocumentsView::AddMissingBaseDocument( | ||
| const std::vector<FSTMutationBatch*>& matchingBatches, | ||
| const DocumentMap& existingDocs) { |
There was a problem hiding this comment.
This function's signature mandates an unnecessary copy. Even though DocumentMap is relatively cheap to copy, it strikes me as an avoidable inefficiency. I'd suggest moving the map instead. Make the function take existing_docs by value:
DocumentMap existing_docs) {and then move results into the function parameter at the call point:
results = AddMissingBaseDocuments(matching_batches, std::move(results));| } | ||
| } | ||
|
|
||
| DocumentMap mergedDocs = existingDocs; |
There was a problem hiding this comment.
In C++, this copy isn't entirely free. If you follow the suggestion above to make existing_docs a value, you will no longer need this copy, and I'd suggest you omit it.
| DocumentMap mergedDocs = existingDocs; | ||
| MaybeDocumentMap missingDocs = remote_document_cache_->GetAll(missingDocKeys); | ||
| for (const auto& kv : missingDocs) { | ||
| if (kv.second != nil && [kv.second isKindOfClass:[FSTDocument class]]) { |
There was a problem hiding this comment.
Optional: kv.second is repeated three times in this loop, consider adding a variable:
FSTMaybeDocument* maybe_doc = kv.second;
Firestore/CHANGELOG.md
Outdated
| @@ -1,6 +1,8 @@ | |||
| # Unreleased | |||
|
|
|||
| # 1.4.2 | |||
| - [fixed] Fixed an issue where query results were temporarily missing documents | |||
| that previously had not matched but had been updated to now match the query. | |||
There was a problem hiding this comment.
It's OK to refer to issues in other repos, like we've done below. In this case it would be https://github.com/firebase/firebase-android-sdk/issues/155.
| @@ -103,6 +103,18 @@ class LocalDocumentsView { | |||
| /** Queries the remote documents and overlays mutations. */ | |||
| model::DocumentMap GetDocumentsMatchingCollectionQuery(FSTQuery* query); | |||
|
|
|||
| /** It is possible that a `PatchMutation` can make a document match a query, | |||
There was a problem hiding this comment.
Nit: in a multiline comment, /** should be on its own line:
/**
* It is possible that a `PatchMutation` can make a document match a query,
No description provided.