Skip to content

Delete Overlay Flag #3211

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 8, 2021
Merged

Delete Overlay Flag #3211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f85a5e1
Delete Overlay Flag
wu-hui Dec 7, 2021
0f449ad
Delete some TODOs
wu-hui Dec 7, 2021
11dd88b
More code can be deleted.
wu-hui Dec 7, 2021
cb297fb
More more code to delete.
wu-hui Dec 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 22 additions & 166 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,54 +88,13 @@ IndexManager getIndexManager() {
* for it.
*/
Document getDocument(DocumentKey key) {
if (Persistence.OVERLAY_SUPPORT_ENABLED) {
Mutation overlay = documentOverlayCache.getOverlay(key);
MutableDocument fromOverlay = remoteDocumentCache.get(key);
if (overlay != null) {
overlay.applyToLocalView(fromOverlay, null, Timestamp.now());
}

// TODO(Overlay): Remove below and just return `fromOverlay`.
List<MutationBatch> batches = mutationQueue.getAllMutationBatchesAffectingDocumentKey(key);
Document fromMutationQueue = getDocument(key, batches);
hardAssert(
fromOverlay.equals(fromMutationQueue),
"Document from overlay does not match mutation queue");

return fromOverlay;
Mutation overlay = documentOverlayCache.getOverlay(key);
MutableDocument fromOverlay = remoteDocumentCache.get(key);
if (overlay != null) {
overlay.applyToLocalView(fromOverlay, null, Timestamp.now());
}
List<MutationBatch> batches = mutationQueue.getAllMutationBatchesAffectingDocumentKey(key);
return getDocument(key, batches);
}

// Internal version of {@code getDocument} that allows reusing batches.
private Document getDocument(DocumentKey key, List<MutationBatch> inBatches) {
MutableDocument document = remoteDocumentCache.get(key);
for (MutationBatch batch : inBatches) {
batch.applyToLocalView(document);
}
return document;
}

/**
* Applies the given {@code batches} to the given {@code docs}. The docs are updated to reflect
* the contents of the mutations.
*
* <p>Returns a {@link DocumentKey} to {@link FieldMask} map, representing the fields mutated for
* each document. This is useful to build overlays.
*/
private Map<DocumentKey, FieldMask> applyLocalMutationsToDocuments(
Map<DocumentKey, MutableDocument> docs, List<MutationBatch> batches) {
Map<DocumentKey, FieldMask> changedMasks = new HashMap<>();
for (Map.Entry<DocumentKey, MutableDocument> base : docs.entrySet()) {
FieldMask mask = null;
for (MutationBatch batch : batches) {
mask = batch.applyToLocalView(base.getValue(), mask);
}
changedMasks.put(base.getKey(), mask);
}

return changedMasks;
return fromOverlay;
}

/**
Expand All @@ -160,31 +119,25 @@ ImmutableSortedMap<DocumentKey, Document> getDocuments(Iterable<DocumentKey> key
ImmutableSortedMap<DocumentKey, Document> getLocalViewOfDocuments(
Map<DocumentKey, MutableDocument> docs, Set<DocumentKey> existenceStateChanged) {
ImmutableSortedMap<DocumentKey, Document> results = emptyDocumentMap();
if (Persistence.OVERLAY_SUPPORT_ENABLED) {
Map<DocumentKey, MutableDocument> recalculateDocuments = new HashMap<>();
for (Map.Entry<DocumentKey, MutableDocument> entry : docs.entrySet()) {
Mutation overlay = documentOverlayCache.getOverlay(entry.getKey());
// Recalculate an overlay if the document's existence state is changed due to a remote
// event *and* the overlay is a PatchMutation. This is because document existence state
// can change if some patch mutation's preconditions are met.
// NOTE: we recalculate when `overlay` is null as well, because there might be a patch
// mutation whose precondition does not match before the change (hence overlay==null),
// but would now match.
if (existenceStateChanged.contains(entry.getKey())
&& (overlay == null || overlay instanceof PatchMutation)) {
recalculateDocuments.put(entry.getKey(), docs.get(entry.getKey()));
} else if (overlay != null) {
overlay.applyToLocalView(entry.getValue(), null, Timestamp.now());
}
Map<DocumentKey, MutableDocument> recalculateDocuments = new HashMap<>();
for (Map.Entry<DocumentKey, MutableDocument> entry : docs.entrySet()) {
Mutation overlay = documentOverlayCache.getOverlay(entry.getKey());
// Recalculate an overlay if the document's existence state is changed due to a remote
// event *and* the overlay is a PatchMutation. This is because document existence state
// can change if some patch mutation's preconditions are met.
// NOTE: we recalculate when `overlay` is null as well, because there might be a patch
// mutation whose precondition does not match before the change (hence overlay==null),
// but would now match.
if (existenceStateChanged.contains(entry.getKey())
&& (overlay == null || overlay instanceof PatchMutation)) {
recalculateDocuments.put(entry.getKey(), docs.get(entry.getKey()));
} else if (overlay != null) {
overlay.applyToLocalView(entry.getValue(), null, Timestamp.now());
}

recalculateAndSaveOverlays(recalculateDocuments);
} else {
List<MutationBatch> batches =
mutationQueue.getAllMutationBatchesAffectingDocumentKeys(docs.keySet());
applyLocalMutationsToDocuments(docs, batches);
}

recalculateAndSaveOverlays(recalculateDocuments);

for (Map.Entry<DocumentKey, MutableDocument> entry : docs.entrySet()) {
results = results.insert(entry.getKey(), entry.getValue());
}
Expand Down Expand Up @@ -295,31 +248,9 @@ private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollection
return results;
}

/** Queries the remote documents and overlays by doing a full collection scan. */
private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollectionQuery(
Query query, IndexOffset offset) {
if (Persistence.OVERLAY_SUPPORT_ENABLED) {
// TODO(Overlay): Remove the assert and just return `fromOverlay`.
ImmutableSortedMap<DocumentKey, Document> fromOverlay =
getDocumentsMatchingCollectionQueryFromOverlayCache(query, offset);
// TODO(Overlay): Delete below before merging. The code passes, but there are tests
// looking at how many documents read from remote document, and this would double
// the count.
/*
ImmutableSortedMap<DocumentKey, Document> fromMutationQueue =
getDocumentsMatchingCollectionQueryFromMutationQueue(query, sinceReadTime);
hardAssert(
fromOverlay.equals(fromMutationQueue),
"Documents from overlay do not match mutation queue version.");
*/
return fromOverlay;
} else {
return getDocumentsMatchingCollectionQueryFromMutationQueue(query, offset);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}

/** Queries the remote documents and overlays by doing a full collection scan. */
private ImmutableSortedMap<DocumentKey, Document>
getDocumentsMatchingCollectionQueryFromOverlayCache(Query query, IndexOffset offset) {
ImmutableSortedMap<DocumentKey, MutableDocument> remoteDocuments =
remoteDocumentCache.getAllDocumentsMatchingQuery(query, offset);
Map<DocumentKey, Mutation> overlays = documentOverlayCache.getOverlays(query.getPath(), -1);
Expand Down Expand Up @@ -352,79 +283,4 @@ private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollection

return results;
}

/** Queries the remote documents and mutation queue, by doing a full collection scan. */
private ImmutableSortedMap<DocumentKey, Document>
getDocumentsMatchingCollectionQueryFromMutationQueue(Query query, IndexOffset offset) {
ImmutableSortedMap<DocumentKey, MutableDocument> remoteDocuments =
remoteDocumentCache.getAllDocumentsMatchingQuery(query, offset);

// TODO(indexing): We should plumb sinceReadTime through to the mutation queue
List<MutationBatch> matchingBatches = mutationQueue.getAllMutationBatchesAffectingQuery(query);

remoteDocuments = addMissingBaseDocuments(matchingBatches, remoteDocuments);

for (MutationBatch batch : matchingBatches) {
for (Mutation mutation : batch.getMutations()) {
// Only process documents belonging to the collection.
if (!query.getPath().isImmediateParentOf(mutation.getKey().getPath())) {
continue;
}

DocumentKey key = mutation.getKey();
MutableDocument document = remoteDocuments.get(key);
if (document == null) {
// Create invalid document to apply mutations on top of
document = MutableDocument.newInvalidDocument(key);
remoteDocuments = remoteDocuments.insert(key, document);
}
mutation.applyToLocalView(
document, FieldMask.fromSet(new HashSet<>()), batch.getLocalWriteTime());
if (!document.isFoundDocument()) {
remoteDocuments = remoteDocuments.remove(key);
}
}
}

ImmutableSortedMap<DocumentKey, Document> results = emptyDocumentMap();
for (Map.Entry<DocumentKey, MutableDocument> docEntry : remoteDocuments) {
// Finally, insert the documents that still match the query
if (query.matches(docEntry.getValue())) {
results = results.insert(docEntry.getKey(), docEntry.getValue());
}
}

return results;
}

/**
* It is possible that a {@code PatchMutation} can make a document match a query, even if the
* version in the {@code RemoteDocumentCache} is not a match yet (waiting for server to ack). To
* handle this, we find all document keys affected by the {@code PatchMutation}s that are not in
* {@code existingDocs} yet, and back fill them via {@code remoteDocumentCache.getAll}, otherwise
* those {@code PatchMutation}s will be ignored because no base document can be found, and lead to
* missing results for the query.
*/
private ImmutableSortedMap<DocumentKey, MutableDocument> addMissingBaseDocuments(
List<MutationBatch> matchingBatches,
ImmutableSortedMap<DocumentKey, MutableDocument> existingDocs) {
HashSet<DocumentKey> missingDocKeys = new HashSet<>();
for (MutationBatch batch : matchingBatches) {
for (Mutation mutation : batch.getMutations()) {
if (mutation instanceof PatchMutation && !existingDocs.containsKey(mutation.getKey())) {
missingDocKeys.add(mutation.getKey());
}
}
}

ImmutableSortedMap<DocumentKey, MutableDocument> mergedDocs = existingDocs;
Map<DocumentKey, MutableDocument> missingDocs = remoteDocumentCache.getAll(missingDocKeys);
for (Map.Entry<DocumentKey, MutableDocument> entry : missingDocs.entrySet()) {
if (entry.getValue().isFoundDocument()) {
mergedDocs = mergedDocs.insert(entry.getKey(), entry.getValue());
}
}

return mergedDocs;
}
}
16 changes: 5 additions & 11 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,7 @@ public LocalWriteResult writeLocally(List<Mutation> mutations) {
MutationBatch batch =
mutationQueue.addMutationBatch(localWriteTime, baseMutations, mutations);
Map<DocumentKey, Mutation> overlays = batch.applyToLocalDocumentSet(documents);
if (Persistence.OVERLAY_SUPPORT_ENABLED) {
documentOverlayCache.saveOverlays(batch.getBatchId(), overlays);
}
documentOverlayCache.saveOverlays(batch.getBatchId(), overlays);
return new LocalWriteResult(batch.getBatchId(), documents);
});
}
Expand Down Expand Up @@ -310,10 +308,8 @@ public ImmutableSortedMap<DocumentKey, Document> acknowledgeBatch(
applyWriteToRemoteDocuments(batchResult);
mutationQueue.performConsistencyCheck();

if (Persistence.OVERLAY_SUPPORT_ENABLED) {
documentOverlayCache.removeOverlaysForBatchId(batchResult.getBatch().getBatchId());
localDocuments.recalculateAndSaveOverlays(getKeysWithTransformResults(batchResult));
}
documentOverlayCache.removeOverlaysForBatchId(batchResult.getBatch().getBatchId());
localDocuments.recalculateAndSaveOverlays(getKeysWithTransformResults(batchResult));

return localDocuments.getDocuments(batch.getKeys());
});
Expand Down Expand Up @@ -350,10 +346,8 @@ public ImmutableSortedMap<DocumentKey, Document> rejectBatch(int batchId) {
mutationQueue.removeMutationBatch(toReject);
mutationQueue.performConsistencyCheck();

if (Persistence.OVERLAY_SUPPORT_ENABLED) {
documentOverlayCache.removeOverlaysForBatchId(batchId);
localDocuments.recalculateAndSaveOverlays(toReject.getKeys());
}
documentOverlayCache.removeOverlaysForBatchId(batchId);
localDocuments.recalculateAndSaveOverlays(toReject.getKeys());

return localDocuments.getDocuments(toReject.getKeys());
});
Expand Down
3 changes: 0 additions & 3 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@
public abstract class Persistence {
static final String TAG = Persistence.class.getSimpleName();

/** Temporary setting for enabling document overlays. */
// TODO(Overlay): Remove this.
public static boolean OVERLAY_SUPPORT_ENABLED = true;
/** Constant string to indicate a data migration is required to support overlays. */
public static String DATA_MIGRATION_BUILD_OVERLAYS = "BUILD_OVERLAYS";

Expand Down
3 changes: 0 additions & 3 deletions ...tore/src/main/java/com/google/firebase/firestore/local/SQLiteOverlayMigrationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@ public SQLiteOverlayMigrationManager(SQLitePersistence persistence) {

@Override
public void run() {
if (Persistence.OVERLAY_SUPPORT_ENABLED == false || !hasPendingOverlayMigration()) {
return;
}
buildOverlays();
}

Expand Down
2 changes: 0 additions & 2 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,6 @@ void runSchemaUpgrades(int fromVersion, int toVersion) {
}

if (fromVersion < 14 && toVersion >= 14) {
Preconditions.checkState(
Persistence.OVERLAY_SUPPORT_ENABLED || Persistence.INDEXING_SUPPORT_ENABLED);
createOverlays();
createDataMigrationTable();
addPendingDataMigration(Persistence.DATA_MIGRATION_BUILD_OVERLAYS);
Expand Down
8 changes: 2 additions & 6 deletions firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,12 +978,8 @@ public void testReadsAllDocumentsForInitialCollectionQueries() {
localStore.executeQuery(query, /* usePreviousResults= */ true);

assertRemoteDocumentsRead(/* byKey= */ 0, /* byQuery= */ 2);
if (Persistence.OVERLAY_SUPPORT_ENABLED) {
// No mutations are read because only overlay is needed.
assertMutationsRead(/* byKey= */ 0, /* byQuery= */ 0);
} else {
assertMutationsRead(/* byKey= */ 0, /* byQuery= */ 1);
}
// No mutations are read because only overlay is needed.
assertMutationsRead(/* byKey= */ 0, /* byQuery= */ 0);
}

@Test
Expand Down