Skip to content

Commit 7671330

Browse files
schmidt-sebastianschmidt-sebastian
committed
Don't delete column
1 parent 07500fc commit 7671330

File tree

3 files changed

+77
-107
lines changed

3 files changed

+77
-107
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java

Lines changed: 34 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,12 @@
2323
import com.google.firebase.firestore.model.DocumentCollections;
2424
import com.google.firebase.firestore.model.DocumentKey;
2525
import com.google.firebase.firestore.model.MutableDocument;
26-
import com.google.firebase.firestore.model.ResourcePath;
2726
import com.google.firebase.firestore.model.SnapshotVersion;
2827
import com.google.firebase.firestore.util.BackgroundQueue;
2928
import com.google.firebase.firestore.util.Executors;
3029
import com.google.protobuf.InvalidProtocolBufferException;
3130
import com.google.protobuf.MessageLite;
3231
import java.util.ArrayList;
33-
import java.util.Collections;
3432
import java.util.HashMap;
3533
import java.util.List;
3634
import java.util.Map;
@@ -64,10 +62,10 @@ public void add(MutableDocument document, SnapshotVersion readTime) {
6462

6563
db.execute(
6664
"INSERT OR REPLACE INTO remote_documents "
67-
+ "(collection_path, document_id, read_time_seconds, read_time_nanos, contents) "
65+
+ "(path, parent_path, read_time_seconds, read_time_nanos, contents) "
6866
+ "VALUES (?, ?, ?, ?, ?)",
67+
EncodedPath.encode(documentKey.getPath()),
6968
EncodedPath.encode(documentKey.getCollectionPath()),
70-
documentKey.getPath().getLastSegment(),
7169
timestamp.getSeconds(),
7270
timestamp.getNanoseconds(),
7371
message.toByteArray());
@@ -77,64 +75,53 @@ public void add(MutableDocument document, SnapshotVersion readTime) {
7775

7876
@Override
7977
public void remove(DocumentKey documentKey) {
80-
db.execute(
81-
"DELETE FROM remote_documents WHERE collection_path = ? AND document_id = ?",
82-
EncodedPath.encode(documentKey.getCollectionPath()),
83-
documentKey.getDocumentId());
78+
String path = EncodedPath.encode(documentKey.getPath());
79+
80+
db.execute("DELETE FROM remote_documents WHERE path = ?", path);
8481
}
8582

8683
@Override
8784
public MutableDocument get(DocumentKey documentKey) {
85+
String path = EncodedPath.encode(documentKey.getPath());
86+
8887
MutableDocument document =
8988
db.query(
90-
"SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents "
91-
+ "WHERE collection_path = ? AND document_id = ?")
92-
.binding(
93-
EncodedPath.encode(documentKey.getCollectionPath()), documentKey.getDocumentId())
89+
"SELECT contents, read_time_seconds, read_time_nanos "
90+
+ "FROM remote_documents WHERE path = ?")
91+
.binding(path)
9492
.firstValue(row -> decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2)));
9593
return document != null ? document : MutableDocument.newInvalidDocument(documentKey);
9694
}
9795

9896
@Override
9997
public Map<DocumentKey, MutableDocument> getAll(Iterable<DocumentKey> documentKeys) {
10098
Map<DocumentKey, MutableDocument> results = new HashMap<>();
101-
102-
// We issue one query by collection, so first we have to sort the keys into collection buckets.
103-
Map<ResourcePath, List<Object>> collectionToDocumentIds = new HashMap<>();
99+
List<Object> bindVars = new ArrayList<>();
104100
for (DocumentKey key : documentKeys) {
105-
ResourcePath path = key.getPath();
106-
List<Object> documentIds = collectionToDocumentIds.get(path.popLast());
107-
if (documentIds == null) {
108-
documentIds = new ArrayList<>();
109-
collectionToDocumentIds.put(path.popLast(), documentIds);
110-
}
111-
documentIds.add(path.getLastSegment());
101+
bindVars.add(EncodedPath.encode(key.getPath()));
112102

113103
// Make sure each key has a corresponding entry, which is null in case the document is not
114104
// found.
115105
results.put(key, MutableDocument.newInvalidDocument(key));
116106
}
117107

118-
for (Map.Entry<ResourcePath, List<Object>> entry : collectionToDocumentIds.entrySet()) {
119-
SQLitePersistence.LongQuery longQuery =
120-
new SQLitePersistence.LongQuery(
121-
db,
122-
"SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents "
123-
+ "WHERE collection_path = ? AND document_id IN (",
124-
Collections.singletonList(EncodedPath.encode(entry.getKey())),
125-
entry.getValue(),
126-
")");
127-
128-
while (longQuery.hasMoreSubqueries()) {
129-
longQuery
130-
.performNextSubquery()
131-
.forEach(
132-
row -> {
133-
MutableDocument decoded =
134-
decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2));
135-
results.put(decoded.getKey(), decoded);
136-
});
137-
}
108+
SQLitePersistence.LongQuery longQuery =
109+
new SQLitePersistence.LongQuery(
110+
db,
111+
"SELECT contents, read_time_seconds, read_time_nanos FROM remote_documents "
112+
+ "WHERE path IN (",
113+
bindVars,
114+
") ORDER BY path");
115+
116+
while (longQuery.hasMoreSubqueries()) {
117+
longQuery
118+
.performNextSubquery()
119+
.forEach(
120+
row -> {
121+
MutableDocument decoded =
122+
decodeMaybeDocument(row.getBlob(0), row.getInt(1), row.getInt(2));
123+
results.put(decoded.getKey(), decoded);
124+
});
138125
}
139126

