Skip to content

Commit 45c3004

Browse files
MarkDuckworthMarkDuckworth
authored
Fix: Aggregate query order-by normalization (#11635)
Fix aggregate query order-by normalization to better support future aggregations. This has no affect on count.
1 parent e04f324 commit 45c3004

File tree

4 files changed

+150
-67
lines changed

4 files changed

+150
-67
lines changed

‎Firestore/Source/API/FIRQuery.mm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -587,7 +587,7 @@ - (Bound)boundFromSnapshot:(FIRDocumentSnapshot *)snapshot isInclusive:(BOOL)isI
587587
}
588588
const Document &document = *snapshot.internalDocument;
589589
const DatabaseId &databaseID = self.firestore.databaseID;
590-
const std::vector<OrderBy> &order_bys = self.query.order_bys();
590+
const std::vector<OrderBy> &order_bys = self.query.normalized_order_bys();
591591

592592
SharedMessage<google_firestore_v1_ArrayValue> components{{}};
593593
components->values_count = CheckedSize(order_bys.size());

‎Firestore/core/src/core/query.cc

Lines changed: 52 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -89,20 +89,20 @@ absl::optional<Operator> Query::FindOpInsideFilters(
8989
return absl::nullopt;
9090
}
9191

92-
const std::vector<OrderBy>& Query::order_bys() const {
93-
if (memoized_order_bys_.empty()) {
92+
const std::vector<OrderBy>& Query::normalized_order_bys() const {
93+
if (memoized_normalized_order_bys_.empty()) {
9494
const FieldPath* inequality_field = InequalityFilterField();
9595
const FieldPath* first_order_by_field = FirstOrderByField();
9696
if (inequality_field && !first_order_by_field) {
9797
// In order to implicitly add key ordering, we must also add the
9898
// inequality filter field for it to be a valid query. Note that the
9999
// default inequality field and key ordering is ascending.
100100
if (inequality_field->IsKeyFieldPath()) {
101-
memoized_order_bys_ = {
101+
memoized_normalized_order_bys_ = {
102102
OrderBy(FieldPath::KeyFieldPath(), Direction::Ascending),
103103
};
104104
} else {
105-
memoized_order_bys_ = {
105+
memoized_normalized_order_bys_ = {
106106
OrderBy(*inequality_field, Direction::Ascending),
107107
OrderBy(FieldPath::KeyFieldPath(), Direction::Ascending),
108108
};
@@ -133,10 +133,10 @@ const std::vector<OrderBy>& Query::order_bys() const {
133133
result.emplace_back(FieldPath::KeyFieldPath(), last_direction);
134134
}
135135

136-
memoized_order_bys_ = std::move(result);
136+
memoized_normalized_order_bys_ = std::move(result);
137137
}
138138
}
139-
return memoized_order_bys_;
139+
return memoized_normalized_order_bys_;
140140
}
141141

142142
const FieldPath* Query::FirstOrderByField() const {
@@ -265,7 +265,7 @@ bool Query::MatchesOrderBy(const Document& doc) const {
265265
// to the inequality, and is evaluated as "a > 1 orderBy a || b == 1 orderBy
266266
// a". A document with content of {b:1} matches the filters, but does not
267267
// match the orderBy because it's missing the field 'a'.
268-
for (const OrderBy& order_by : order_bys()) {
268+
for (const OrderBy& order_by : normalized_order_bys()) {
269269
const FieldPath& field_path = order_by.field();
270270
// order by key always matches
271271
if (field_path != FieldPath::KeyFieldPath() &&
@@ -277,17 +277,18 @@ bool Query::MatchesOrderBy(const Document& doc) const {
277277
}
278278

279279
bool Query::MatchesBounds(const Document& doc) const {
280-
if (start_at_ && !start_at_->SortsBeforeDocument(order_bys(), doc)) {
280+
if (start_at_ &&
281+
!start_at_->SortsBeforeDocument(normalized_order_bys(), doc)) {
281282
return false;
282283
}
283-
if (end_at_ && !end_at_->SortsAfterDocument(order_bys(), doc)) {
284+
if (end_at_ && !end_at_->SortsAfterDocument(normalized_order_bys(), doc)) {
284285
return false;
285286
}
286287
return true;
287288
}
288289

289290
model::DocumentComparator Query::Comparator() const {
290-
std::vector<OrderBy> ordering = order_bys();
291+
std::vector<OrderBy> ordering = normalized_order_bys();
291292

292293
bool has_key_ordering = false;
293294
for (const OrderBy& order_by : ordering) {
@@ -329,39 +330,52 @@ std::string Query::ToString() const {
329330

330331
const Target& Query::ToTarget() const& {
331332
if (memoized_target == nullptr) {
332-
if (limit_type_ == LimitType::Last) {
333-
// Flip the orderBy directions since we want the last results
334-
std::vector<OrderBy> new_order_bys;
335-
for (const auto& order_by : order_bys()) {
336-
Direction dir = order_by.direction() == Direction::Descending
337-
? Direction::Ascending
338-
: Direction::Descending;
339-
new_order_bys.emplace_back(order_by.field(), dir);
340-
}
341-
342-
// We need to swap the cursors to match the now-flipped query ordering.
343-
auto new_start_at = end_at_
344-
? absl::optional<Bound>{Bound::FromValue(
345-
end_at_->position(), end_at_->inclusive())}
346-
: absl::nullopt;
347-
auto new_end_at =
348-
start_at_ ? absl::optional<Bound>{Bound::FromValue(
349-
start_at_->position(), start_at_->inclusive())}
350-
: absl::nullopt;
351-
352-
Target target(path(), collection_group(), filters(), new_order_bys,
353-
limit_, new_start_at, new_end_at);
354-
memoized_target = std::make_shared<Target>(std::move(target));
355-
} else {
356-
Target target(path(), collection_group(), filters(), order_bys(), limit_,
357-
start_at(), end_at());
358-
memoized_target = std::make_shared<Target>(std::move(target));
359-
}
333+
memoized_target = ToTarget(normalized_order_bys());
360334
}
361335

362336
return *memoized_target;
363337
}
364338

339+
const Target& Query::ToAggregateTarget() const& {
340+
if (memoized_aggregate_target == nullptr) {
341+
memoized_aggregate_target = ToTarget(explicit_order_bys_);
342+
}
343+
344+
return *memoized_aggregate_target;
345+
}
346+
347+
const std::shared_ptr<const Target> Query::ToTarget(
348+
const std::vector<OrderBy>& order_bys) const& {
349+
if (limit_type_ == LimitType::Last) {
350+
// Flip the orderBy directions since we want the last results
351+
std::vector<OrderBy> new_order_bys;
352+
for (const auto& order_by : order_bys) {
353+
Direction dir = order_by.direction() == Direction::Descending
354+
? Direction::Ascending
355+
: Direction::Descending;
356+
new_order_bys.emplace_back(order_by.field(), dir);
357+
}
358+
359+
// We need to swap the cursors to match the now-flipped query ordering.
360+
auto new_start_at = end_at_
361+
? absl::optional<Bound>{Bound::FromValue(
362+
end_at_->position(), end_at_->inclusive())}
363+
: absl::nullopt;
364+
auto new_end_at = start_at_
365+
? absl::optional<Bound>{Bound::FromValue(
366+
start_at_->position(), start_at_->inclusive())}
367+
: absl::nullopt;
368+
369+
Target target(path(), collection_group(), filters(), new_order_bys, limit_,
370+
new_start_at, new_end_at);
371+
return std::make_shared<Target>(std::move(target));
372+
} else {
373+
Target target(path(), collection_group(), filters(), order_bys, limit_,
374+
start_at(), end_at());
375+
return std::make_shared<Target>(std::move(target));
376+
}
377+
}
378+
365379
std::ostream& operator<<(std::ostream& os, const Query& query) {
366380
return os << query.ToString();
367381
}

‎Firestore/core/src/core/query.h

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,12 +136,12 @@ class Query {
136136
}
137137

138138
/**
139-
* Returns the full list of ordering constraints on the query.
139+
* Returns the normalized list of ordering constraints on the query.
140140
*
141141
* This might include additional sort orders added implicitly to match the
142142
* backend behavior.
143143
*/
144-
const std::vector<OrderBy>& order_bys() const;
144+
const std::vector<OrderBy>& normalized_order_bys() const;
145145

146146
/** Returns the first field in an order-by constraint, or nullptr if none. */
147147
const model::FieldPath* FirstOrderByField() const;
@@ -244,6 +244,14 @@ class Query {
244244
*/
245245
const Target& ToTarget() const&;
246246

247+
/**
248+
* Returns a `Target` instance this query will be mapped to in backend
249+
* and local store, for use within an aggregate query. Unlike targets
250+
* for non-aggregate queries, aggregate query targets do not contain
251+
* normalized order-bys, they only contain explicit order-bys.
252+
*/
253+
const Target& ToAggregateTarget() const&;
254+
247255
friend std::ostream& operator<<(std::ostream& os, const Query& query);
248256

249257
friend bool operator==(const Query& lhs, const Query& rhs);
@@ -270,7 +278,7 @@ class Query {
270278
std::vector<OrderBy> explicit_order_bys_;
271279

272280
// The memoized list of sort orders.
273-
mutable std::vector<OrderBy> memoized_order_bys_;
281+
mutable std::vector<OrderBy> memoized_normalized_order_bys_;
274282

275283
int32_t limit_ = Target::kNoLimit;
276284
LimitType limit_type_ = LimitType::None;
@@ -280,6 +288,14 @@ class Query {
280288

281289
// The corresponding Target of this Query instance.
282290
mutable std::shared_ptr<const Target> memoized_target;
291+
292+
// The corresponding aggregate Target of this Query instance. Unlike targets
293+
// for non-aggregate queries, aggregate query targets do not contain
294+
// normalized order-bys, they only contain explicit order-bys.
295+
mutable std::shared_ptr<const Target> memoized_aggregate_target;
296+
297+
const std::shared_ptr<const Target> ToTarget(
298+
const std::vector<OrderBy>& order_bys) const&;
283299
};
284300

285301
bool operator==(const Query& lhs, const Query& rhs);

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

Lines changed: 78 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,10 @@ TEST(QueryTest, Constructor) {
6565
const ResourcePath path{"rooms", "Firestore", "messages", "0001"};
6666
Query query(path);
6767

68-
ASSERT_EQ(1, query.order_bys().size());
68+
ASSERT_EQ(1, query.normalized_order_bys().size());
6969
EXPECT_EQ(FieldPath::kDocumentKeyPath,
70-
query.order_bys()[0].field().CanonicalString());
71-
EXPECT_EQ(true, query.order_bys()[0].ascending());
70+
query.normalized_order_bys()[0].field().CanonicalString());
71+
EXPECT_EQ(true, query.normalized_order_bys()[0].ascending());
7272

7373
ASSERT_EQ(0, query.explicit_order_bys().size());
7474
}
@@ -78,12 +78,13 @@ TEST(QueryTest, OrderBy) {
7878
.AddingOrderBy(testutil::OrderBy(Field("length"),
7979
Direction::Descending));
8080

81-
ASSERT_EQ(2, query.order_bys().size());
82-
EXPECT_EQ("length", query.order_bys()[0].field().CanonicalString());
83-
EXPECT_EQ(false, query.order_bys()[0].ascending());
81+
ASSERT_EQ(2, query.normalized_order_bys().size());
82+
EXPECT_EQ("length",
83+
query.normalized_order_bys()[0].field().CanonicalString());
84+
EXPECT_EQ(false, query.normalized_order_bys()[0].ascending());
8485
EXPECT_EQ(FieldPath::kDocumentKeyPath,
85-
query.order_bys()[1].field().CanonicalString());
86-
EXPECT_EQ(false, query.order_bys()[1].ascending());
86+
query.normalized_order_bys()[1].field().CanonicalString());
87+
EXPECT_EQ(false, query.normalized_order_bys()[1].ascending());
8788

8889
ASSERT_EQ(1, query.explicit_order_bys().size());
8990
EXPECT_EQ("length", query.explicit_order_bys()[0].field().CanonicalString());
@@ -770,64 +771,64 @@ TEST(QueryTest, UniqueIds) {
770771
TEST(QueryTest, ImplicitOrderBy) {
771772
auto base_query = testutil::Query("foo");
772773
// Default is ascending
773-
ASSERT_EQ(base_query.order_bys(),
774+
ASSERT_EQ(base_query.normalized_order_bys(),
774775
std::vector<core::OrderBy>{
775776
testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")});
776777

777778
// Explicit key ordering is respected
778779
ASSERT_EQ(
779780
base_query
780781
.AddingOrderBy(testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"))
781-
.order_bys(),
782+
.normalized_order_bys(),
782783
std::vector<OrderBy>{
783784
testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")});
784785
ASSERT_EQ(
785786
base_query
786787
.AddingOrderBy(testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc"))
787-
.order_bys(),
788+
.normalized_order_bys(),
788789
std::vector<OrderBy>{
789790
testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc")});
790791

791792
ASSERT_EQ(
792793
base_query.AddingOrderBy(testutil::OrderBy("foo", "asc"))
793794
.AddingOrderBy(testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"))
794-
.order_bys(),
795+
.normalized_order_bys(),
795796
(std::vector<OrderBy>{
796797
testutil::OrderBy("foo", "asc"),
797798
testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")}));
798799

799800
ASSERT_EQ(
800801
base_query.AddingOrderBy(testutil::OrderBy("foo", "asc"))
801802
.AddingOrderBy(testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc"))
802-
.order_bys(),
803+
.normalized_order_bys(),
803804
(std::vector<OrderBy>{
804805
testutil::OrderBy("foo", "asc"),
805806
testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc")}));
806807

807808
// Inequality filters add order bys
808-
ASSERT_EQ(
809-
base_query.AddingFilter(testutil::Filter("foo", "<", 5)).order_bys(),
810-
(std::vector<OrderBy>{
811-
testutil::OrderBy("foo", "asc"),
812-
testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")}));
809+
ASSERT_EQ(base_query.AddingFilter(testutil::Filter("foo", "<", 5))
810+
.normalized_order_bys(),
811+
(std::vector<OrderBy>{
812+
testutil::OrderBy("foo", "asc"),
813+
testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc")}));
813814

814815
// Descending order by applies to implicit key ordering
815-
ASSERT_EQ(
816-
base_query.AddingOrderBy(testutil::OrderBy("foo", "desc")).order_bys(),
817-
(std::vector<OrderBy>{
818-
testutil::OrderBy("foo", "desc"),
819-
testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc")}));
816+
ASSERT_EQ(base_query.AddingOrderBy(testutil::OrderBy("foo", "desc"))
817+
.normalized_order_bys(),
818+
(std::vector<OrderBy>{
819+
testutil::OrderBy("foo", "desc"),
820+
testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc")}));
820821
ASSERT_EQ(base_query.AddingOrderBy(testutil::OrderBy("foo", "asc"))
821822
.AddingOrderBy(testutil::OrderBy("bar", "desc"))
822-
.order_bys(),
823+
.normalized_order_bys(),
823824
(std::vector<OrderBy>{
824825
testutil::OrderBy("foo", "asc"),
825826
testutil::OrderBy("bar", "desc"),
826827
testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc"),
827828
}));
828829
ASSERT_EQ(base_query.AddingOrderBy(testutil::OrderBy("foo", "desc"))
829830
.AddingOrderBy(testutil::OrderBy("bar", "asc"))
830-
.order_bys(),
831+
.normalized_order_bys(),
831832
(std::vector<OrderBy>{
832833
testutil::OrderBy("foo", "desc"),
833834
testutil::OrderBy("bar", "asc"),
@@ -917,6 +918,58 @@ TEST(QueryTest, MatchesAllDocuments) {
917918
EXPECT_FALSE(query.MatchesAllDocuments());
918919
}
919920

921+
TEST(QueryTest, OrderByForAggregateAndNonAggregate) {
922+
auto col = testutil::Query("coll");
923+
924+
// Build two identical queries
925+
auto query1 = col.AddingFilter(testutil::Filter("foo", ">", 1));
926+
auto query2 = col.AddingFilter(testutil::Filter("foo", ">", 1));
927+
928+
// Compute an aggregate and non-aggregate target from the queries
929+
auto aggregateTarget = query1.ToAggregateTarget();
930+
auto target = query2.ToTarget();
931+
932+
EXPECT_EQ(aggregateTarget.order_bys().size(), 0);
933+
934+
ASSERT_EQ(target.order_bys().size(), 2);
935+
EXPECT_EQ(target.order_bys()[0].direction(), Direction::Ascending);
936+
EXPECT_EQ(target.order_bys()[0].field().CanonicalString(), "foo");
937+
EXPECT_EQ(target.order_bys()[1].direction(), Direction::Ascending);
938+
EXPECT_EQ(target.order_bys()[1].field().CanonicalString(), "__name__");
939+
}
940+
941+
TEST(QueryTest, GeneratedOrderBysNotAffectedByPreviouslyMemoizedTargets) {
942+
auto col = testutil::Query("coll");
943+
944+
// Build two identical queries
945+
auto query1 = col.AddingFilter(testutil::Filter("foo", ">", 1));
946+
auto query2 = col.AddingFilter(testutil::Filter("foo", ">", 1));
947+
948+
// query1 - first to aggregate target, then to non-aggregate target
949+
auto aggregateTarget1 = query1.ToAggregateTarget();
950+
auto target1 = query1.ToTarget();
951+
952+
// query2 - first to aggregate target, then to non-aggregate target
953+
auto target2 = query2.ToTarget();
954+
auto aggregateTarget2 = query2.ToAggregateTarget();
955+
956+
EXPECT_EQ(aggregateTarget1.order_bys().size(), 0);
957+
958+
EXPECT_EQ(aggregateTarget2.order_bys().size(), 0);
959+
960+
ASSERT_EQ(target1.order_bys().size(), 2);
961+
EXPECT_EQ(target1.order_bys()[0].direction(), Direction::Ascending);
962+
EXPECT_EQ(target1.order_bys()[0].field().CanonicalString(), "foo");
963+
EXPECT_EQ(target1.order_bys()[1].direction(), Direction::Ascending);
964+
EXPECT_EQ(target1.order_bys()[1].field().CanonicalString(), "__name__");
965+
966+
ASSERT_EQ(target2.order_bys().size(), 2);
967+
EXPECT_EQ(target2.order_bys()[0].direction(), Direction::Ascending);
968+
EXPECT_EQ(target2.order_bys()[0].field().CanonicalString(), "foo");
969+
EXPECT_EQ(target2.order_bys()[1].direction(), Direction::Ascending);
970+
EXPECT_EQ(target2.order_bys()[1].field().CanonicalString(), "__name__");
971+
}
972+
920973
} // namespace core
921974
} // namespace firestore
922975
} // namespace firebase

0 commit comments

Comments
 (0)