From 86ba758fbfce7cb4a35259c647ef30526d359c0e Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Tue, 1 Aug 2023 15:21:20 -0400 Subject: [PATCH 01/13] refactor the orderBy to normalizedOrderBy --- firebase-firestore/CHANGELOG.md | 1 + .../firebase/firestore/AggregateField.java | 3 - .../firebase/firestore/AggregateQuery.java | 4 - .../firestore/AggregateQuerySnapshot.java | 13 -- .../com/google/firebase/firestore/Query.java | 6 +- .../google/firebase/firestore/core/Query.java | 122 ++++++++++-------- .../firebase/firestore/core/QueryTest.java | 32 +++-- 7 files changed, 94 insertions(+), 87 deletions(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 5f944dd015c..c8114a8d47c 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased * [fixed] Implement equals method on Filter class. [#5210](//github.com/firebase/firebase-android-sdk/issues/5210) +* [feature] Expose Sum/Average aggregate query support in API. [#5217](//github.com/firebase/firebase-android-sdk/pull/5217) # 24.7.0 * [feature] Expose MultiDb support in API. [#4015](//github.com/firebase/firebase-android-sdk/issues/4015) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java index d461bc42262..22a013c33ae 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java @@ -19,9 +19,6 @@ import androidx.annotation.RestrictTo; import java.util.Objects; -// TODO(sumavg): Remove the `hide` and scope annotations. -/** @hide */ -@RestrictTo(RestrictTo.Scope.LIBRARY) public abstract class AggregateField { @Nullable private final FieldPath fieldPath; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java index f465bf92dcc..0cb048d3259 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java @@ -15,7 +15,6 @@ package com.google.firebase.firestore; import androidx.annotation.NonNull; -import androidx.annotation.RestrictTo; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.firestore.util.Executors; @@ -48,9 +47,6 @@ public Query getQuery() { } /** Returns the AggregateFields included inside this object. */ - // TODO(sumavg): Remove the `hide` and scope annotations. - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) @NonNull public List getAggregateFields() { return aggregateFieldList; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuerySnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuerySnapshot.java index effcf21d980..ddb3247bd7c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuerySnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuerySnapshot.java @@ -73,9 +73,6 @@ public long getCount() { * @param aggregateField The aggregation for which the value is requested. * @return The result of the given aggregation. */ - // TODO(sumavg): Remove the `hide` and scope annotations. - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) @Nullable public Object get(@Nonnull AggregateField aggregateField) { return getInternal(aggregateField); @@ -87,8 +84,6 @@ public Object get(@Nonnull AggregateField aggregateField) { * @param countAggregateField The count aggregation for which the value is requested. * @return The result of the given count aggregation. */ - // TODO(sumavg): Remove the `hide` and scope annotations. - /** @hide */ @RestrictTo(RestrictTo.Scope.LIBRARY) public long get(@Nonnull AggregateField.CountAggregateField countAggregateField) { Long value = getLong(countAggregateField); @@ -108,8 +103,6 @@ public long get(@Nonnull AggregateField.CountAggregateField countAggregateField) * @param averageAggregateField The average aggregation for which the value is requested. * @return The result of the given average aggregation. */ - // TODO(sumavg): Remove the `hide` and scope annotations. - /** @hide */ @RestrictTo(RestrictTo.Scope.LIBRARY) @Nullable public Double get(@Nonnull AggregateField.AverageAggregateField averageAggregateField) { @@ -125,9 +118,6 @@ public Double get(@Nonnull AggregateField.AverageAggregateField averageAggregate * @param aggregateField The aggregation for which the value is requested. * @return The result of the given average aggregation as a double. */ - // TODO(sumavg): Remove the `hide` and scope annotations. - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) @Nullable public Double getDouble(@Nonnull AggregateField aggregateField) { Number val = getTypedValue(aggregateField, Number.class); @@ -142,9 +132,6 @@ public Double getDouble(@Nonnull AggregateField aggregateField) { * @param aggregateField The aggregation for which the value is requested. * @return The result of the given average aggregation as a long. */ - // TODO(sumavg): Remove the `hide` and scope annotations. - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) @Nullable public Long getLong(@Nonnull AggregateField aggregateField) { Number val = getTypedValue(aggregateField, Number.class); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 1685406dd9b..b6ef64c7370 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -22,7 +22,6 @@ import android.app.Activity; import androidx.annotation.NonNull; import androidx.annotation.Nullable; -import androidx.annotation.RestrictTo; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; @@ -899,7 +898,7 @@ private Bound boundFromDocumentSnapshot( // contain the document key. That way the position becomes unambiguous and the query // continues/ends exactly at the provided document. Without the key (by using the explicit sort // orders), multiple documents could match the position, yielding duplicate results. - for (OrderBy orderBy : query.getOrderBy()) { + for (OrderBy orderBy : query.getNormalizedOrderBy()) { if (orderBy.getField().equals(com.google.firebase.firestore.model.FieldPath.KEY_PATH)) { components.add(Values.refValue(firestore.getDatabaseId(), document.getKey())); } else { @@ -1244,9 +1243,6 @@ public AggregateQuery count() { * @return The {@code AggregateQuery} that performs aggregations on the documents in the result * set of this query. */ - // TODO(sumavg): Remove the `hide` and scope annotations. - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) @NonNull public AggregateQuery aggregate( @NonNull AggregateField aggregateField, @NonNull AggregateField... aggregateFields) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index b51b6058727..48afdbac2c1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -57,11 +57,17 @@ public static Query atPath(ResourcePath path) { private final List explicitSortOrder; - private List memoizedOrderBy; + private List memoizedNormalizedOrderBys; - // The corresponding Target of this Query instance. + /** Returns a `Target` instance this query will be mapped to in backend and local store. */ private @Nullable Target memoizedTarget; + /** + * Returns a `Target` instance this query will be mapped to in backend and local store, for use + * within an aggregate query. + */ + private @Nullable Target memoizedAggregateTarget; + private final List filters; private final ResourcePath path; @@ -333,8 +339,8 @@ public List getExplicitOrderBy() { *

The returned list is unmodifiable, to prevent ConcurrentModificationExceptions, if one * thread is iterating the list and one thread is modifying the list. */ - public synchronized List getOrderBy() { - if (memoizedOrderBy == null) { + public synchronized List getNormalizedOrderBy() { + if (memoizedNormalizedOrderBys == null) { FieldPath inequalityField = inequalityField(); FieldPath firstOrderByField = getFirstOrderByField(); if (inequalityField != null && firstOrderByField == null) { @@ -342,9 +348,9 @@ public synchronized List getOrderBy() { // it to be a valid query. Note that the default inequality field and key ordering is // ascending. if (inequalityField.isKeyField()) { - this.memoizedOrderBy = Collections.singletonList(KEY_ORDERING_ASC); + this.memoizedNormalizedOrderBys = Collections.singletonList(KEY_ORDERING_ASC); } else { - memoizedOrderBy = + memoizedNormalizedOrderBys = Collections.unmodifiableList( Arrays.asList( OrderBy.getInstance(Direction.ASCENDING, inequalityField), KEY_ORDERING_ASC)); @@ -367,10 +373,10 @@ public synchronized List getOrderBy() { : Direction.ASCENDING; res.add(lastDirection.equals(Direction.ASCENDING) ? KEY_ORDERING_ASC : KEY_ORDERING_DESC); } - memoizedOrderBy = Collections.unmodifiableList(res); + memoizedNormalizedOrderBys = Collections.unmodifiableList(res); } } - return memoizedOrderBy; + return memoizedNormalizedOrderBys; } private boolean matchesPathAndCollectionGroup(Document doc) { @@ -403,7 +409,7 @@ private boolean matchesOrderBy(Document doc) { // to the inequality, and is evaluated as "a > 1 orderBy a || b==1 orderBy a". // A document with content of {b:1} matches the filters, but does not match the orderBy because // it's missing the field 'a'. - for (OrderBy order : getOrderBy()) { + for (OrderBy order : getNormalizedOrderBy()) { // order by key always matches if (!order.getField().equals(FieldPath.KEY_PATH) && (doc.getField(order.field) == null)) { return false; @@ -414,10 +420,10 @@ private boolean matchesOrderBy(Document doc) { /** Makes sure a document is within the bounds, if provided. */ private boolean matchesBounds(Document doc) { - if (startAt != null && !startAt.sortsBeforeDocument(getOrderBy(), doc)) { + if (startAt != null && !startAt.sortsBeforeDocument(getNormalizedOrderBy(), doc)) { return false; } - if (endAt != null && !endAt.sortsAfterDocument(getOrderBy(), doc)) { + if (endAt != null && !endAt.sortsAfterDocument(getNormalizedOrderBy(), doc)) { return false; } return true; @@ -434,7 +440,7 @@ && matchesFilters(doc) /** Returns a comparator that will sort documents according to this Query's sort order. */ public Comparator comparator() { - return new QueryComparator(getOrderBy()); + return new QueryComparator(getNormalizedOrderBy()); } private static class QueryComparator implements Comparator { @@ -470,50 +476,62 @@ public int compare(Document doc1, Document doc2) { */ public synchronized Target toTarget() { if (this.memoizedTarget == null) { - if (this.limitType == LimitType.LIMIT_TO_FIRST) { - this.memoizedTarget = - new Target( - this.getPath(), - this.getCollectionGroup(), - this.getFilters(), - this.getOrderBy(), - this.limit, - this.getStartAt(), - this.getEndAt()); - } else { - // Flip the orderBy directions since we want the last results - ArrayList newOrderBy = new ArrayList<>(); - for (OrderBy orderBy : this.getOrderBy()) { - Direction dir = - orderBy.getDirection() == Direction.DESCENDING - ? Direction.ASCENDING - : Direction.DESCENDING; - newOrderBy.add(OrderBy.getInstance(dir, orderBy.getField())); - } + memoizedTarget = toTarget(getNormalizedOrderBy()); + } + return this.memoizedTarget; + } - // We need to swap the cursors to match the now-flipped query ordering. - Bound newStartAt = - this.endAt != null - ? new Bound(this.endAt.getPosition(), this.endAt.isInclusive()) - : null; - Bound newEndAt = - this.startAt != null - ? new Bound(this.startAt.getPosition(), this.startAt.isInclusive()) - : null; - - this.memoizedTarget = - new Target( - this.getPath(), - this.getCollectionGroup(), - this.getFilters(), - newOrderBy, - this.limit, - newStartAt, - newEndAt); + private synchronized Target toTarget(List orderBys) { + if (this.limitType == LimitType.LIMIT_TO_FIRST) { + return new Target( + this.getPath(), + this.getCollectionGroup(), + this.getFilters(), + orderBys, + this.limit, + this.getStartAt(), + this.getEndAt()); + } else { + // Flip the orderBy directions since we want the last results + ArrayList newOrderBy = new ArrayList<>(); + for (OrderBy orderBy : orderBys) { + Direction dir = + orderBy.getDirection() == Direction.DESCENDING + ? Direction.ASCENDING + : Direction.DESCENDING; + newOrderBy.add(OrderBy.getInstance(dir, orderBy.getField())); } + + // We need to swap the cursors to match the now-flipped query ordering. + Bound newStartAt = + this.endAt != null ? new Bound(this.endAt.getPosition(), this.endAt.isInclusive()) : null; + Bound newEndAt = + this.startAt != null + ? new Bound(this.startAt.getPosition(), this.startAt.isInclusive()) + : null; + + return new Target( + this.getPath(), + this.getCollectionGroup(), + this.getFilters(), + newOrderBy, + this.limit, + newStartAt, + newEndAt); } + } - return this.memoizedTarget; + /** + * This method is marked as synchronized because it modifies the internal state in some cases. + * + * @return A {@code Target} instance this query will be mapped to in backend and local store, for + * use within an aggregate query. + */ + private synchronized Target toAggregateTarget() { + if (this.memoizedAggregateTarget == null) { + memoizedAggregateTarget = toTarget(explicitSortOrder); + } + return this.memoizedAggregateTarget; } /** diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index dbfdf07bafd..5599d2d27c2 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -582,37 +582,49 @@ public void testHashCode() { public void testImplicitOrderBy() { Query baseQuery = Query.atPath(path("foo")); // Default is ascending - assertEquals(asList(orderBy(KEY_FIELD_NAME, "asc")), baseQuery.getOrderBy()); + assertEquals(asList(orderBy(KEY_FIELD_NAME, "asc")), baseQuery.getNormalizedOrderBy()); // Explicit key ordering is respected assertEquals( asList(orderBy(KEY_FIELD_NAME, "asc")), - baseQuery.orderBy(orderBy(KEY_FIELD_NAME, "asc")).getOrderBy()); + baseQuery.orderBy(orderBy(KEY_FIELD_NAME, "asc")).getNormalizedOrderBy()); assertEquals( asList(orderBy(KEY_FIELD_NAME, "desc")), - baseQuery.orderBy(orderBy(KEY_FIELD_NAME, "desc")).getOrderBy()); + baseQuery.orderBy(orderBy(KEY_FIELD_NAME, "desc")).getNormalizedOrderBy()); assertEquals( asList(orderBy("foo"), orderBy(KEY_FIELD_NAME, "asc")), - baseQuery.orderBy(orderBy("foo")).orderBy(orderBy(KEY_FIELD_NAME, "asc")).getOrderBy()); + baseQuery + .orderBy(orderBy("foo")) + .orderBy(orderBy(KEY_FIELD_NAME, "asc")) + .getNormalizedOrderBy()); assertEquals( asList(orderBy("foo"), orderBy(KEY_FIELD_NAME, "desc")), - baseQuery.orderBy(orderBy("foo")).orderBy(orderBy(KEY_FIELD_NAME, "desc")).getOrderBy()); + baseQuery + .orderBy(orderBy("foo")) + .orderBy(orderBy(KEY_FIELD_NAME, "desc")) + .getNormalizedOrderBy()); // Inequality filters add order bys assertEquals( asList(orderBy("foo"), orderBy(KEY_FIELD_NAME, "asc")), - baseQuery.filter(filter("foo", "<", 5)).getOrderBy()); + baseQuery.filter(filter("foo", "<", 5)).getNormalizedOrderBy()); // Descending order by applies to implicit key ordering assertEquals( asList(orderBy("foo", "desc"), orderBy(KEY_FIELD_NAME, "desc")), - baseQuery.orderBy(orderBy("foo", "desc")).getOrderBy()); + baseQuery.orderBy(orderBy("foo", "desc")).getNormalizedOrderBy()); assertEquals( asList(orderBy("foo", "asc"), orderBy("bar", "desc"), orderBy(KEY_FIELD_NAME, "desc")), - baseQuery.orderBy(orderBy("foo", "asc")).orderBy(orderBy("bar", "desc")).getOrderBy()); + baseQuery + .orderBy(orderBy("foo", "asc")) + .orderBy(orderBy("bar", "desc")) + .getNormalizedOrderBy()); assertEquals( asList(orderBy("foo", "desc"), orderBy("bar", "asc"), orderBy(KEY_FIELD_NAME, "asc")), - baseQuery.orderBy(orderBy("foo", "desc")).orderBy(orderBy("bar", "asc")).getOrderBy()); + baseQuery + .orderBy(orderBy("foo", "desc")) + .orderBy(orderBy("bar", "asc")) + .getNormalizedOrderBy()); } @Test @@ -891,7 +903,7 @@ public void testGetOrderByReturnsUnmodifiableList() { .orderBy(orderBy("e")) .orderBy(orderBy("f")); - List orderByList = query.getOrderBy(); + List orderByList = query.getNormalizedOrderBy(); assertThrows(UnsupportedOperationException.class, () -> orderByList.add(orderBy("g"))); } From d7f2e654c9736872044e38f76720c4d008d88df0 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Tue, 1 Aug 2023 15:58:59 -0400 Subject: [PATCH 02/13] Add test for getting aggregate target --- .../google/firebase/firestore/core/Query.java | 2 +- .../firebase/firestore/core/QueryTest.java | 54 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 48afdbac2c1..654a1255d58 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -527,7 +527,7 @@ private synchronized Target toTarget(List orderBys) { * @return A {@code Target} instance this query will be mapped to in backend and local store, for * use within an aggregate query. */ - private synchronized Target toAggregateTarget() { + public synchronized Target toAggregateTarget() { if (this.memoizedAggregateTarget == null) { memoizedAggregateTarget = toTarget(explicitSortOrder); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index 5599d2d27c2..34cac4305dd 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -908,6 +908,60 @@ public void testGetOrderByReturnsUnmodifiableList() { assertThrows(UnsupportedOperationException.class, () -> orderByList.add(orderBy("g"))); } + @Test + public void testOrderByForAggregateAndNonAggregate() { + Query col = query("collection"); + + // Build two identical queries + Query query1 = col.filter(filter("foo", ">", 1)); + Query query2 = col.filter(filter("foo", ">", 1)); + + // Compute an aggregate and non-aggregate target from the queries + Target aggregateTarget = query1.toAggregateTarget(); + Target target = query2.toTarget(); + + assertEquals(aggregateTarget.getOrderBy().size(), 0); + + assertEquals(target.getOrderBy().size(), 2); + assertEquals(target.getOrderBy().get(0).getDirection(), OrderBy.Direction.ASCENDING); + assertEquals(target.getOrderBy().get(0).getField().toString(), "foo"); + assertEquals(target.getOrderBy().get(1).getDirection(), OrderBy.Direction.ASCENDING); + assertEquals(target.getOrderBy().get(1).getField().toString(), "__name__"); + } + + @Test + public void testGeneratedOrderBysNotAffectedByPreviouslyMemoizedTargets() { + Query col = query("collection"); + + // Build two identical queries + Query query1 = col.filter(filter("foo", ">", 1)); + Query query2 = col.filter(filter("foo", ">", 1)); + + // query1 - first to aggregate target, then to non-aggregate target + Target aggregateTarget1 = query1.toAggregateTarget(); + Target target1 = query1.toTarget(); + + // query2 - first to non-aggregate target, then to aggregate target + Target target2 = query2.toTarget(); + Target aggregateTarget2 = query2.toAggregateTarget(); + + assertEquals(aggregateTarget1.getOrderBy().size(), 0); + + assertEquals(aggregateTarget2.getOrderBy().size(), 0); + + assertEquals(target1.getOrderBy().size(), 2); + assertEquals(target1.getOrderBy().get(0).getDirection(), OrderBy.Direction.ASCENDING); + assertEquals(target1.getOrderBy().get(0).getField().toString(), "foo"); + assertEquals(target1.getOrderBy().get(1).getDirection(), OrderBy.Direction.ASCENDING); + assertEquals(target1.getOrderBy().get(1).getField().toString(), "__name__"); + + assertEquals(target2.getOrderBy().size(), 2); + assertEquals(target2.getOrderBy().get(0).getDirection(), OrderBy.Direction.ASCENDING); + assertEquals(target2.getOrderBy().get(0).getField().toString(), "foo"); + assertEquals(target2.getOrderBy().get(1).getDirection(), OrderBy.Direction.ASCENDING); + assertEquals(target2.getOrderBy().get(1).getField().toString(), "__name__"); + } + private void assertQueryMatches( Query query, List matching, List nonMatching) { for (MutableDocument doc : matching) { From 23b02296c1e1b42a9e10e2e0cac1a6c45c231219 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Tue, 1 Aug 2023 16:01:08 -0400 Subject: [PATCH 03/13] change comments --- .../src/main/java/com/google/firebase/firestore/core/Query.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 654a1255d58..7ca0309a70f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -403,7 +403,7 @@ private boolean matchesFilters(Document doc) { /** A document must have a value for every ordering clause in order to show up in the results. */ private boolean matchesOrderBy(Document doc) { - // We must use `getOrderBy()` to get the list of all orderBys (both implicit and explicit). + // We must use `getNormalizedOrderBy()` to get the list of all orderBys (both implicit and explicit). // Note that for OR queries, orderBy applies to all disjunction terms and implicit orderBys must // be taken into account. For example, the query "a > 1 || b==1" has an implicit "orderBy a" due // to the inequality, and is evaluated as "a > 1 orderBy a || b==1 orderBy a". From 495020b221d977bd365c88bcf457af95cef3ac48 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Tue, 1 Aug 2023 18:51:40 -0400 Subject: [PATCH 04/13] Change tests --- .../firebase/firestore/AggregationTest.java | 17 ++++++++--------- .../google/firebase/firestore/core/Query.java | 3 ++- .../firebase/firestore/remote/Datastore.java | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java index cf360b8b3e3..8cd0f6be3b1 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java @@ -575,7 +575,11 @@ public void testThrowsAnErrorWhenGettingTheResultOfAnUnrequestedAggregation() { CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = - waitFor(collection.aggregate(sum("pages")).get(AggregateSource.SERVER)); + waitFor( + collection + .whereGreaterThan("pages", 200) + .aggregate(sum("pages")) + .get(AggregateSource.SERVER)); Exception exception = null; try { @@ -860,17 +864,13 @@ public void testPerformsSumThatIsValidButCouldOverflowDuringAggregation() { "Skip this test if running against production because sum/avg is only support " + "in emulator currently.", isRunningAgainstEmulator()); - + // Sum of rating would be 0, but if the accumulation overflow, we expect infinity Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", Double.MAX_VALUE), "b", map("author", "authorB", "title", "titleB", "rating", Double.MAX_VALUE), "c", map("author", "authorC", "title", "titleC", "rating", -Double.MAX_VALUE), - "d", map("author", "authorD", "title", "titleD", "rating", -Double.MAX_VALUE), - "e", map("author", "authorE", "title", "titleE", "rating", Double.MAX_VALUE), - "f", map("author", "authorF", "title", "titleF", "rating", -Double.MAX_VALUE), - "g", map("author", "authorG", "title", "titleG", "rating", -Double.MAX_VALUE), - "h", map("author", "authorH", "title", "titleH", "rating", Double.MAX_VALUE)); + "d", map("author", "authorD", "title", "titleD", "rating", -Double.MAX_VALUE)); CollectionReference collection = testCollectionWithDocs(testDocs); AggregateQuerySnapshot snapshot = @@ -878,7 +878,7 @@ public void testPerformsSumThatIsValidButCouldOverflowDuringAggregation() { Object sum = snapshot.get(sum("rating")); assertTrue(sum instanceof Long); - assertEquals(sum, 0L); + assertTrue(sum.equals(0L) || sum.equals(Long.MAX_VALUE) || sum.equals(Long.MIN_VALUE)); } @Test @@ -1240,7 +1240,6 @@ public void allowsAliasesLongerThan1500Bytes() { // 1500 bytes, the alias will be longer than 1500, which is the limit for aliases. This is to // make sure the client // can handle this corner case correctly. - StringBuilder builder = new StringBuilder(1500); for (int i = 0; i < 1500; i++) { builder.append("a"); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 7ca0309a70f..63fd0496645 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -403,7 +403,8 @@ private boolean matchesFilters(Document doc) { /** A document must have a value for every ordering clause in order to show up in the results. */ private boolean matchesOrderBy(Document doc) { - // We must use `getNormalizedOrderBy()` to get the list of all orderBys (both implicit and explicit). + // We must use `getNormalizedOrderBy()` to get the list of all orderBys (both implicit and + // explicit). // Note that for OR queries, orderBy applies to all disjunction terms and implicit orderBys must // be taken into account. For example, the query "a > 1 || b==1" has an implicit "orderBy a" due // to the inequality, and is evaluated as "a > 1 orderBy a || b==1 orderBy a". diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java index c3de588b354..8fe62677439 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java @@ -225,7 +225,7 @@ public void onClose(Status status) { public Task> runAggregateQuery( Query query, List aggregateFields) { com.google.firestore.v1.Target.QueryTarget encodedQueryTarget = - serializer.encodeQueryTarget(query.toTarget()); + serializer.encodeQueryTarget(query.toAggregateTarget()); HashMap aliasMap = new HashMap<>(); StructuredAggregationQuery structuredAggregationQuery = serializer.encodeStructuredAggregationQuery(encodedQueryTarget, aggregateFields, aliasMap); From 8e53ab4c2a9fa453b1d8e78d277778c87aeffed5 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Wed, 2 Aug 2023 11:30:28 -0400 Subject: [PATCH 05/13] generate api.txt --- firebase-firestore/api.txt | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/firebase-firestore/api.txt b/firebase-firestore/api.txt index c61fc547913..9def04499f3 100644 --- a/firebase-firestore/api.txt +++ b/firebase-firestore/api.txt @@ -19,13 +19,39 @@ package com.google.firebase { package com.google.firebase.firestore { + public abstract class AggregateField { + method @NonNull public static com.google.firebase.firestore.AggregateField.AverageAggregateField average(@NonNull String); + method @NonNull public static com.google.firebase.firestore.AggregateField.AverageAggregateField average(@NonNull com.google.firebase.firestore.FieldPath); + method @NonNull public static com.google.firebase.firestore.AggregateField.CountAggregateField count(); + method @NonNull @RestrictTo(androidx.annotation.RestrictTo.Scope.LIBRARY) public String getAlias(); + method @NonNull @RestrictTo(androidx.annotation.RestrictTo.Scope.LIBRARY) public String getFieldPath(); + method @NonNull @RestrictTo(androidx.annotation.RestrictTo.Scope.LIBRARY) public String getOperator(); + method @NonNull public static com.google.firebase.firestore.AggregateField.SumAggregateField sum(@NonNull String); + method @NonNull public static com.google.firebase.firestore.AggregateField.SumAggregateField sum(@NonNull com.google.firebase.firestore.FieldPath); + } + + public static class AggregateField.AverageAggregateField extends com.google.firebase.firestore.AggregateField { + } + + public static class AggregateField.CountAggregateField extends com.google.firebase.firestore.AggregateField { + } + + public static class AggregateField.SumAggregateField extends com.google.firebase.firestore.AggregateField { + } + public class AggregateQuery { method @NonNull public com.google.android.gms.tasks.Task get(@NonNull com.google.firebase.firestore.AggregateSource); + method @NonNull public java.util.List getAggregateFields(); method @NonNull public com.google.firebase.firestore.Query getQuery(); } public class AggregateQuerySnapshot { + method @Nullable public Object get(@NonNull com.google.firebase.firestore.AggregateField); + method @RestrictTo(androidx.annotation.RestrictTo.Scope.LIBRARY) public long get(@NonNull com.google.firebase.firestore.AggregateField.CountAggregateField); + method @Nullable @RestrictTo(androidx.annotation.RestrictTo.Scope.LIBRARY) public Double get(@NonNull com.google.firebase.firestore.AggregateField.AverageAggregateField); method public long getCount(); + method @Nullable public Double getDouble(@NonNull com.google.firebase.firestore.AggregateField); + method @Nullable public Long getLong(@NonNull com.google.firebase.firestore.AggregateField); method @NonNull public com.google.firebase.firestore.AggregateQuery getQuery(); } @@ -378,6 +404,7 @@ package com.google.firebase.firestore { method @NonNull public com.google.firebase.firestore.ListenerRegistration addSnapshotListener(@NonNull com.google.firebase.firestore.MetadataChanges, @NonNull com.google.firebase.firestore.EventListener); method @NonNull public com.google.firebase.firestore.ListenerRegistration addSnapshotListener(@NonNull java.util.concurrent.Executor, @NonNull com.google.firebase.firestore.MetadataChanges, @NonNull com.google.firebase.firestore.EventListener); method @NonNull public com.google.firebase.firestore.ListenerRegistration addSnapshotListener(@NonNull android.app.Activity, @NonNull com.google.firebase.firestore.MetadataChanges, @NonNull com.google.firebase.firestore.EventListener); + method @NonNull public com.google.firebase.firestore.AggregateQuery aggregate(@NonNull com.google.firebase.firestore.AggregateField, @NonNull com.google.firebase.firestore.AggregateField...); method @NonNull public com.google.firebase.firestore.AggregateQuery count(); method @NonNull public com.google.firebase.firestore.Query endAt(@NonNull com.google.firebase.firestore.DocumentSnapshot); method @NonNull public com.google.firebase.firestore.Query endAt(java.lang.Object...); From bcb4f927aed1bb8908bda40edb929cba3c4f9481 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Thu, 3 Aug 2023 15:06:23 -0400 Subject: [PATCH 06/13] Change tests to match production --- .../firebase/firestore/AggregationTest.java | 304 ++++-------------- 1 file changed, 64 insertions(+), 240 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java index 8cd0f6be3b1..35faa234a4e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java @@ -43,7 +43,6 @@ import java.util.Collections; import java.util.Map; import org.junit.After; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; @@ -68,11 +67,6 @@ public void tearDown() { @Test public void testAggregateCountQueryEquals() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference coll1 = testCollection("foo"); CollectionReference coll1_same = coll1.firestore.collection(coll1.getPath()); AggregateQuery query1 = coll1.aggregate(AggregateField.count()); @@ -124,11 +118,6 @@ public void testAggregateCountQueryEquals() { @Test public void testAggregateSumQueryEquals() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference coll1 = testCollection("foo"); CollectionReference coll1_same = coll1.firestore.collection(coll1.getPath()); AggregateQuery query1 = coll1.aggregate(sum("baz")); @@ -180,11 +169,6 @@ public void testAggregateSumQueryEquals() { @Test public void testAggregateAvgQueryEquals() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference coll1 = testCollection("foo"); CollectionReference coll1_same = coll1.firestore.collection(coll1.getPath()); AggregateQuery query1 = coll1.aggregate(average("baz")); @@ -236,11 +220,6 @@ public void testAggregateAvgQueryEquals() { @Test public void testAggregateQueryNotEquals() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference coll = testCollection("foo"); AggregateQuery query1 = coll.aggregate(AggregateField.count()); @@ -279,11 +258,6 @@ public void testAggregateQueryNotEquals() { @Test public void testCanRunCountUsingAggregationMethod() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = @@ -294,11 +268,6 @@ public void testCanRunCountUsingAggregationMethod() { @Test public void testCanRunSumUsingAggregationMethod() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshotPages = @@ -318,11 +287,6 @@ public void testCanRunSumUsingAggregationMethod() { @Test public void testCanRunAvgUsingAggregationMethod() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = @@ -339,11 +303,6 @@ public void testCanRunAvgUsingAggregationMethod() { @Test public void testCanGetDuplicateAggregations() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = @@ -359,11 +318,6 @@ public void testCanGetDuplicateAggregations() { @Test public void testCanGetMultipleAggregationsInTheSameQuery() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = @@ -379,11 +333,6 @@ public void testCanGetMultipleAggregationsInTheSameQuery() { @Test public void testTerminateDoesNotCrashWithFlyingAggregateQuery() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); collection @@ -396,8 +345,9 @@ public void testTerminateDoesNotCrashWithFlyingAggregateQuery() { @Test public void testCanGetCorrectTypeForSum() { assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", + "This query requires a composite index, which is not support when testing" + + " against production. So it only runs against emulator which doesn't require " + + "an index. ", isRunningAgainstEmulator()); CollectionReference collection = testCollectionWithDocs(testDocs1); @@ -412,17 +362,45 @@ public void testCanGetCorrectTypeForSum() { Object sumHeight = snapshot.get(sum("height")); Object sumWeight = snapshot.get(sum("weight")); assertTrue(sumPages instanceof Long); - assertTrue(sumHeight instanceof Long); + assertTrue(sumHeight instanceof Double); assertTrue(sumWeight instanceof Double); } @Test - public void testCanGetCorrectTypeForAvg() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); + public void testCanGetCorrectDoubleTypeForSum() { + CollectionReference collection = testCollectionWithDocs(testDocs1); + AggregateQuerySnapshot snapshot = + waitFor(collection.aggregate(sum("weight")).get(AggregateSource.SERVER)); + + Object sumWeight = snapshot.get(sum("weight")); + assertTrue(sumWeight instanceof Double); + } + + @Test + public void testCanGetCorrectDoubleTypeForSumWhenFieldsAddUpToInteger() { + CollectionReference collection = testCollectionWithDocs(testDocs1); + + AggregateQuerySnapshot snapshot = + waitFor(collection.aggregate(sum("height")).get(AggregateSource.SERVER)); + + Object sumWeight = snapshot.get(sum("height")); + assertTrue(sumWeight instanceof Double); + } + + @Test + public void testCanGetCorrectLongTypeForSum() { + CollectionReference collection = testCollectionWithDocs(testDocs1); + + AggregateQuerySnapshot snapshot = + waitFor(collection.aggregate(sum("pages")).get(AggregateSource.SERVER)); + + Object sumPages = snapshot.get(sum("pages")); + assertTrue(sumPages instanceof Long); + } + + @Test + public void testCanGetCorrectTypeForAvg() { CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = @@ -435,8 +413,9 @@ public void testCanGetCorrectTypeForAvg() { @Test public void testCanPerformMaxAggregations() { assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", + "This query requires a composite index, which is not support when testing" + + " against production. So it only runs against emulator which doesn't require " + + "an index. ", isRunningAgainstEmulator()); CollectionReference collection = testCollectionWithDocs(testDocs1); @@ -458,11 +437,6 @@ public void testCanPerformMaxAggregations() { @Test public void testCannotPerformMoreThanMaxAggregations() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateField f1 = sum("pages"); AggregateField f2 = average("pages"); @@ -483,8 +457,9 @@ public void testCannotPerformMoreThanMaxAggregations() { @Test public void testCanRunAggregateCollectionGroupQuery() { assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", + "This query requires a composite index, which is not support when testing" + + " against production. So it only runs against emulator which doesn't require " + + "an index. ", isRunningAgainstEmulator()); FirebaseFirestore db = testFirestore(); @@ -526,11 +501,6 @@ public void testCanRunAggregateCollectionGroupQuery() { @Test public void testPerformsAggregationsWhenNaNExistsForSomeFieldValues() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", @@ -554,24 +524,14 @@ public void testPerformsAggregationsWhenNaNExistsForSomeFieldValues() { CollectionReference collection = testCollectionWithDocs(testDocs); AggregateQuerySnapshot snapshot = - waitFor( - collection - .aggregate(sum("rating"), sum("pages"), average("year"), average("rating")) - .get(AggregateSource.SERVER)); + waitFor(collection.aggregate(sum("rating"), average("rating")).get(AggregateSource.SERVER)); assertEquals(snapshot.get(sum("rating")), Double.NaN); - assertEquals(snapshot.get(sum("pages")), 300L); assertEquals(snapshot.get(average("rating")), (Double) Double.NaN); - assertEquals(snapshot.get(average("year")), (Double) 2000.0); } @Test public void testThrowsAnErrorWhenGettingTheResultOfAnUnrequestedAggregation() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = @@ -603,11 +563,6 @@ public void testThrowsAnErrorWhenGettingTheResultOfAnUnrequestedAggregation() { @Test public void testPerformsAggregationWhenUsingInOperator() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", @@ -624,26 +579,20 @@ public void testPerformsAggregationWhenUsingInOperator() { waitFor( collection .whereIn("rating", asList(5, 3)) - .aggregate( - sum("rating"), - average("rating"), - sum("pages"), - average("pages"), - AggregateField.count()) + .aggregate(sum("rating"), average("rating"), AggregateField.count()) .get(AggregateSource.SERVER)); assertEquals(snapshot.get(sum("rating")), 8L); assertEquals(snapshot.get(average("rating")), (Double) 4.0); - assertEquals(snapshot.get(sum("pages")), 200L); - assertEquals(snapshot.get(average("pages")), (Double) 100.0); assertEquals(snapshot.get(AggregateField.count()), 2L); } @Test public void testPerformsAggregationWhenUsingArrayContainsAnyOperator() { assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", + "This query requires a composite index, which is not support when testing" + + " against production. So it only runs against emulator which doesn't require " + + "an index. ", isRunningAgainstEmulator()); Map> testDocs = @@ -686,16 +635,9 @@ public void testPerformsAggregationWhenUsingArrayContainsAnyOperator() { waitFor( collection .whereArrayContainsAny("rating", asList(5, 3)) - .aggregate( - sum("rating"), - average("rating"), - sum("pages"), - average("pages"), - AggregateField.count()) + .aggregate(sum("pages"), average("pages"), AggregateField.count()) .get(AggregateSource.SERVER)); - assertEquals(snapshot.get(sum("rating")), 0L); - assertNull(snapshot.get(average("rating"))); assertEquals(snapshot.get(sum("pages")), 200L); assertEquals(snapshot.get(average("pages")), (Double) 100.0); assertEquals(snapshot.get(AggregateField.count()), 2L); @@ -703,11 +645,6 @@ public void testPerformsAggregationWhenUsingArrayContainsAnyOperator() { @Test public void testPerformsAggregationsOnNestedMapValues() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", @@ -732,28 +669,16 @@ public void testPerformsAggregationsOnNestedMapValues() { AggregateQuerySnapshot snapshot = waitFor( collection - .aggregate( - sum("metadata.pages"), - average("metadata.pages"), - average("metadata.rating.critic"), - sum("metadata.rating.user"), - AggregateField.count()) + .aggregate(sum("metadata.pages"), average("metadata.pages"), AggregateField.count()) .get(AggregateSource.SERVER)); assertEquals(snapshot.get(sum("metadata.pages")), 150L); assertEquals(snapshot.get(average("metadata.pages")), (Double) 75.0); - assertEquals(snapshot.get(average("metadata.rating.critic")), (Double) 3.0); - assertEquals(snapshot.get(sum("metadata.rating.user")), 9L); assertEquals(snapshot.get(AggregateField.count()), 2L); } @Test public void testPerformsSumThatOverflowsMaxLong() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", Long.MAX_VALUE), @@ -770,11 +695,6 @@ public void testPerformsSumThatOverflowsMaxLong() { @Test public void testPerformsSumThatCanOverflowIntegerValuesDuringAccumulation() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", Long.MAX_VALUE), @@ -792,11 +712,6 @@ public void testPerformsSumThatCanOverflowIntegerValuesDuringAccumulation() { @Test public void testPerformsSumThatIsNegative() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", Long.MAX_VALUE), @@ -813,11 +728,6 @@ public void testPerformsSumThatIsNegative() { @Test public void testPerformsSumThatIsPositiveInfinity() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", Double.MAX_VALUE), @@ -836,11 +746,6 @@ public void testPerformsSumThatIsPositiveInfinity() { @Test public void testPerformsSumThatIsNegativeInfinity() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", -Double.MAX_VALUE), @@ -860,10 +765,6 @@ public void testPerformsSumThatIsNegativeInfinity() { @Test public void testPerformsSumThatIsValidButCouldOverflowDuringAggregation() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); // Sum of rating would be 0, but if the accumulation overflow, we expect infinity Map> testDocs = map( @@ -877,17 +778,15 @@ public void testPerformsSumThatIsValidButCouldOverflowDuringAggregation() { waitFor(collection.aggregate(sum("rating")).get(AggregateSource.SERVER)); Object sum = snapshot.get(sum("rating")); - assertTrue(sum instanceof Long); - assertTrue(sum.equals(0L) || sum.equals(Long.MAX_VALUE) || sum.equals(Long.MIN_VALUE)); + assertTrue(sum instanceof Double); + assertTrue( + sum.equals(0.0) + || sum.equals(Double.POSITIVE_INFINITY) + || sum.equals(Double.NEGATIVE_INFINITY)); } @Test public void testPerformsSumOverResultSetOfZeroDocuments() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = @@ -902,11 +801,6 @@ public void testPerformsSumOverResultSetOfZeroDocuments() { @Test public void testPerformsSumOnlyOnNumericFields() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", 5), @@ -927,11 +821,6 @@ public void testPerformsSumOnlyOnNumericFields() { @Test public void testPerformsSumOfMinIEEE754() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map("a", map("author", "authorA", "title", "titleA", "rating", Double.MIN_VALUE)); @@ -945,11 +834,6 @@ public void testPerformsSumOfMinIEEE754() { @Test public void testPerformsAverageOfIntsThatResultsInAnInt() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", 10), @@ -967,11 +851,6 @@ public void testPerformsAverageOfIntsThatResultsInAnInt() { @Test public void testPerformsAverageOfFloatsThatResultsInAnInt() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", 10.5), @@ -988,11 +867,6 @@ public void testPerformsAverageOfFloatsThatResultsInAnInt() { @Test public void testPerformsAverageOfFloatsAndIntsThatResultsInAnInt() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", 10), @@ -1010,11 +884,6 @@ public void testPerformsAverageOfFloatsAndIntsThatResultsInAnInt() { @Test public void testPerformsAverageOfFloatsThatResultsInAFloat() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", 5.5), @@ -1032,11 +901,6 @@ public void testPerformsAverageOfFloatsThatResultsInAFloat() { @Test public void testPerformsAverageOfFloatsAndIntsThatResultsInAFloat() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", 8.6), @@ -1053,11 +917,6 @@ public void testPerformsAverageOfFloatsAndIntsThatResultsInAFloat() { @Test public void testPerformsAverageOfIntsThatResultsInAFloat() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", 10), @@ -1074,11 +933,6 @@ public void testPerformsAverageOfIntsThatResultsInAFloat() { @Test public void testPerformsAverageCausingUnderflow() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", Double.MIN_VALUE), @@ -1095,11 +949,6 @@ public void testPerformsAverageCausingUnderflow() { @Test public void testPerformsAverageOfMinIEEE754() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map("a", map("author", "authorA", "title", "titleA", "rating", Double.MIN_VALUE)); CollectionReference collection = testCollectionWithDocs(testDocs); @@ -1114,11 +963,6 @@ public void testPerformsAverageOfMinIEEE754() { @Test public void testPerformsAverageOverflowIEEE754DuringAccumulation() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", @@ -1137,11 +981,6 @@ public void testPerformsAverageOverflowIEEE754DuringAccumulation() { @Test public void testPerformsAverageThatIncludesNaN() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", @@ -1164,11 +1003,6 @@ public void testPerformsAverageThatIncludesNaN() { @Test public void testPerformsAverageOverResultSetOfZeroDocuments() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - CollectionReference collection = testCollectionWithDocs(testDocs1); AggregateQuerySnapshot snapshot = @@ -1185,11 +1019,6 @@ public void testPerformsAverageOverResultSetOfZeroDocuments() { @Test public void testPerformsAverageOnlyOnNumericFields() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - Map> testDocs = map( "a", map("author", "authorA", "title", "titleA", "rating", 5), @@ -1209,11 +1038,11 @@ public void testPerformsAverageOnlyOnNumericFields() { } @Test - @Ignore("TODO: Enable once we have production support for sum/avg.") public void testAggregateFailWithGoodMessageIfMissingIndex() { assumeFalse( - "Skip this test when running against the Firestore emulator because the Firestore emulator " - + "does not use indexes and never fails with a 'missing index' error", + "Skip this test when running against the Firestore emulator because the " + + "Firestore emulator does not use indexes and never fails with a 'missing index'" + + " error", isRunningAgainstEmulator()); CollectionReference collection = testCollectionWithDocs(Collections.emptyMap()); @@ -1231,17 +1060,12 @@ public void testAggregateFailWithGoodMessageIfMissingIndex() { @Test public void allowsAliasesLongerThan1500Bytes() { - assumeTrue( - "Skip this test if running against production because sum/avg is only support " - + "in emulator currently.", - isRunningAgainstEmulator()); - // The longest field name allowed is 1500. The alias chosen by the client is _. - // If the field name is - // 1500 bytes, the alias will be longer than 1500, which is the limit for aliases. This is to - // make sure the client - // can handle this corner case correctly. - StringBuilder builder = new StringBuilder(1500); - for (int i = 0; i < 1500; i++) { + // The longest field name allowed is 1500 bytes, and string sizes are calculated as the number + // of UTF-8 encoded bytes + 1 in server. The alias chosen by the client is _. + // If the field name is 1500 bytes, the alias will be longer than 1500, which is the limit for + // aliases. This is to make sure the client can handle this corner case correctly. + StringBuilder builder = new StringBuilder(1499); + for (int i = 0; i < 1499; i++) { builder.append("a"); } String longField = builder.toString(); From c78b50e3860c766f6cd63eef9b48641abcb766c9 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Fri, 11 Aug 2023 00:20:43 -0400 Subject: [PATCH 07/13] Address feedbacks --- .../firebase/firestore/AggregateField.java | 68 +++++++++++++++++++ .../firebase/firestore/AggregateQuery.java | 3 + .../firestore/AggregateQuerySnapshot.java | 3 - .../google/firebase/firestore/core/Query.java | 5 +- 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java index 22a013c33ae..19deaaff319 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java @@ -19,7 +19,9 @@ import androidx.annotation.RestrictTo; import java.util.Objects; +/** Represents an aggregation that can be performed by Firestore. */ public abstract class AggregateField { + /** The field over which the aggregation is performed. */ @Nullable private final FieldPath fieldPath; @NonNull private final String operator; @@ -86,43 +88,109 @@ public int hashCode() { return Objects.hash(getOperator(), getFieldPath()); } + /** + * Create a {@link CountAggregateField} object that can be used to compute the count of documents in the result set of a query. + * + *

The result of a count operation will always be a 64-bit integer value. + * + * @return The `CountAggregateField` object that can be used to compute the count of documents in the result set of a query. + */ @NonNull public static CountAggregateField count() { return new CountAggregateField(); } + /** + * Create a {@link SumAggregateField} object that can be used to compute the sum of a specified field over a range of documents in the result set of a query. + * + *

The result of a sum operation will always be a 64-bit integer value, a double, or NaN. + * + *

    + *
  • Summing over zero documents or fields will result in 0L. + *
  • Summing over NaN will result in a double value representing NaN. + *
  • A sum that overflows the maximum representable 64-bit integer value will result in a double return value. This may result in lost precision of the result. + *
  • A sum that overflows the maximum representable double value will result in a double return value representing infinity. + *
+ * + * @param field Specifies the field to sum across the result set. + * @return The `SumAggregateField` object that can be used to compute the sum of a specified field over a range of documents in the result set of a query. + */ @NonNull public static SumAggregateField sum(@NonNull String field) { return new SumAggregateField(FieldPath.fromDotSeparatedPath(field)); } + /** + * Create a {@link SumAggregateField} object that can be used to compute the sum of a specified field over a range of documents in the result set of a query. + * + *

The result of a sum operation will always be a 64-bit integer value, a double, or NaN. + * + *

    + *
  • Summing over zero documents or fields will result in 0L. + *
  • Summing over NaN will result in a double value representing NaN. + *
  • A sum that overflows the maximum representable 64-bit integer value will result in a double return value. This may result in lost precision of the result. + *
  • A sum that overflows the maximum representable double value will result in a double return value representing infinity. + *
+ * + * @param fieldPath Specifies the field to sum across the result set. + * @return The `SumAggregateField` object that can be used to compute the sum of a specified field over a range of documents in the result set of a query. + */ @NonNull public static SumAggregateField sum(@NonNull FieldPath fieldPath) { return new SumAggregateField(fieldPath); } + /** + * Create an {@link AverageAggregateField} object that can be used to compute the average of a specified field over a range of documents in the result set of a query. + * + *

The result of an average operation will always be a 64-bit integer value, a double, or NaN. + * + *

    + *
  • Averaging over zero documents or fields will result in a double value representing NaN. + *
  • Averaging over NaN will result in a double value representing NaN. + *
+ * + * @param field Specifies the field to average across the result set. + * @return The `AverageAggregateField` object that can be used to compute the average of a specified field over a range of documents in the result set of a query. + */ @NonNull public static AverageAggregateField average(@NonNull String field) { return new AverageAggregateField(FieldPath.fromDotSeparatedPath(field)); } + /** + * Create an {@link AverageAggregateField} object that can be used to compute the average of a specified field over a range of documents in the result set of a query. + * + *

The result of an average operation will always be a 64-bit integer value, a double, or NaN. + * + *

    + *
  • Averaging over zero documents or fields will result in a double value representing NaN. + *
  • Averaging over NaN will result in a double value representing NaN. + *
+ * + * @param fieldPath Specifies the field to average across the result set. + * @return The `AverageAggregateField` object that can be used to compute the average of a specified field over a range of documents in the result set of a query. + */ @NonNull public static AverageAggregateField average(@NonNull FieldPath fieldPath) { return new AverageAggregateField(fieldPath); } + /** Represents a "count" aggregation that can be performed by Firestore. */ public static class CountAggregateField extends AggregateField { private CountAggregateField() { super(null, "count"); } } + /** Represents a "sum" aggregation that can be performed by Firestore. */ public static class SumAggregateField extends AggregateField { private SumAggregateField(@NonNull FieldPath fieldPath) { super(fieldPath, "sum"); } } + /** Represents an "average" aggregation that can be performed by Firestore. */ public static class AverageAggregateField extends AggregateField { private AverageAggregateField(@NonNull FieldPath fieldPath) { super(fieldPath, "average"); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java index 0cb048d3259..549941b10c4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java @@ -15,6 +15,8 @@ package com.google.firebase.firestore; import androidx.annotation.NonNull; +import androidx.annotation.RestrictTo; + import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.firestore.util.Executors; @@ -47,6 +49,7 @@ public Query getQuery() { } /** Returns the AggregateFields included inside this object. */ + @RestrictTo(RestrictTo.Scope.LIBRARY) @NonNull public List getAggregateFields() { return aggregateFieldList; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuerySnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuerySnapshot.java index ddb3247bd7c..8f1ce3985c1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuerySnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuerySnapshot.java @@ -18,7 +18,6 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; -import androidx.annotation.RestrictTo; import com.google.firestore.v1.Value; import java.util.Collections; import java.util.Map; @@ -84,7 +83,6 @@ public Object get(@Nonnull AggregateField aggregateField) { * @param countAggregateField The count aggregation for which the value is requested. * @return The result of the given count aggregation. */ - @RestrictTo(RestrictTo.Scope.LIBRARY) public long get(@Nonnull AggregateField.CountAggregateField countAggregateField) { Long value = getLong(countAggregateField); if (value == null) { @@ -103,7 +101,6 @@ public long get(@Nonnull AggregateField.CountAggregateField countAggregateField) * @param averageAggregateField The average aggregation for which the value is requested. * @return The result of the given average aggregation. */ - @RestrictTo(RestrictTo.Scope.LIBRARY) @Nullable public Double get(@Nonnull AggregateField.AverageAggregateField averageAggregateField) { return getDouble(averageAggregateField); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 63fd0496645..1dd88bb4b09 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -59,12 +59,11 @@ public static Query atPath(ResourcePath path) { private List memoizedNormalizedOrderBys; - /** Returns a `Target` instance this query will be mapped to in backend and local store. */ + /** The corresponding `Target` of this `Query` instance, for use with non-aggregate queries. */ private @Nullable Target memoizedTarget; /** - * Returns a `Target` instance this query will be mapped to in backend and local store, for use - * within an aggregate query. + * The corresponding `Target` of this `Query` instance, for use with aggregate queries. Unlike targets for non-aggregate queries, aggregate query targets do not contain normalized order-bys, they only contain explicit order-bys. */ private @Nullable Target memoizedAggregateTarget; From 5bdc2ec31a453aa339a4310135d11d9d8abe13a5 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 11 Aug 2023 13:07:06 -0600 Subject: [PATCH 08/13] API text file, formatting, and PR feedback. --- firebase-firestore/api.txt | 6 +-- .../firebase/firestore/AggregateField.java | 46 ++++++++++++------- .../firebase/firestore/AggregateQuery.java | 1 - .../google/firebase/firestore/core/Query.java | 4 +- 4 files changed, 36 insertions(+), 21 deletions(-) diff --git a/firebase-firestore/api.txt b/firebase-firestore/api.txt index 9def04499f3..5499306241a 100644 --- a/firebase-firestore/api.txt +++ b/firebase-firestore/api.txt @@ -41,14 +41,14 @@ package com.google.firebase.firestore { public class AggregateQuery { method @NonNull public com.google.android.gms.tasks.Task get(@NonNull com.google.firebase.firestore.AggregateSource); - method @NonNull public java.util.List getAggregateFields(); + method @NonNull @RestrictTo(androidx.annotation.RestrictTo.Scope.LIBRARY) public java.util.List getAggregateFields(); method @NonNull public com.google.firebase.firestore.Query getQuery(); } public class AggregateQuerySnapshot { method @Nullable public Object get(@NonNull com.google.firebase.firestore.AggregateField); - method @RestrictTo(androidx.annotation.RestrictTo.Scope.LIBRARY) public long get(@NonNull com.google.firebase.firestore.AggregateField.CountAggregateField); - method @Nullable @RestrictTo(androidx.annotation.RestrictTo.Scope.LIBRARY) public Double get(@NonNull com.google.firebase.firestore.AggregateField.AverageAggregateField); + method public long get(@NonNull com.google.firebase.firestore.AggregateField.CountAggregateField); + method @Nullable public Double get(@NonNull com.google.firebase.firestore.AggregateField.AverageAggregateField); method public long getCount(); method @Nullable public Double getDouble(@NonNull com.google.firebase.firestore.AggregateField); method @Nullable public Long getLong(@NonNull com.google.firebase.firestore.AggregateField); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java index 19deaaff319..555f9ab5a88 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateField.java @@ -89,11 +89,13 @@ public int hashCode() { } /** - * Create a {@link CountAggregateField} object that can be used to compute the count of documents in the result set of a query. + * Create a {@link CountAggregateField} object that can be used to compute the count of documents + * in the result set of a query. * *

The result of a count operation will always be a 64-bit integer value. * - * @return The `CountAggregateField` object that can be used to compute the count of documents in the result set of a query. + * @return The `CountAggregateField` object that can be used to compute the count of documents in + * the result set of a query. */ @NonNull public static CountAggregateField count() { @@ -101,19 +103,23 @@ public static CountAggregateField count() { } /** - * Create a {@link SumAggregateField} object that can be used to compute the sum of a specified field over a range of documents in the result set of a query. + * Create a {@link SumAggregateField} object that can be used to compute the sum of a specified + * field over a range of documents in the result set of a query. * *

The result of a sum operation will always be a 64-bit integer value, a double, or NaN. * *

    *
  • Summing over zero documents or fields will result in 0L. *
  • Summing over NaN will result in a double value representing NaN. - *
  • A sum that overflows the maximum representable 64-bit integer value will result in a double return value. This may result in lost precision of the result. - *
  • A sum that overflows the maximum representable double value will result in a double return value representing infinity. + *
  • A sum that overflows the maximum representable 64-bit integer value will result in a + * double return value. This may result in lost precision of the result. + *
  • A sum that overflows the maximum representable double value will result in a double + * return value representing infinity. *
* * @param field Specifies the field to sum across the result set. - * @return The `SumAggregateField` object that can be used to compute the sum of a specified field over a range of documents in the result set of a query. + * @return The `SumAggregateField` object that can be used to compute the sum of a specified field + * over a range of documents in the result set of a query. */ @NonNull public static SumAggregateField sum(@NonNull String field) { @@ -121,19 +127,23 @@ public static SumAggregateField sum(@NonNull String field) { } /** - * Create a {@link SumAggregateField} object that can be used to compute the sum of a specified field over a range of documents in the result set of a query. + * Create a {@link SumAggregateField} object that can be used to compute the sum of a specified + * field over a range of documents in the result set of a query. * *

The result of a sum operation will always be a 64-bit integer value, a double, or NaN. * *

    *
  • Summing over zero documents or fields will result in 0L. *
  • Summing over NaN will result in a double value representing NaN. - *
  • A sum that overflows the maximum representable 64-bit integer value will result in a double return value. This may result in lost precision of the result. - *
  • A sum that overflows the maximum representable double value will result in a double return value representing infinity. + *
  • A sum that overflows the maximum representable 64-bit integer value will result in a + * double return value. This may result in lost precision of the result. + *
  • A sum that overflows the maximum representable double value will result in a double + * return value representing infinity. *
* * @param fieldPath Specifies the field to sum across the result set. - * @return The `SumAggregateField` object that can be used to compute the sum of a specified field over a range of documents in the result set of a query. + * @return The `SumAggregateField` object that can be used to compute the sum of a specified field + * over a range of documents in the result set of a query. */ @NonNull public static SumAggregateField sum(@NonNull FieldPath fieldPath) { @@ -141,9 +151,10 @@ public static SumAggregateField sum(@NonNull FieldPath fieldPath) { } /** - * Create an {@link AverageAggregateField} object that can be used to compute the average of a specified field over a range of documents in the result set of a query. + * Create an {@link AverageAggregateField} object that can be used to compute the average of a + * specified field over a range of documents in the result set of a query. * - *

The result of an average operation will always be a 64-bit integer value, a double, or NaN. + *

The result of an average operation will always be a double or NaN. * *

    *
  • Averaging over zero documents or fields will result in a double value representing NaN. @@ -151,7 +162,8 @@ public static SumAggregateField sum(@NonNull FieldPath fieldPath) { *
* * @param field Specifies the field to average across the result set. - * @return The `AverageAggregateField` object that can be used to compute the average of a specified field over a range of documents in the result set of a query. + * @return The `AverageAggregateField` object that can be used to compute the average of a + * specified field over a range of documents in the result set of a query. */ @NonNull public static AverageAggregateField average(@NonNull String field) { @@ -159,9 +171,10 @@ public static AverageAggregateField average(@NonNull String field) { } /** - * Create an {@link AverageAggregateField} object that can be used to compute the average of a specified field over a range of documents in the result set of a query. + * Create an {@link AverageAggregateField} object that can be used to compute the average of a + * specified field over a range of documents in the result set of a query. * - *

The result of an average operation will always be a 64-bit integer value, a double, or NaN. + *

The result of an average operation will always be a double or NaN. * *

    *
  • Averaging over zero documents or fields will result in a double value representing NaN. @@ -169,7 +182,8 @@ public static AverageAggregateField average(@NonNull String field) { *
* * @param fieldPath Specifies the field to average across the result set. - * @return The `AverageAggregateField` object that can be used to compute the average of a specified field over a range of documents in the result set of a query. + * @return The `AverageAggregateField` object that can be used to compute the average of a + * specified field over a range of documents in the result set of a query. */ @NonNull public static AverageAggregateField average(@NonNull FieldPath fieldPath) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java index 549941b10c4..db4015d6386 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java @@ -16,7 +16,6 @@ import androidx.annotation.NonNull; import androidx.annotation.RestrictTo; - import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.firestore.util.Executors; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 1dd88bb4b09..cdd14c7f565 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -63,7 +63,9 @@ public static Query atPath(ResourcePath path) { private @Nullable Target memoizedTarget; /** - * The corresponding `Target` of this `Query` instance, for use with aggregate queries. Unlike targets for non-aggregate queries, aggregate query targets do not contain normalized order-bys, they only contain explicit order-bys. + * The corresponding `Target` of this `Query` instance, for use with aggregate queries. Unlike + * targets for non-aggregate queries, aggregate query targets do not contain normalized order-bys, + * they only contain explicit order-bys. */ private @Nullable Target memoizedAggregateTarget; From a74b64953f98fd89c461ab15b71f84c93b60c483 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 26 Sep 2023 14:29:41 -0700 Subject: [PATCH 09/13] Update test to resolve merge conflict. --- .../firebase/firestore/core/QueryTest.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index 2a25daefcce..de3de67463c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -643,7 +643,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("aa", "<", 5)) .filter(filter("b", "<", 5)) .filter(filter("A", "<", 5)) - .getOrderBy()); + .getNormalizedOrderBy()); // numbers assertEquals( @@ -658,7 +658,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("1", "<", 5)) .filter(filter("2", "<", 5)) .filter(filter("19", "<", 5)) - .getOrderBy()); + .getNormalizedOrderBy()); // nested fields assertEquals( @@ -671,7 +671,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .filter(filter("aa", "<", 5)) .filter(filter("a.a", "<", 5)) - .getOrderBy()); + .getNormalizedOrderBy()); // special characters assertEquals( @@ -684,7 +684,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .filter(filter("_a", "<", 5)) .filter(filter("a.a", "<", 5)) - .getOrderBy()); + .getNormalizedOrderBy()); // field name with dot assertEquals( @@ -697,7 +697,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .filter(filter("`a.a`", "<", 5)) // Field name with dot .filter(filter("a.z", "<", 5)) // Nested field - .getOrderBy()); + .getNormalizedOrderBy()); // composite filter assertEquals( @@ -713,7 +713,7 @@ public void testImplicitOrderByInMultipleInequality() { andFilters( orFilters(filter("b", ">=", 1), filter("c", "<=", 1)), orFilters(filter("d", "<=", 1), filter("e", "==", 1)))) - .getOrderBy()); + .getNormalizedOrderBy()); // OrderBy assertEquals( @@ -727,7 +727,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .filter(filter("z", "<", 5)) .orderBy(orderBy("z")) - .getOrderBy()); + .getNormalizedOrderBy()); // last explicit order by direction assertEquals( @@ -740,7 +740,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("b", "<", 5)) .filter(filter("a", "<", 5)) .orderBy(orderBy("z", "desc")) - .getOrderBy()); + .getNormalizedOrderBy()); assertEquals( asList( @@ -754,7 +754,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .orderBy(orderBy("z", "desc")) .orderBy(orderBy("c")) - .getOrderBy()); + .getNormalizedOrderBy()); } @Test From f7b1ab0b3a1fd9266114221aedaf6e0965d9e340 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Tue, 26 Sep 2023 15:39:30 -0600 Subject: [PATCH 10/13] Fix rename issue from last merge. --- .../firebase/firestore/core/QueryTest.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index 2a25daefcce..de3de67463c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -643,7 +643,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("aa", "<", 5)) .filter(filter("b", "<", 5)) .filter(filter("A", "<", 5)) - .getOrderBy()); + .getNormalizedOrderBy()); // numbers assertEquals( @@ -658,7 +658,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("1", "<", 5)) .filter(filter("2", "<", 5)) .filter(filter("19", "<", 5)) - .getOrderBy()); + .getNormalizedOrderBy()); // nested fields assertEquals( @@ -671,7 +671,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .filter(filter("aa", "<", 5)) .filter(filter("a.a", "<", 5)) - .getOrderBy()); + .getNormalizedOrderBy()); // special characters assertEquals( @@ -684,7 +684,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .filter(filter("_a", "<", 5)) .filter(filter("a.a", "<", 5)) - .getOrderBy()); + .getNormalizedOrderBy()); // field name with dot assertEquals( @@ -697,7 +697,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .filter(filter("`a.a`", "<", 5)) // Field name with dot .filter(filter("a.z", "<", 5)) // Nested field - .getOrderBy()); + .getNormalizedOrderBy()); // composite filter assertEquals( @@ -713,7 +713,7 @@ public void testImplicitOrderByInMultipleInequality() { andFilters( orFilters(filter("b", ">=", 1), filter("c", "<=", 1)), orFilters(filter("d", "<=", 1), filter("e", "==", 1)))) - .getOrderBy()); + .getNormalizedOrderBy()); // OrderBy assertEquals( @@ -727,7 +727,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .filter(filter("z", "<", 5)) .orderBy(orderBy("z")) - .getOrderBy()); + .getNormalizedOrderBy()); // last explicit order by direction assertEquals( @@ -740,7 +740,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("b", "<", 5)) .filter(filter("a", "<", 5)) .orderBy(orderBy("z", "desc")) - .getOrderBy()); + .getNormalizedOrderBy()); assertEquals( asList( @@ -754,7 +754,7 @@ public void testImplicitOrderByInMultipleInequality() { .filter(filter("a", "<", 5)) .orderBy(orderBy("z", "desc")) .orderBy(orderBy("c")) - .getOrderBy()); + .getNormalizedOrderBy()); } @Test From 85fe2fb0f6f23f8634dc14b055ca0ba028754e1b Mon Sep 17 00:00:00 2001 From: Ehsan Date: Tue, 26 Sep 2023 15:04:54 -0700 Subject: [PATCH 11/13] Update CHANGELOG.md --- firebase-firestore/CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 6390fa4d175..ffccdfb135a 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,5 +1,5 @@ # Unreleased - +* [feature] Expose Sum/Average aggregate query support in API. [#5217](//github.com/firebase/firebase-android-sdk/pull/5217) # 24.8.1 * [fixed] Disabled `GradleMetadataPublishing` to fix breakage of the Kotlin extensions library. [#5337] @@ -24,7 +24,6 @@ updates. # 24.7.1 * [fixed] Implement equals method on Filter class. [#5210](//github.com/firebase/firebase-android-sdk/issues/5210) -* [feature] Expose Sum/Average aggregate query support in API. [#5217](//github.com/firebase/firebase-android-sdk/pull/5217) # 24.7.0 * [feature] Expose MultiDb support in API. [#4015](//github.com/firebase/firebase-android-sdk/issues/4015) From 002317d9d78decccf656e6a972c04155157f7a9a Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 13 Oct 2023 16:42:43 -0600 Subject: [PATCH 12/13] Update test for missing index configuration. --- .../google/firebase/firestore/AggregationTest.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java index 35faa234a4e..3c0d2b936eb 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java @@ -39,6 +39,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; import com.google.common.truth.Truth; +import com.google.firebase.firestore.model.DatabaseId; import com.google.firebase.firestore.testutil.IntegrationTestUtil; import java.util.Collections; import java.util.Map; @@ -1054,8 +1055,15 @@ public void testAggregateFailWithGoodMessageIfMissingIndex() { Throwable throwable = assertThrows(Throwable.class, () -> waitFor(task)); Throwable cause = throwable.getCause(); - Truth.assertThat(cause).hasMessageThat().ignoringCase().contains("index"); - Truth.assertThat(cause).hasMessageThat().contains("https://console.firebase.google.com"); + if (collection + .firestore + .getDatabaseId() + .getDatabaseId() + .equals(DatabaseId.DEFAULT_DATABASE_ID)) { + Truth.assertThat(cause).hasMessageThat().contains("https://console.firebase.google.com"); + } else { + Truth.assertThat(cause).hasMessageThat().contains("Missing index configuration"); + } } @Test From 891db367c82be110702b7b6530f10c049735add3 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Mon, 16 Oct 2023 12:40:06 -0600 Subject: [PATCH 13/13] Formatting --- .../com/google/firebase/firestore/AggregationTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java index 3c0d2b936eb..cd8580ea20d 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AggregationTest.java @@ -1056,10 +1056,10 @@ public void testAggregateFailWithGoodMessageIfMissingIndex() { Throwable cause = throwable.getCause(); if (collection - .firestore - .getDatabaseId() - .getDatabaseId() - .equals(DatabaseId.DEFAULT_DATABASE_ID)) { + .firestore + .getDatabaseId() + .getDatabaseId() + .equals(DatabaseId.DEFAULT_DATABASE_ID)) { Truth.assertThat(cause).hasMessageThat().contains("https://console.firebase.google.com"); } else { Truth.assertThat(cause).hasMessageThat().contains("Missing index configuration");