140127
return results;
@@ -147,7 +134,7 @@ public ImmutableSortedMap<DocumentKey, MutableDocument> getAllDocumentsMatchingQ
147134
!query.isCollectionGroupQuery(),
148135
"CollectionGroup queries should be handled in LocalDocumentsView");
149136

150-
String collectionPath = EncodedPath.encode(query.getPath());
137+
String parentPath = EncodedPath.encode(query.getPath());
151138
Timestamp readTime = sinceReadTime.getTimestamp();
152139

153140
BackgroundQueue backgroundQueue = new BackgroundQueue();
@@ -161,19 +148,19 @@ public ImmutableSortedMap<DocumentKey, MutableDocument> getAllDocumentsMatchingQ
161148
sqlQuery =
162149
db.query(
163150
"SELECT contents, read_time_seconds, read_time_nanos "
164-
+ "FROM remote_documents WHERE collection_path = ?")
165-
.binding(collectionPath);
151+
+ "FROM remote_documents WHERE parent_path = ?")
152+
.binding(parentPath);
166153
} else {
167154
// Execute an index-free query and filter by read time. This is safe since all document
168155
// changes to queries that have a lastLimboFreeSnapshotVersion (`sinceReadTime`) have a read
169156
// time set.
170157
sqlQuery =
171158
db.query(
172159
"SELECT contents, read_time_seconds, read_time_nanos "
173-
+ "FROM remote_documents WHERE collection_path = ? "
160+
+ "FROM remote_documents WHERE parent_path = ? "
174161
+ "AND (read_time_seconds > ? OR (read_time_seconds = ? AND read_time_nanos > ?))")
175162
.binding(
176-
collectionPath,
163+
parentPath,
177164
readTime.getSeconds(),
178165
readTime.getSeconds(),
179166
readTime.getNanoseconds());

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java

Lines changed: 17 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,8 @@ void runSchemaUpgrades(int fromVersion, int toVersion) {
174174
}
175175

176176
if (fromVersion < 13 && toVersion >= 13) {
177-
rewriteDocumentKeys();
177+
addParentPath();
178+
ensureParentPath();
178179
}
179180

180181
/*
@@ -408,6 +409,9 @@ private void createFieldIndex() {
408409
+ "directional_value BLOB, " // index values for equality and inequalities
409410
+ "document_name TEXT, "
410411
+ "PRIMARY KEY (index_id, uid, array_value, directional_value, document_name))");
412+
413+
db.execSQL(
414+
"CREATE INDEX read_time ON remote_documents(read_time_seconds, read_time_nanos)");
411415
});
412416
}
413417

@@ -440,6 +444,13 @@ private void addSequenceNumber() {
440444
}
441445
}
442446

447+
private void addParentPath() {
448+
if (!tableContainsColumn("remote_documents", "parent_path")) {
449+
db.execSQL("ALTER TABLE remote_documents ADD COLUMN parent_path TEXT");
450+
db.execSQL("CREATE INDEX parent_path_index ON remote_documents(parent_path)");
451+
}
452+
}
453+
443454
private boolean hasReadTime() {
444455
boolean hasReadTimeSeconds = tableContainsColumn("remote_documents", "read_time_seconds");
445456
boolean hasReadTimeNanos = tableContainsColumn("remote_documents", "read_time_nanos");
@@ -615,38 +626,14 @@ private void rewriteCanonicalIds() {
615626
});
616627
}
617628

618-
/**
619-
* Migrates the remote_documents table to contain a distinct column for the document's collection
620-
* path and its id.
621-
*/
622-
private void rewriteDocumentKeys() {
623-
// SQLite does not support dropping a primary key. To create a new primary key on
624-
// collection_path and document_id we need to create a new table :(
625-
db.execSQL("ALTER TABLE remote_documents RENAME TO tmp;");
626-
db.execSQL(
627-
"CREATE TABLE remote_documents ("
628-
+ "collection_path TEXT, "
629-
+ "document_id TEXT, "
630-
+ "read_time_nanos INTEGER, "
631-
+ "read_time_seconds INTEGER, "
632-
+ "contents BLOB, "
633-
+ "PRIMARY KEY (collection_path, document_id))");
634-
db.execSQL(
635-
"CREATE INDEX remote_documents_read_time ON remote_documents (read_time_nanos, read_time_seconds)");
636-
db.execSQL(
637-
"INSERT INTO remote_documents (collection_path, read_time_nanos, read_time_seconds, contents) "
638-
+ "SELECT path AS collection_path, read_time_nanos, read_time_seconds, contents FROM tmp");
639-
db.execSQL("DROP TABLE tmp;");
640-
641-
// Process each entry to split the document key into collection_path and document_id
629+
/** Fill the remote_document's parent path column. */
630+
private void ensureParentPath() {
642631
SQLitePersistence.Query documentsToMigrate =
643632
new SQLitePersistence.Query(
644-
db,
645-
"SELECT collection_path FROM remote_documents WHERE document_id IS NULL LIMIT ?")
633+
db, "SELECT path FROM remote_documents WHERE parent_path IS NULL LIMIT ?")
646634
.binding(MIGRATION_BATCH_SIZE);
647635
SQLiteStatement insertKey =
648-
db.compileStatement(
649-
"UPDATE remote_documents SET collection_path = ?, document_id = ? WHERE collection_path = ?");
636+
db.compileStatement("UPDATE remote_documents SET parent_path = ? WHERE path = ?");
650637

651638
boolean[] resultsRemaining = new boolean[1];
652639

@@ -662,8 +649,7 @@ private void rewriteDocumentKeys() {
662649

663650
insertKey.clearBindings();
664651
insertKey.bindString(1, EncodedPath.encode(decodedPath.popLast()));
665-
insertKey.bindString(2, decodedPath.getLastSegment());
666-
insertKey.bindString(3, encodedPath);
652+
insertKey.bindString(2, encodedPath);
667653
hardAssert(insertKey.executeUpdateDelete() != -1, "Failed to update document path");
668654
});
669655
} while (resultsRemaining[0]);

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java

