Skip to content

Index-Free (2/6): Add update_time to SQLRemoteDocumentCache schema #615

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 8 commits into from
Jul 23, 2019
Merged

Index-Free (2/6): Add update_time to SQLRemoteDocumentCache schema #615

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cce8fd0
Adding LocalStoreTestHelper for NO_CHANGE event
schmidt-sebastian Jul 14, 2019
3d2d896
Add update_time to SQLRemoteDocumentCache schema
schmidt-sebastian Jul 14, 2019
9de2df8
Fix comment
schmidt-sebastian Jul 14, 2019
2bf12c2
Update ktx's test-util
schmidt-sebastian Jul 15, 2019
b9a3664
Merge branch 'mrschmidt/indexfree-1' into mrschmidt/indexfree-2
schmidt-sebastian Jul 15, 2019
ddee17b
Use seconds and nanos
schmidt-sebastian Jul 18, 2019
5679784
Merge
schmidt-sebastian Jul 22, 2019
b77194a
Address comments
schmidt-sebastian Jul 22, 2019
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
30 changes: 0 additions & 30 deletions firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,36 +138,6 @@ public static QuerySnapshot querySnapshot(
return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE);
}

/**
* An implementation of TargetMetadataProvider that provides controlled access to the
* `TargetMetadataProvider` callbacks. Any target accessed via these callbacks must be registered
* beforehand via `setSyncedKeys()`.
*/
public static class TestTargetMetadataProvider
implements WatchChangeAggregator.TargetMetadataProvider {
final Map<Integer, ImmutableSortedSet<DocumentKey>> syncedKeys = new HashMap<>();
final Map<Integer, QueryData> queryData = new HashMap<>();

@Override
public ImmutableSortedSet<DocumentKey> getRemoteKeysForTarget(int targetId) {
return syncedKeys.get(targetId) != null
? syncedKeys.get(targetId)
: DocumentKey.emptyKeySet();
}

@Nullable
@Override
public QueryData getQueryDataForTarget(int targetId) {
return queryData.get(targetId);
}

/** Sets or replaces the local state for the provided query data. */
public void setSyncedKeys(QueryData queryData, ImmutableSortedSet<DocumentKey> keys) {
this.queryData.put(queryData.getTargetId(), queryData);
this.syncedKeys.put(queryData.getTargetId(), keys);
}
}

