Fix: Aggregate query order-by normalization

Firestore/Source/API/FIRQuery.mm

@@ -587,7 +587,7 @@ - (Bound)boundFromSnapshot:(FIRDocumentSnapshot *)snapshot isInclusive:(BOOL)isI
   }
   const Document &document = *snapshot.internalDocument;
   const DatabaseId &databaseID = self.firestore.databaseID;
-  const std::vector<OrderBy> &order_bys = self.query.order_bys();
+  const std::vector<OrderBy> &order_bys = self.query.normalized_order_bys();
 
   SharedMessage<google_firestore_v1_ArrayValue> components{{}};
   components->values_count = CheckedSize(order_bys.size());

Firestore/core/src/core/query.cc

@@ -89,20 +89,20 @@ absl::optional<Operator> Query::FindOpInsideFilters(
   return absl::nullopt;
 }
 
-const std::vector<OrderBy>& Query::order_bys() const {
-  if (memoized_order_bys_.empty()) {
+const std::vector<OrderBy>& Query::normalized_order_bys() const {
+  if (memoized_normalized_order_bys_.empty()) {
     const FieldPath* inequality_field = InequalityFilterField();
     const FieldPath* first_order_by_field = FirstOrderByField();
     if (inequality_field && !first_order_by_field) {
       // In order to implicitly add key ordering, we must also add the
       // inequality filter field for it to be a valid query. Note that the
       // default inequality field and key ordering is ascending.
       if (inequality_field->IsKeyFieldPath()) {
-        memoized_order_bys_ = {
+        memoized_normalized_order_bys_ = {
             OrderBy(FieldPath::KeyFieldPath(), Direction::Ascending),
         };
       } else {
-        memoized_order_bys_ = {
+        memoized_normalized_order_bys_ = {
             OrderBy(*inequality_field, Direction::Ascending),
             OrderBy(FieldPath::KeyFieldPath(), Direction::Ascending),
         };
@@ -133,10 +133,10 @@ const std::vector<OrderBy>& Query::order_bys() const {
         result.emplace_back(FieldPath::KeyFieldPath(), last_direction);
       }
 
-      memoized_order_bys_ = std::move(result);
+      memoized_normalized_order_bys_ = std::move(result);
     }
   }
-  return memoized_order_bys_;
+  return memoized_normalized_order_bys_;
 }
 
 const FieldPath* Query::FirstOrderByField() const {
@@ -265,7 +265,7 @@ bool Query::MatchesOrderBy(const Document& doc) const {
   // 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 (const OrderBy& order_by : order_bys()) {
+  for (const OrderBy& order_by : normalized_order_bys()) {
     const FieldPath& field_path = order_by.field();
     // order by key always matches
     if (field_path != FieldPath::KeyFieldPath() &&
@@ -277,17 +277,18 @@ bool Query::MatchesOrderBy(const Document& doc) const {
 }
 
 bool Query::MatchesBounds(const Document& doc) const {
-  if (start_at_ && !start_at_->SortsBeforeDocument(order_bys(), doc)) {
+  if (start_at_ &&
+      !start_at_->SortsBeforeDocument(normalized_order_bys(), doc)) {
     return false;
   }
-  if (end_at_ && !end_at_->SortsAfterDocument(order_bys(), doc)) {
+  if (end_at_ && !end_at_->SortsAfterDocument(normalized_order_bys(), doc)) {
     return false;
   }
   return true;
 }
 
 model::DocumentComparator Query::Comparator() const {
-  std::vector<OrderBy> ordering = order_bys();
+  std::vector<OrderBy> ordering = normalized_order_bys();
 
   bool has_key_ordering = false;
   for (const OrderBy& order_by : ordering) {
@@ -329,39 +330,52 @@ std::string Query::ToString() const {
 
 const Target& Query::ToTarget() const& {
   if (memoized_target == nullptr) {
-    if (limit_type_ == LimitType::Last) {
-      // Flip the orderBy directions since we want the last results
-      std::vector<OrderBy> new_order_bys;
-      for (const auto& order_by : order_bys()) {
-        Direction dir = order_by.direction() == Direction::Descending
-                            ? Direction::Ascending
-                            : Direction::Descending;
-        new_order_bys.emplace_back(order_by.field(), dir);
-      }
-
-      // We need to swap the cursors to match the now-flipped query ordering.
-      auto new_start_at = end_at_
-                              ? absl::optional<Bound>{Bound::FromValue(
-                                    end_at_->position(), end_at_->inclusive())}
-                              : absl::nullopt;
-      auto new_end_at =
-          start_at_ ? absl::optional<Bound>{Bound::FromValue(
-                          start_at_->position(), start_at_->inclusive())}
-                    : absl::nullopt;
-
-      Target target(path(), collection_group(), filters(), new_order_bys,
-                    limit_, new_start_at, new_end_at);
-      memoized_target = std::make_shared<Target>(std::move(target));
-    } else {
-      Target target(path(), collection_group(), filters(), order_bys(), limit_,
-                    start_at(), end_at());
-      memoized_target = std::make_shared<Target>(std::move(target));
-    }
+    memoized_target = ToTarget(normalized_order_bys());
   }
 
   return *memoized_target;
 }
 
+const Target& Query::ToAggregateTarget() const& {
+  if (memoized_aggregate_target == nullptr) {
+    memoized_aggregate_target = ToTarget(explicit_order_bys_);
+  }
+
+  return *memoized_aggregate_target;
+}
+
+const std::shared_ptr<const Target> Query::ToTarget(
+    const std::vector<OrderBy>& order_bys) const& {
+  if (limit_type_ == LimitType::Last) {
+    // Flip the orderBy directions since we want the last results
+    std::vector<OrderBy> new_order_bys;
+    for (const auto& order_by : order_bys) {
+      Direction dir = order_by.direction() == Direction::Descending
+                          ? Direction::Ascending
+                          : Direction::Descending;
+      new_order_bys.emplace_back(order_by.field(), dir);
+    }
+
+    // We need to swap the cursors to match the now-flipped query ordering.
+    auto new_start_at = end_at_
+                            ? absl::optional<Bound>{Bound::FromValue(
+                                  end_at_->position(), end_at_->inclusive())}
+                            : absl::nullopt;
+    auto new_end_at = start_at_
+                          ? absl::optional<Bound>{Bound::FromValue(
+                                start_at_->position(), start_at_->inclusive())}
+                          : absl::nullopt;
+
+    Target target(path(), collection_group(), filters(), new_order_bys, limit_,
+                  new_start_at, new_end_at);
+    return std::make_shared<Target>(std::move(target));
+  } else {
+    Target target(path(), collection_group(), filters(), order_bys, limit_,
+                  start_at(), end_at());
+    return std::make_shared<Target>(std::move(target));
+  }
+}
+
 std::ostream& operator<<(std::ostream& os, const Query& query) {
   return os << query.ToString();
 }

Firestore/core/src/core/query.h

@@ -136,12 +136,12 @@ class Query {
   }
 
   /**
-   * Returns the full list of ordering constraints on the query.
+   * Returns the normalized list of ordering constraints on the query.
    *
    * This might include additional sort orders added implicitly to match the
    * backend behavior.
    */
-  const std::vector<OrderBy>& order_bys() const;
+  const std::vector<OrderBy>& normalized_order_bys() const;
 
   /** Returns the first field in an order-by constraint, or nullptr if none. */
   const model::FieldPath* FirstOrderByField() const;
@@ -244,6 +244,14 @@ class Query {
    */
   const Target& ToTarget() const&;
 
+  /**
+   * Returns a `Target` instance this query will be mapped to in backend
+   * and local store, for use within an aggregate query. Unlike targets
+   * for non-aggregate queries, aggregate query targets do not contain
+   * normalized order-bys, they only contain explicit order-bys.
+   */
+  const Target& ToAggregateTarget() const&;
+
   friend std::ostream& operator<<(std::ostream& os, const Query& query);
 
   friend bool operator==(const Query& lhs, const Query& rhs);
@@ -270,7 +278,7 @@ class Query {
   std::vector<OrderBy> explicit_order_bys_;
 
   // The memoized list of sort orders.
-  mutable std::vector<OrderBy> memoized_order_bys_;
+  mutable std::vector<OrderBy> memoized_normalized_order_bys_;
 
   int32_t limit_ = Target::kNoLimit;
   LimitType limit_type_ = LimitType::None;
@@ -280,6 +288,14 @@ class Query {
 
   // The corresponding Target of this Query instance.
   mutable std::shared_ptr<const Target> memoized_target;
+
+  // The corresponding aggregate Target of this Query instance. Unlike targets
+  // for non-aggregate queries, aggregate query targets do not contain
+  // normalized order-bys, they only contain explicit order-bys.
+  mutable std::shared_ptr<const Target> memoized_aggregate_target;
+
+  const std::shared_ptr<const Target> ToTarget(
+      const std::vector<OrderBy>& order_bys) const&;
 };
 
 bool operator==(const Query& lhs, const Query& rhs);

Firestore/core/test/unit/core/query_test.cc

@@ -65,10 +65,10 @@ TEST(QueryTest, Constructor) {
   const ResourcePath path{"rooms", "Firestore", "messages", "0001"};
   Query query(path);
 
-  ASSERT_EQ(1, query.order_bys().size());
+  ASSERT_EQ(1, query.normalized_order_bys().size());
   EXPECT_EQ(FieldPath::kDocumentKeyPath,
-            query.order_bys()[0].field().CanonicalString());
-  EXPECT_EQ(true, query.order_bys()[0].ascending());
+            query.normalized_order_bys()[0].field().CanonicalString());
+  EXPECT_EQ(true, query.normalized_order_bys()[0].ascending());
 
   ASSERT_EQ(0, query.explicit_order_bys().size());
 }
@@ -78,12 +78,13 @@ TEST(QueryTest, OrderBy) {
                    .AddingOrderBy(testutil::OrderBy(Field("length"),
                                                     Direction::Descending));
 
-  ASSERT_EQ(2, query.order_bys().size());
-  EXPECT_EQ("length", query.order_bys()[0].field().CanonicalString());
-  EXPECT_EQ(false, query.order_bys()[0].ascending());
+  ASSERT_EQ(2, query.normalized_order_bys().size());
+  EXPECT_EQ("length",
+            query.normalized_order_bys()[0].field().CanonicalString());
+  EXPECT_EQ(false, query.normalized_order_bys()[0].ascending());
   EXPECT_EQ(FieldPath::kDocumentKeyPath,
-            query.order_bys()[1].field().CanonicalString());
-  EXPECT_EQ(false, query.order_bys()[1].ascending());
+            query.normalized_order_bys()[1].field().CanonicalString());
+  EXPECT_EQ(false, query.normalized_order_bys()[1].ascending());
 
   ASSERT_EQ(1, query.explicit_order_bys().size());
   EXPECT_EQ("length", query.explicit_order_bys()[0].field().CanonicalString());
@@ -770,64 +771,64 @@ TEST(QueryTest, UniqueIds) {
 TEST(QueryTest, ImplicitOrderBy) {
   auto base_query = testutil::Query("foo");
   // Default is ascending
-  ASSERT_EQ(base_query.order_bys(),
+  ASSERT_EQ(base_query.normalized_order_bys(),
             std::vector<core::OrderBy>{
                 testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")});
 
   // Explicit key ordering is respected
   ASSERT_EQ(
       base_query
           .AddingOrderBy(testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"))
-          .order_bys(),
+          .normalized_order_bys(),
       std::vector<OrderBy>{
           testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")});
   ASSERT_EQ(
       base_query
           .AddingOrderBy(testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc"))
-          .order_bys(),
+          .normalized_order_bys(),
       std::vector<OrderBy>{
           testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc")});
 
   ASSERT_EQ(
       base_query.AddingOrderBy(testutil::OrderBy("foo", "asc"))
           .AddingOrderBy(testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"))
-          .order_bys(),
+          .normalized_order_bys(),
       (std::vector<OrderBy>{
           testutil::OrderBy("foo", "asc"),
           testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")}));
 
   ASSERT_EQ(
       base_query.AddingOrderBy(testutil::OrderBy("foo", "asc"))
           .AddingOrderBy(testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc"))
-          .order_bys(),
+          .normalized_order_bys(),
       (std::vector<OrderBy>{
           testutil::OrderBy("foo", "asc"),
           testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc")}));
 
   // Inequality filters add order bys
-  ASSERT_EQ(
-      base_query.AddingFilter(testutil::Filter("foo", "<", 5)).order_bys(),
-      (std::vector<OrderBy>{
-          testutil::OrderBy("foo", "asc"),
-          testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")}));
+  ASSERT_EQ(base_query.AddingFilter(testutil::Filter("foo", "<", 5))
+                .normalized_order_bys(),
+            (std::vector<OrderBy>{
+                testutil::OrderBy("foo", "asc"),
+                testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")}));
 
   // Descending order by applies to implicit key ordering
-  ASSERT_EQ(
-      base_query.AddingOrderBy(testutil::OrderBy("foo", "desc")).order_bys(),
-      (std::vector<OrderBy>{
-          testutil::OrderBy("foo", "desc"),
-          testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc")}));
+  ASSERT_EQ(base_query.AddingOrderBy(testutil::OrderBy("foo", "desc"))
+                .normalized_order_bys(),
+            (std::vector<OrderBy>{
+                testutil::OrderBy("foo", "desc"),
+                testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc")}));
   ASSERT_EQ(base_query.AddingOrderBy(testutil::OrderBy("foo", "asc"))
                 .AddingOrderBy(testutil::OrderBy("bar", "desc"))
-                .order_bys(),
+                .normalized_order_bys(),
             (std::vector<OrderBy>{
                 testutil::OrderBy("foo", "asc"),
                 testutil::OrderBy("bar", "desc"),
                 testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc"),
             }));
   ASSERT_EQ(base_query.AddingOrderBy(testutil::OrderBy("foo", "desc"))
                 .AddingOrderBy(testutil::OrderBy("bar", "asc"))
-                .order_bys(),
+                .normalized_order_bys(),
             (std::vector<OrderBy>{
                 testutil::OrderBy("foo", "desc"),
                 testutil::OrderBy("bar", "asc"),
@@ -917,6 +918,58 @@ TEST(QueryTest, MatchesAllDocuments) {
   EXPECT_FALSE(query.MatchesAllDocuments());
 }
 
+TEST(QueryTest, OrderByForAggregateAndNonAggregate) {
+  auto col = testutil::Query("coll");
+
+  // Build two identical queries
+  auto query1 = col.AddingFilter(testutil::Filter("foo", ">", 1));
+  auto query2 = col.AddingFilter(testutil::Filter("foo", ">", 1));
+
+  // Compute an aggregate and non-aggregate target from the queries
+  auto aggregateTarget = query1.ToAggregateTarget();
+  auto target = query2.ToTarget();
+
+  EXPECT_EQ(aggregateTarget.order_bys().size(), 0);
+
+  ASSERT_EQ(target.order_bys().size(), 2);
+  EXPECT_EQ(target.order_bys()[0].direction(), Direction::Ascending);
+  EXPECT_EQ(target.order_bys()[0].field().CanonicalString(), "foo");
+  EXPECT_EQ(target.order_bys()[1].direction(), Direction::Ascending);
+  EXPECT_EQ(target.order_bys()[1].field().CanonicalString(), "__name__");
+}
+
+TEST(QueryTest, GeneratedOrderBysNotAffectedByPreviouslyMemoizedTargets) {
+  auto col = testutil::Query("coll");
+
+  // Build two identical queries
+  auto query1 = col.AddingFilter(testutil::Filter("foo", ">", 1));
+  auto query2 = col.AddingFilter(testutil::Filter("foo", ">", 1));
+
+  // query1 - first to aggregate target, then to non-aggregate target
+  auto aggregateTarget1 = query1.ToAggregateTarget();
+  auto target1 = query1.ToTarget();
+
+  // query2 - first to aggregate target, then to non-aggregate target
+  auto aggregateTarget2 = query2.ToAggregateTarget();
+  auto target2 = query2.ToTarget();
+
+  EXPECT_EQ(aggregateTarget1.order_bys().size(), 0);
+
+  EXPECT_EQ(aggregateTarget2.order_bys().size(), 0);
+
+  ASSERT_EQ(target1.order_bys().size(), 2);
+  EXPECT_EQ(target1.order_bys()[0].direction(), Direction::Ascending);
+  EXPECT_EQ(target1.order_bys()[0].field().CanonicalString(), "foo");
+  EXPECT_EQ(target1.order_bys()[1].direction(), Direction::Ascending);
+  EXPECT_EQ(target1.order_bys()[1].field().CanonicalString(), "__name__");
+
+  ASSERT_EQ(target2.order_bys().size(), 2);
+  EXPECT_EQ(target2.order_bys()[0].direction(), Direction::Ascending);
+  EXPECT_EQ(target2.order_bys()[0].field().CanonicalString(), "foo");
+  EXPECT_EQ(target2.order_bys()[1].direction(), Direction::Ascending);
+  EXPECT_EQ(target2.order_bys()[1].field().CanonicalString(), "__name__");
+}
+
 }  // namespace core
 }  // namespace firestore
 }  // namespace firebase