Lines changed: 26 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import static com.google.firebase.firestore.local.EncodedPath.decodeResourcePath;
1818
import static com.google.firebase.firestore.local.EncodedPath.encode;
19+
import static com.google.firebase.firestore.local.SQLiteSchema.VERSION;
1920
import static com.google.firebase.firestore.testutil.TestUtil.filter;
2021
import static com.google.firebase.firestore.testutil.TestUtil.key;
2122
import static com.google.firebase.firestore.testutil.TestUtil.map;
@@ -144,7 +145,7 @@ public void migrationsDontDeleteTablesOrColumns() {
144145
// deletes an existing table or column, which would be very likely to break old versions of the
145146
// SDK relying on that table or column.
146147
Map<String, Set<String>> tables = new HashMap<>();
147-
for (int toVersion = 1; toVersion <= SQLiteSchema.VERSION; toVersion++) {
148+
for (int toVersion = 1; toVersion <= VERSION; toVersion++) {
148149
schema.runSchemaUpgrades(toVersion - 1, toVersion);
149150
Map<String, Set<String>> newTables = getCurrentSchema();
150151
assertNoRemovals(tables, newTables, toVersion);
@@ -154,10 +155,10 @@ public void migrationsDontDeleteTablesOrColumns() {
154155

155156
@Test
156157
public void canRecoverFromDowngrades() {
157-
for (int downgradeVersion = 0; downgradeVersion < SQLiteSchema.VERSION; downgradeVersion++) {
158+
for (int downgradeVersion = 0; downgradeVersion < VERSION; downgradeVersion++) {
158159
// Upgrade schema to current, then upgrade from `downgradeVersion` to current
159160
schema.runSchemaUpgrades();
160-
schema.runSchemaUpgrades(downgradeVersion, SQLiteSchema.VERSION);
161+
schema.runSchemaUpgrades(downgradeVersion, VERSION);
161162
}
162163
}
163164

@@ -437,6 +438,8 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() {
437438

438439
SQLiteRemoteDocumentCache remoteDocumentCache = createRemoteDocumentCache();
439440

441+
schema.runSchemaUpgrades(10, VERSION);
442+
440443
// Verify that queries with SnapshotVersion.NONE return all results, regardless of whether the
441444
// read time has been set.
442445
ImmutableSortedMap<DocumentKey, MutableDocument> results =
@@ -572,36 +575,32 @@ public void rewritesCanonicalIds() {
572575
}
573576

574577
@Test
575-
public void rewritesDocumentKeys() {
578+
public void addParentPaths() {
576579
schema.runSchemaUpgrades(0, 12);
577580

578-
ResourcePath path = path("coll/docA");
579-
db.execSQL(
580-
"INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)",
581-
new Object[] {EncodedPath.encode(path), 1, 2, new byte[] {3}});
581+
ResourcePath paths[] = new ResourcePath[] {path("collA/doc"), path("collB/doc")};
582+
583+
for (ResourcePath path : paths) {
584+
db.execSQL(
585+
"INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)",
586+
new Object[] {EncodedPath.encode(path)});
587+
}
582588

583589
schema.runSchemaUpgrades(12, 13);
584-
new SQLitePersistence.Query(
585-
db,
586-
"SELECT collection_path, document_id, read_time_seconds, read_time_nanos, contents FROM remote_documents")
590+
591+
int[] current = new int[] {0};
592+
new SQLitePersistence.Query(db, "SELECT parent_path FROM remote_documents ORDER BY path")
587593
.forEach(
588594
cursor -> {
589-
String encodedCollectionPath = cursor.getString(0);
590-
String documentId = cursor.getString(1);
591-
long readTimeSeconds = cursor.getLong(2);
592-
int readTimeNanos = cursor.getInt(3);
593-
byte[] contents = cursor.getBlob(4);
594-
595-
assertEquals(path("coll"), EncodedPath.decodeResourcePath(encodedCollectionPath));
596-
assertEquals("docA", documentId);
597-
assertEquals(1, readTimeSeconds);
598-
assertEquals(2, readTimeNanos);
599-
assertArrayEquals(new byte[] {3}, contents);
595+
ResourcePath parentPath = EncodedPath.decodeResourcePath(cursor.getString(0));
596+
assertEquals(paths[current[0]].popLast(), parentPath);
597+
++current[0];
600598
});
599+
assertEquals(2, current[0]);
601600
}
602601

603602
@Test
604-
public void usesMultipleBatchesToRewriteDocumentKeys() {
603+
public void usesMultipleBatchesToAddParentPaths() {
605604
schema.runSchemaUpgrades(0, 12);
606605

607606
for (int i = 0; i < SQLiteSchema.MIGRATION_BATCH_SIZE + 1; ++i) {
@@ -614,16 +613,14 @@ public void usesMultipleBatchesToRewriteDocumentKeys() {
614613
schema.runSchemaUpgrades(12, 13);
615614

616615
int[] current = new int[] {0};
617-
618-
new SQLitePersistence.Query(db, "SELECT document_id FROM remote_documents ORDER by document_id")
616+
new SQLitePersistence.Query(db, "SELECT parent_path FROM remote_documents ORDER by path")
619617
.forEach(
620618
cursor -> {
621-
String documentId = cursor.getString(0);
622-
assertEquals(String.format("doc%03d", current[0]), documentId);
619+
ResourcePath parentPath = EncodedPath.decodeResourcePath(cursor.getString(0));
620+
assertEquals(path("coll"), parentPath);
623621
++current[0];
624622
});
625-
626-
assertEquals(current[0], SQLiteSchema.MIGRATION_BATCH_SIZE + 1);
623+
assertEquals(SQLiteSchema.MIGRATION_BATCH_SIZE + 1, current[0]);
627624
}
628625

629626
@Test

0 commit comments

Comments
 (0)