public static <T> T waitFor(Task<T> task) {
if (!task.isComplete()) {
Robolectric.flushBackgroundThreadScheduler();
Expand Down
6 changes: 5 additions & 1 deletion ...irestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.MaybeDocument;
import com.google.firebase.firestore.model.ResourcePath;
import com.google.firebase.firestore.model.SnapshotVersion;
import com.google.firebase.firestore.util.BackgroundQueue;
import com.google.firebase.firestore.util.Executors;
import com.google.protobuf.InvalidProtocolBufferException;
Expand Down Expand Up @@ -51,13 +52,16 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache {
@Override
public void add(MaybeDocument maybeDocument) {
String path = pathForKey(maybeDocument.getKey());
SnapshotVersion snapshotVersion = maybeDocument.getVersion();
MessageLite message = serializer.encodeMaybeDocument(maybeDocument);

statsCollector.recordRowsWritten(STATS_TAG, 1);

db.execute(
"INSERT OR REPLACE INTO remote_documents (path, contents) VALUES (?, ?)",
"INSERT OR REPLACE INTO remote_documents (path, snapshot_version_micros, contents) "
+ "VALUES (?, ?, ?)",
path,
snapshotVersion.toMicroseconds(),
message.toByteArray());

db.getIndexManager().addToCollectionParentIndex(maybeDocument.getKey().getPath().popLast());
Expand Down
12 changes: 11 additions & 1 deletion 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 @@ -46,7 +46,7 @@ class SQLiteSchema {
* The version of the schema. Increase this by one for each migration added to runMigrations
* below.
*/
static final int VERSION = 8;
static final int VERSION = 9;
// Remove this constant and increment VERSION to enable indexing support
static final int INDEXING_SUPPORT_VERSION = VERSION + 1;

Expand Down Expand Up @@ -127,6 +127,10 @@ void runMigrations(int fromVersion, int toVersion) {
createV8CollectionParentsIndex();
}

if (fromVersion < 9 && toVersion >= 9) {
addUpdateTime();
}

/*
* Adding a new migration? READ THIS FIRST!
*
Expand Down Expand Up @@ -351,6 +355,12 @@ private void addSequenceNumber() {
}
}

private void addUpdateTime() {
if (!tableContainsColumn("remote_documents", "snapshot_version_micros")) {
db.execSQL("ALTER TABLE remote_documents ADD COLUMN snapshot_version_micros INTEGER");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If all the documents in the cache have a zero version doesn't that mean the query to find recently updated documents has to also check for zero? Doesn't that nearly nullify the benefits of index-free on any device that has pre-existing data?

Consider an existing device containing two targets, A updated at T1 matching D1 and D2. There's also a second target, listened to later: B updated at T2 matching D3. D3 actually matches the query for target A. All of this happened before this schema upgrade, so after the upgrade they'll all have update_time of zero.

If the user queries on A again we need to find D3, but it's going to have a zero update_time.

I think we're better off either:

  • actually populating update_time based on the value from inside the document.
  • dropping the contents of the remote_documents table

Though the latter is pretty unfriendly so I think we should only consider it if all other possibilities are exhausted.

Alternatively, maybe you have something up your sleeve to handle this case that I'm not seeing :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to describe my strategy for this in the PR comment, but added it as a comment in the code itself for more visibility. Basically - I haven't quite decided what the best way forward it as migrating the contents of the entire RemoteDocumentCache could be quite slow. I decided to punt on this till later (when we actually want to enable this).

}

/**
* Ensures that each entry in the remote document cache has a corresponding sentinel row. Any
* entries that lack a sentinel row are given one with the sequence number set to the highest
Expand Down
7 changes: 7 additions & 0 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/model/SnapshotVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.firebase.firestore.model;

import com.google.firebase.Timestamp;
import java.util.concurrent.TimeUnit;

/**
* A version of a document in Firestore. This corresponds to the version timestamp, such as
Expand All @@ -36,6 +37,12 @@ public Timestamp getTimestamp() {
return timestamp;
}

/** Returns the microseconds since EPOCH that this snapshot version represents. */
public long toMicroseconds() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Historically we've tried to avoid baking in this assumption about backend timestamp resolution into our system. This is because the backend currently truncates this time but they have an open bug for fixing this.

Meanwhile we've already established a precedent for how to represent timestamps literally in our schema. For example, the targets table includes the following:

                  + "snapshot_version_seconds INTEGER, "
                  + "snapshot_version_nanos INTEGER, "

It seems like we lose nothing by representing these accurately--even if we only query on seconds we'll be fine for the purposes of index-free querying--so it seems like we should follow the established precedent and represent timestamps at full resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this PR to use the same seconds/nanos approach as the targets table. I didn't want to do this originally cause it would result in some overselection, but I assume that you are correct when you say that we don't need to worry about this here.

return TimeUnit.MICROSECONDS.convert(timestamp.getSeconds(), TimeUnit.SECONDS)
+ TimeUnit.MICROSECONDS.convert(timestamp.getNanoseconds(), TimeUnit.NANOSECONDS);
}

@Override
public int compareTo(SnapshotVersion other) {
return timestamp.compareTo(other.timestamp);
Expand Down
34 changes: 0 additions & 34 deletions firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,17 @@
import static com.google.firebase.firestore.testutil.TestUtil.key;
import static org.mockito.Mockito.mock;

import androidx.annotation.Nullable;
import com.google.android.gms.tasks.Task;
import com.google.firebase.database.collection.ImmutableSortedSet;
import com.google.firebase.firestore.core.DocumentViewChange;
import com.google.firebase.firestore.core.DocumentViewChange.Type;
import com.google.firebase.firestore.core.ViewSnapshot;
import com.google.firebase.firestore.local.QueryData;
import com.google.firebase.firestore.model.Document;
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.DocumentSet;
import com.google.firebase.firestore.model.ResourcePath;
import com.google.firebase.firestore.model.value.ObjectValue;
import com.google.firebase.firestore.remote.WatchChangeAggregator;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.junit.Assert;
Expand Down Expand Up @@ -138,36 +134,6 @@ public static QuerySnapshot querySnapshot(
return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE);
}

/**
* An implementation of TargetMetadataProvider that provides controlled access to the
* `TargetMetadataProvider` callbacks. Any target accessed via these callbacks must be registered
* beforehand via `setSyncedKeys()`.
*/
public static class TestTargetMetadataProvider
implements WatchChangeAggregator.TargetMetadataProvider {
final Map<Integer, ImmutableSortedSet<DocumentKey>> syncedKeys = new HashMap<>();
final Map<Integer, QueryData> queryData = new HashMap<>();

@Override
public ImmutableSortedSet<DocumentKey> getRemoteKeysForTarget(int targetId) {
return syncedKeys.get(targetId) != null
? syncedKeys.get(targetId)
: DocumentKey.emptyKeySet();
}

@Nullable
@Override
public QueryData getQueryDataForTarget(int targetId) {
return queryData.get(targetId);
}

/** Sets or replaces the local state for the provided query data. */
public void setSyncedKeys(QueryData queryData, ImmutableSortedSet<DocumentKey> keys) {
this.queryData.put(queryData.getTargetId(), queryData);
this.syncedKeys.put(queryData.getTargetId(), keys);
}
}

public static <T> T waitFor(Task<T> task) {
if (!task.isComplete()) {
Robolectric.flushBackgroundThreadScheduler();
Expand Down
44 changes: 6 additions & 38 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 @@ -21,6 +21,7 @@
import static com.google.firebase.firestore.testutil.TestUtil.doc;
import static com.google.firebase.firestore.testutil.TestUtil.key;
import static com.google.firebase.firestore.testutil.TestUtil.map;
import static com.google.firebase.firestore.testutil.TestUtil.noChangeEvent;
import static com.google.firebase.firestore.testutil.TestUtil.patchMutation;
import static com.google.firebase.firestore.testutil.TestUtil.query;
import static com.google.firebase.firestore.testutil.TestUtil.resumeToken;
Expand All @@ -46,7 +47,6 @@
import com.google.firebase.database.collection.ImmutableSortedMap;
import com.google.firebase.database.collection.ImmutableSortedSet;
import com.google.firebase.firestore.FieldValue;
import com.google.firebase.firestore.TestUtil.TestTargetMetadataProvider;
import com.google.firebase.firestore.auth.User;
import com.google.firebase.firestore.core.Query;
import com.google.firebase.firestore.model.Document;
Expand All @@ -61,13 +61,9 @@
import com.google.firebase.firestore.model.mutation.MutationResult;
import com.google.firebase.firestore.model.mutation.SetMutation;
import com.google.firebase.firestore.remote.RemoteEvent;
import com.google.firebase.firestore.remote.WatchChange.WatchTargetChange;
import com.google.firebase.firestore.remote.WatchChange.WatchTargetChangeType;
import com.google.firebase.firestore.remote.WatchChangeAggregator;
import com.google.firebase.firestore.remote.WatchStream;
import com.google.firebase.firestore.remote.WriteStream;
import com.google.firebase.firestore.testutil.TestUtil;
import com.google.protobuf.ByteString;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -912,26 +908,15 @@ public void testPersistsResumeTokens() {

Query query = query("foo/bar");
int targetId = allocateQuery(query);
ByteString resumeToken = resumeToken(1000);

QueryData queryData = TestUtil.queryData(targetId, QueryPurpose.LISTEN, "foo/bar");
TestTargetMetadataProvider testTargetMetadataProvider = new TestTargetMetadataProvider();
testTargetMetadataProvider.setSyncedKeys(queryData, DocumentKey.emptyKeySet());

WatchChangeAggregator aggregator = new WatchChangeAggregator(testTargetMetadataProvider);

WatchTargetChange watchChange =
new WatchTargetChange(WatchTargetChangeType.Current, asList(targetId), resumeToken);
aggregator.handleTargetChange(watchChange);
RemoteEvent remoteEvent = aggregator.createRemoteEvent(version(1000));
applyRemoteEvent(remoteEvent);
applyRemoteEvent(noChangeEvent(targetId, 1000));

// Stop listening so that the query should become inactive (but persistent)
localStore.releaseQuery(query);

// Should come back with the same resume token
QueryData queryData2 = localStore.allocateQuery(query);
assertEquals(resumeToken, queryData2.getResumeToken());
assertEquals(resumeToken(1000), queryData2.getResumeToken());
}

@Test
Expand All @@ -943,35 +928,18 @@ public void testDoesNotReplaceResumeTokenWithEmptyByteString() {

Query query = query("foo/bar");
int targetId = allocateQuery(query);
ByteString resumeToken = resumeToken(1000);

QueryData queryData = TestUtil.queryData(targetId, QueryPurpose.LISTEN, "foo/bar");
TestTargetMetadataProvider testTargetMetadataProvider = new TestTargetMetadataProvider();
testTargetMetadataProvider.setSyncedKeys(queryData, DocumentKey.emptyKeySet());

WatchChangeAggregator aggregator1 = new WatchChangeAggregator(testTargetMetadataProvider);

WatchTargetChange watchChange1 =
new WatchTargetChange(WatchTargetChangeType.Current, asList(targetId), resumeToken);
aggregator1.handleTargetChange(watchChange1);
RemoteEvent remoteEvent1 = aggregator1.createRemoteEvent(version(1000));
applyRemoteEvent(remoteEvent1);
applyRemoteEvent(noChangeEvent(targetId, 1000));

// New message with empty resume token should not replace the old resume token
WatchChangeAggregator aggregator2 = new WatchChangeAggregator(testTargetMetadataProvider);
WatchTargetChange watchChange2 =
new WatchTargetChange(
WatchTargetChangeType.Current, asList(targetId), WatchStream.EMPTY_RESUME_TOKEN);
aggregator2.handleTargetChange(watchChange2);
RemoteEvent remoteEvent2 = aggregator2.createRemoteEvent(version(2000));
applyRemoteEvent(remoteEvent2);
applyRemoteEvent(TestUtil.noChangeEvent(targetId, 2000, WatchStream.EMPTY_RESUME_TOKEN));

// Stop listening so that the query should become inactive (but persistent)
localStore.releaseQuery(query);

// Should come back with the same resume token
QueryData queryData2 = localStore.allocateQuery(query);
assertEquals(resumeToken, queryData2.getResumeToken());
assertEquals(resumeToken(1000), queryData2.getResumeToken());
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteEventTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@
import static org.junit.Assert.fail;

import com.google.firebase.database.collection.ImmutableSortedSet;
import com.google.firebase.firestore.TestUtil.TestTargetMetadataProvider;
import com.google.firebase.firestore.local.QueryData;
import com.google.firebase.firestore.model.Document;
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.NoDocument;
import com.google.firebase.firestore.remote.WatchChange.DocumentChange;
import com.google.firebase.firestore.remote.WatchChange.WatchTargetChange;
import com.google.firebase.firestore.remote.WatchChange.WatchTargetChangeType;
import com.google.firebase.firestore.testutil.TestUtil.TestTargetMetadataProvider;
import com.google.protobuf.ByteString;
import java.util.ArrayList;
import java.util.Collections;
Expand Down
49 changes: 49 additions & 0 deletions firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import com.google.firebase.firestore.model.value.ObjectValue;
import com.google.firebase.firestore.remote.RemoteEvent;
import com.google.firebase.firestore.remote.TargetChange;
import com.google.firebase.firestore.remote.WatchChange;
import com.google.firebase.firestore.remote.WatchChange.DocumentChange;
import com.google.firebase.firestore.remote.WatchChangeAggregator;
import com.google.protobuf.ByteString;
Expand Down Expand Up @@ -374,6 +375,24 @@ public static Map<Integer, QueryData> activeLimboQueries(String docKey, Integer.
return activeLimboQueries(docKey, asList(targets));
}

public static RemoteEvent noChangeEvent(int targetId, int version) {
return noChangeEvent(targetId, version, resumeToken(version));
}

public static RemoteEvent noChangeEvent(int targetId, int version, ByteString resumeToken) {
QueryData queryData = TestUtil.queryData(targetId, QueryPurpose.LISTEN, "foo/bar");
TestTargetMetadataProvider testTargetMetadataProvider = new TestTargetMetadataProvider();
testTargetMetadataProvider.setSyncedKeys(queryData, DocumentKey.emptyKeySet());

WatchChangeAggregator aggregator = new WatchChangeAggregator(testTargetMetadataProvider);

WatchChange.WatchTargetChange watchChange =
new WatchChange.WatchTargetChange(
WatchChange.WatchTargetChangeType.NoChange, asList(targetId), resumeToken);
aggregator.handleTargetChange(watchChange);
return aggregator.createRemoteEvent(version(version));
}

public static RemoteEvent addedRemoteEvent(
MaybeDocument doc, List<Integer> updatedInTargets, List<Integer> removedFromTargets) {
DocumentChange change =
Expand Down Expand Up @@ -526,6 +545,36 @@ public static ByteString streamToken(String contents) {
return ByteString.copyFrom(contents, Charsets.UTF_8);
}

/**
* An implementation of TargetMetadataProvider that provides controlled access to the
* `TargetMetadataProvider` callbacks. Any target accessed via these callbacks must be registered
* beforehand via `setSyncedKeys()`.
*/
public static class TestTargetMetadataProvider
implements WatchChangeAggregator.TargetMetadataProvider {
final Map<Integer, ImmutableSortedSet<DocumentKey>> syncedKeys = new HashMap<>();
final Map<Integer, QueryData> queryData = new HashMap<>();

@Override
public ImmutableSortedSet<DocumentKey> getRemoteKeysForTarget(int targetId) {
return syncedKeys.get(targetId) != null
? syncedKeys.get(targetId)
: DocumentKey.emptyKeySet();
}

@androidx.annotation.Nullable
@Override
public QueryData getQueryDataForTarget(int targetId) {
return queryData.get(targetId);
}

/** Sets or replaces the local state for the provided query data. */
public void setSyncedKeys(QueryData queryData, ImmutableSortedSet<DocumentKey> keys) {
this.queryData.put(queryData.getTargetId(), queryData);
this.syncedKeys.put(queryData.getTargetId(), keys);
}
}

private static Map<String, Object> fromJsonString(String json) {
try {
ObjectMapper mapper = new ObjectMapper();
Expand Down