Remove internal FieldValue representation

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,10 @@
-# Unreleased (21.4.0)
+# Unreleased 
+- [changed] Changed the in-memory representation of Firestore documents to
+  reduce memory allocations and improve performance. Calls to 
+  `DocumentSnapshot.getData()` and `DocumentSnapshot.toObject()` will see
+  the biggest improvement.
+
+# 21.4.0
 - [feature] Firestore previously required that every document read in a
   transaction must also be written. This requirement has been removed, and
   you can now read a document in a transaction without writing to it.

firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java

@@ -26,7 +26,7 @@
 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.value.ObjectValue;
+import com.google.firebase.firestore.model.ObjectValue;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;

firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/testutil/TestUtil.java

@@ -21,17 +21,17 @@
 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.ObjectValue;
 import com.google.firebase.firestore.model.ResourcePath;
 import com.google.firebase.firestore.model.SnapshotVersion;
-import com.google.firebase.firestore.model.value.FieldValue;
-import com.google.firebase.firestore.model.value.ObjectValue;
+import com.google.firestore.v1.Value;
 import java.util.Comparator;
 import java.util.Map;
 
 /** A set of utilities for tests */
 public class TestUtil {
 
-  public static FieldValue wrap(Object value) {
+  public static Value wrap(Object value) {
     DatabaseId databaseId = DatabaseId.forProject("project");
     UserDataReader dataReader = new UserDataReader(databaseId);
     // HACK: We use parseQueryValue() since it accepts scalars as well as arrays / objects, and
@@ -41,7 +41,7 @@ public static FieldValue wrap(Object value) {
 
   public static ObjectValue wrapObject(Map<String, Object> value) {
     // Cast is safe here because value passed in is a map
-    return (ObjectValue) wrap(value);
+    return new ObjectValue(wrap(value));
   }
 
   public static DocumentKey key(String key) {
@@ -64,21 +64,21 @@ public static SnapshotVersion version(long versionMicros) {
 
   public static Document doc(String key, long version, Map<String, Object> data) {
     return new Document(
-        key(key), version(version), Document.DocumentState.SYNCED, wrapObject(data));
+        key(key), version(version), wrapObject(data), Document.DocumentState.SYNCED);
   }
 
   public static Document doc(DocumentKey key, long version, Map<String, Object> data) {
-    return new Document(key, version(version), Document.DocumentState.SYNCED, wrapObject(data));
+    return new Document(key, version(version), wrapObject(data), Document.DocumentState.SYNCED);
   }
 
   public static Document doc(
       String key, long version, ObjectValue data, Document.DocumentState documentState) {
-    return new Document(key(key), version(version), documentState, data);
+    return new Document(key(key), version(version), data, documentState);
   }
 
   public static Document doc(
       String key, long version, Map<String, Object> data, Document.DocumentState documentState) {
-    return new Document(key(key), version(version), documentState, wrapObject(data));
+    return new Document(key(key), version(version), wrapObject(data), documentState);
   }
 
   public static DocumentSet docSet(Comparator<Document> comparator, Document... documents) {

firebase-firestore/ktx/src/test/kotlin/com/google/firebase/firestore/ktx/FirestoreTests.kt

@@ -22,8 +22,8 @@ import com.google.firebase.firestore.FieldPath
 import com.google.firebase.firestore.FirebaseFirestore
 import com.google.firebase.firestore.FirebaseFirestoreSettings
 import com.google.firebase.firestore.TestUtil
-import com.google.firebase.firestore.model.value.IntegerValue
-import com.google.firebase.firestore.model.value.ObjectValue
+import com.google.firebase.firestore.model.ObjectValue
+import com.google.firebase.firestore.testutil.TestUtil.wrap
 import com.google.firebase.ktx.Firebase
 import com.google.firebase.ktx.app
 import com.google.firebase.ktx.initialize
@@ -171,7 +171,7 @@ class QuerySnapshotTests {
         val qs = TestUtil.querySnapshot(
                 "rooms",
                 mapOf(),
-                mapOf("id" to ObjectValue.fromMap(mapOf("a" to IntegerValue.valueOf(1), "b" to IntegerValue.valueOf(2)))),
+                mapOf("id" to ObjectValue.fromMap(mapOf("a" to wrap(1), "b" to wrap(2)))),
                 false,
                 false)
 

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java

@@ -169,6 +169,26 @@ public void testCanMergeEmptyObject() {
     listenerRegistration.remove();
   }
 
+  @Test
+  public void testUpdateWithEmptyObjectReplacesAllFields() {
+    DocumentReference documentReference = testDocument();
+    documentReference.set(map("a", "a"));
+
+    waitFor(documentReference.update("a", Collections.emptyMap()));
+    DocumentSnapshot snapshot = waitFor(documentReference.get());
+    assertEquals(map("a", Collections.emptyMap()), snapshot.getData());
+  }
+
+  @Test
+  public void testMergeWithEmptyObjectReplacesAllFields() {
+    DocumentReference documentReference = testDocument();
+    documentReference.set(map("a", "a"));
+
+    waitFor(documentReference.set(map("a", Collections.emptyMap()), SetOptions.merge()));
+    DocumentSnapshot snapshot = waitFor(documentReference.get());
+    assertEquals(map("a", Collections.emptyMap()), snapshot.getData());
+  }
+
   @Test
   public void testCanDeleteFieldUsingMerge() {
     DocumentReference documentReference = testCollection("rooms").document("eros");

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/WriteBatchTest.java

@@ -222,7 +222,7 @@ public void testWriteTheSameServerTimestampAcrossWrites() {
     assertTrue(localSnap.getMetadata().hasPendingWrites());
     assertEquals(asList(map("when", null), map("when", null)), querySnapshotToValues(localSnap));
 
-    QuerySnapshot serverSnap = accumulator.await();
+    QuerySnapshot serverSnap = accumulator.awaitRemoteEvent();
     assertFalse(serverSnap.getMetadata().hasPendingWrites());
     assertEquals(2, serverSnap.size());
     Timestamp when = serverSnap.getDocuments().get(0).getTimestamp("when");

firebase-firestore/src/main/java/com/google/firebase/firestore/Blob.java

@@ -82,18 +82,6 @@ public int hashCode() {
 
   @Override
   public int compareTo(@NonNull Blob other) {
-    int size = Math.min(bytes.size(), other.bytes.size());
-    for (int i = 0; i < size; i++) {
-      // Make sure the bytes are unsigned
-      int thisByte = bytes.byteAt(i) & 0xff;
-      int otherByte = other.bytes.byteAt(i) & 0xff;
-      if (thisByte < otherByte) {
-        return -1;
-      } else if (thisByte > otherByte) {
-        return 1;
-      }
-      // Byte values are equal, continue with comparison
-    }
-    return Util.compareIntegers(bytes.size(), other.bytes.size());
+    return Util.compareByteStrings(bytes, other.bytes);
   }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java

@@ -21,8 +21,8 @@
 import com.google.firebase.Timestamp;
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
-import com.google.firebase.firestore.model.value.FieldValue;
 import com.google.firebase.firestore.util.CustomClassMapper;
+import com.google.firestore.v1.Value;
 import java.util.Date;
 import java.util.Map;
 
@@ -149,7 +149,7 @@ public Map<String, Object> getData(@NonNull ServerTimestampBehavior serverTimest
             firestore,
             firestore.getFirestoreSettings().areTimestampsInSnapshotsEnabled(),
             serverTimestampBehavior);
-    return doc == null ? null : userDataWriter.convertObject(doc.getData());
+    return doc == null ? null : userDataWriter.convertObject(doc.getData().getFieldsMap());
   }
 
   /**
@@ -529,7 +529,7 @@ private Object getInternal(
       @NonNull ServerTimestampBehavior serverTimestampBehavior,
       boolean timestampsInSnapshots) {
     if (doc != null) {
-      FieldValue val = doc.getField(fieldPath);
+      Value val = doc.getField(fieldPath);
       if (val != null) {
         UserDataWriter userDataWriter =
             new UserDataWriter(firestore, timestampsInSnapshots, serverTimestampBehavior);

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

@@ -39,12 +39,12 @@
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.ResourcePath;
-import com.google.firebase.firestore.model.value.ArrayValue;
-import com.google.firebase.firestore.model.value.FieldValue;
-import com.google.firebase.firestore.model.value.ReferenceValue;
-import com.google.firebase.firestore.model.value.ServerTimestampValue;
+import com.google.firebase.firestore.model.ServerTimestamps;
+import com.google.firebase.firestore.model.Values;
 import com.google.firebase.firestore.util.Executors;
 import com.google.firebase.firestore.util.Util;
+import com.google.firestore.v1.ArrayValue;
+import com.google.firestore.v1.Value;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
@@ -326,7 +326,7 @@ public Query whereIn(@NonNull FieldPath fieldPath, @NonNull List<? extends Objec
   private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object value) {
     checkNotNull(fieldPath, "Provided field path must not be null.");
     checkNotNull(op, "Provided op must not be null.");
-    FieldValue fieldValue;
+    Value fieldValue;
     com.google.firebase.firestore.model.FieldPath internalPath = fieldPath.getInternalPath();
     if (internalPath.isKeyField()) {
       if (op == Operator.ARRAY_CONTAINS || op == Operator.ARRAY_CONTAINS_ANY) {
@@ -336,11 +336,11 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu
                 + "' queries on FieldPath.documentId().");
       } else if (op == Operator.IN) {
         validateDisjunctiveFilterElements(value, op);
-        List<FieldValue> referenceList = new ArrayList<>();
+        ArrayValue.Builder referenceList = ArrayValue.newBuilder();
         for (Object arrayValue : (List) value) {
-          referenceList.add(parseDocumentIdValue(arrayValue));
+          referenceList.addValues(parseDocumentIdValue(arrayValue));
         }
-        fieldValue = ArrayValue.fromList(referenceList);
+        fieldValue = Value.newBuilder().setArrayValue(referenceList).build();
       } else {
         fieldValue = parseDocumentIdValue(value);
       }
@@ -367,7 +367,7 @@ private void validateOrderByField(com.google.firebase.firestore.model.FieldPath
    * Parses the given documentIdValue into a ReferenceValue, throwing appropriate errors if the
    * value is anything other than a DocumentReference or String, or if the string is malformed.
    */
-  private ReferenceValue parseDocumentIdValue(Object documentIdValue) {
+  private Value parseDocumentIdValue(Object documentIdValue) {
     if (documentIdValue instanceof String) {
       String documentId = (String) documentIdValue;
       if (documentId.isEmpty()) {
@@ -392,11 +392,10 @@ private ReferenceValue parseDocumentIdValue(Object documentIdValue) {
                 + path.length()
                 + ").");
       }
-      return ReferenceValue.valueOf(
-          this.getFirestore().getDatabaseId(), DocumentKey.fromPath(path));
+      return Values.refValue(this.getFirestore().getDatabaseId(), DocumentKey.fromPath(path));
     } else if (documentIdValue instanceof DocumentReference) {
       DocumentReference ref = (DocumentReference) documentIdValue;
-      return ReferenceValue.valueOf(this.getFirestore().getDatabaseId(), ref.getKey());
+      return Values.refValue(this.getFirestore().getDatabaseId(), ref.getKey());
     } else {
       throw new IllegalArgumentException(
           "Invalid query. When querying with FieldPath.documentId() you must provide a valid "
@@ -740,7 +739,7 @@ private Bound boundFromDocumentSnapshot(
               + "().");
     }
     Document document = snapshot.getDocument();
-    List<FieldValue> components = new ArrayList<>();
+    List<Value> components = new ArrayList<>();
 
     // Because people expect to continue/end a query at the exact document provided, we need to
     // use the implicit sort order rather than the explicit sort order, because it's guaranteed to
@@ -749,10 +748,10 @@ private Bound boundFromDocumentSnapshot(
     // orders), multiple documents could match the position, yielding duplicate results.
     for (OrderBy orderBy : query.getOrderBy()) {
       if (orderBy.getField().equals(com.google.firebase.firestore.model.FieldPath.KEY_PATH)) {
-        components.add(ReferenceValue.valueOf(firestore.getDatabaseId(), document.getKey()));
+        components.add(Values.refValue(firestore.getDatabaseId(), document.getKey()));
       } else {
-        FieldValue value = document.getField(orderBy.getField());
-        if (value instanceof ServerTimestampValue) {
+        Value value = document.getField(orderBy.getField());
+        if (ServerTimestamps.isServerTimestamp(value)) {
           throw new IllegalArgumentException(
               "Invalid query. You are trying to start or end a query using a document for which "
                   + "the field '"
@@ -785,7 +784,7 @@ private Bound boundFromFields(String methodName, Object[] values, boolean before
               + "than or equal to the number of orderBy() clauses.");
     }
 
-    List<FieldValue> components = new ArrayList<>();
+    List<Value> components = new ArrayList<>();
     for (int i = 0; i < values.length; i++) {
       Object rawValue = values[i];
       OrderBy orderBy = explicitOrderBy.get(i);
@@ -819,9 +818,9 @@ private Bound boundFromFields(String methodName, Object[] values, boolean before
                   + "' is not because it contains an odd number of segments.");
         }
         DocumentKey key = DocumentKey.fromPath(path);
-        components.add(ReferenceValue.valueOf(firestore.getDatabaseId(), key));
+        components.add(Values.refValue(firestore.getDatabaseId(), key));
       } else {
-        FieldValue wrapped = firestore.getUserDataReader().parseQueryValue(rawValue);
+        Value wrapped = firestore.getUserDataReader().parseQueryValue(rawValue);
         components.add(wrapped);
       }
     }