Skip to content

Commit 6d24463

Browse files
cherylEnkiducherylEnkidu
authored
Update Limit and OrderBy filtering mechanism for Or Query (#10307)
* Update Limit and OrderBy filtering mechanism for Or Query * Remove TODO
1 parent 42c3010 commit 6d24463

File tree

12 files changed

+807
-41
lines changed

12 files changed

+807
-41
lines changed

‎Firestore/Example/Tests/Integration/API/FIRQueryTests.mm

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#import <XCTest/XCTest.h>
2020

21+
#import "Firestore/Source/API/FIRFilter+Internal.h"
2122
#import "Firestore/Source/API/FIRQuery+Internal.h"
2223

2324
#import "Firestore/Example/Tests/Util/FSTEventAccumulator.h"
@@ -29,6 +30,25 @@ @interface FIRQueryTests : FSTIntegrationTestCase
2930

3031
@implementation FIRQueryTests
3132

33+
/**
34+
* Checks that running the query while online (against the backend/emulator) results in the same
35+
* documents as running the query while offline. It also checks that both online and offline
36+
* query result is equal to the expected documents.
37+
*
38+
* @param query The query to check.
39+
* @param expectedDocs Array of document keys that are expected to match the query.
40+
*/
41+
- (void)checkOnlineAndOfflineQuery:(FIRQuery *)query matchesResult:(NSArray *)expectedDocs {
42+
FIRQuerySnapshot *docsFromServer = [self readDocumentSetForRef:query
43+
source:FIRFirestoreSourceServer];
44+
FIRQuerySnapshot *docsFromCache = [self readDocumentSetForRef:query
45+
source:FIRFirestoreSourceCache];
46+
47+
XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(docsFromServer),
48+
FIRQuerySnapshotGetIDs(docsFromCache));
49+
XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(docsFromCache), expectedDocs);
50+
}
51+
3252
- (void)testLimitQueries {
3353
FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
3454
@"a" : @{@"k" : @"a"},
@@ -804,4 +824,156 @@ - (void)testCollectionGroupQueriesWithWhereFiltersOnArbitraryDocumentIDs {
804824
XCTAssertEqualObjects(ids, (@[ @"cg-doc2" ]));
805825
}
806826

827+
- (void)testOrQueries {
828+
FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
829+
@"doc1" : @{@"a" : @1, @"b" : @0},
830+
@"doc2" : @{@"a" : @2, @"b" : @1},
831+
@"doc3" : @{@"a" : @3, @"b" : @2},
832+
@"doc4" : @{@"a" : @1, @"b" : @3},
833+
@"doc5" : @{@"a" : @1, @"b" : @1}
834+
}];
835+
836+
// Two equalities: a==1 || b==1.
837+
FIRFilter *filter1 = [FIRFilter orFilterWithFilters:@[
838+
[FIRFilter filterWhereField:@"a" isEqualTo:@1], [FIRFilter filterWhereField:@"b" isEqualTo:@1]
839+
]];
840+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter1]
841+
matchesResult:@[ @"doc1", @"doc2", @"doc4", @"doc5" ]];
842+
843+
// with one inequality: a>2 || b==1.
844+
FIRFilter *filter2 = [FIRFilter orFilterWithFilters:@[
845+
[FIRFilter filterWhereField:@"a" isGreaterThan:@2], [FIRFilter filterWhereField:@"b"
846+
isEqualTo:@1]
847+
]];
848+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter2]
849+
matchesResult:@[ @"doc5", @"doc2", @"doc3" ]];
850+
851+
// (a==1 && b==0) || (a==3 && b==2)
852+
FIRFilter *filter3 = [FIRFilter orFilterWithFilters:@[
853+
[FIRFilter andFilterWithFilters:@[
854+
[FIRFilter filterWhereField:@"a" isEqualTo:@1], [FIRFilter filterWhereField:@"b" isEqualTo:@0]
855+
]],
856+
[FIRFilter andFilterWithFilters:@[
857+
[FIRFilter filterWhereField:@"a" isEqualTo:@3], [FIRFilter filterWhereField:@"b" isEqualTo:@2]
858+
]]
859+
]];
860+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter3]
861+
matchesResult:@[ @"doc1", @"doc3" ]];
862+
863+
// a==1 && (b==0 || b==3).
864+
FIRFilter *filter4 = [FIRFilter andFilterWithFilters:@[
865+
[FIRFilter filterWhereField:@"a" isEqualTo:@1], [FIRFilter orFilterWithFilters:@[
866+
[FIRFilter filterWhereField:@"b" isEqualTo:@0], [FIRFilter filterWhereField:@"b" isEqualTo:@3]
867+
]]
868+
]];
869+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter4]
870+
matchesResult:@[ @"doc1", @"doc4" ]];
871+
872+
// (a==2 || b==2) && (a==3 || b==3)
873+
FIRFilter *filter5 = [FIRFilter andFilterWithFilters:@[
874+
[FIRFilter orFilterWithFilters:@[
875+
[FIRFilter filterWhereField:@"a" isEqualTo:@2], [FIRFilter filterWhereField:@"b" isEqualTo:@2]
876+
]],
877+
[FIRFilter orFilterWithFilters:@[
878+
[FIRFilter filterWhereField:@"a" isEqualTo:@3], [FIRFilter filterWhereField:@"b" isEqualTo:@3]
879+
]]
880+
]];
881+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter5] matchesResult:@[ @"doc3" ]];
882+
883+
// Test with limits (implicit order by ASC): (a==1) || (b > 0) LIMIT 2
884+
FIRFilter *filter6 = [FIRFilter orFilterWithFilters:@[
885+
[FIRFilter filterWhereField:@"a" isEqualTo:@1], [FIRFilter filterWhereField:@"b"
886+
isGreaterThan:@0]
887+
]];
888+
[self checkOnlineAndOfflineQuery:[[collRef queryWhereFilter:filter6] queryLimitedTo:2]
889+
matchesResult:@[ @"doc1", @"doc2" ]];
890+
891+
// Test with limits (explicit order by): (a==1) || (b > 0) LIMIT_TO_LAST 2
892+
// Note: The public query API does not allow implicit ordering when limitToLast is used.
893+
FIRFilter *filter7 = [FIRFilter orFilterWithFilters:@[
894+
[FIRFilter filterWhereField:@"a" isEqualTo:@1], [FIRFilter filterWhereField:@"b"
895+
isGreaterThan:@0]
896+
]];
897+
[self checkOnlineAndOfflineQuery:[[[collRef queryWhereFilter:filter7] queryLimitedToLast:2]
898+
queryOrderedByField:@"b"]
899+
matchesResult:@[ @"doc3", @"doc4" ]];
900+
901+
// Test with limits (explicit order by ASC): (a==2) || (b == 1) ORDER BY a LIMIT 1
902+
FIRFilter *filter8 = [FIRFilter orFilterWithFilters:@[
903+
[FIRFilter filterWhereField:@"a" isEqualTo:@2], [FIRFilter filterWhereField:@"b" isEqualTo:@1]
904+
]];
905+
[self checkOnlineAndOfflineQuery:[[[collRef queryWhereFilter:filter8] queryLimitedTo:1]
906+
queryOrderedByField:@"a"]
907+
matchesResult:@[ @"doc5" ]];
908+
909+
// Test with limits (explicit order by DESC): (a==2) || (b == 1) ORDER BY a LIMIT_TO_LAST 1
910+
FIRFilter *filter9 = [FIRFilter orFilterWithFilters:@[
911+
[FIRFilter filterWhereField:@"a" isEqualTo:@2], [FIRFilter filterWhereField:@"b" isEqualTo:@1]
912+
]];
913+
[self checkOnlineAndOfflineQuery:[[[collRef queryWhereFilter:filter9] queryLimitedToLast:1]
914+
queryOrderedByField:@"a"]
915+
matchesResult:@[ @"doc2" ]];
916+
917+
// Test with limits without orderBy (the __name__ ordering is the tie breaker).
918+
FIRFilter *filter10 = [FIRFilter orFilterWithFilters:@[
919+
[FIRFilter filterWhereField:@"a" isEqualTo:@2], [FIRFilter filterWhereField:@"b" isEqualTo:@1]
920+
]];
921+
[self checkOnlineAndOfflineQuery:[[collRef queryWhereFilter:filter10] queryLimitedTo:1]
922+
matchesResult:@[ @"doc2" ]];
923+
}
924+
925+
- (void)testOrQueriesWithInAndNotIn {
926+
FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
927+
@"doc1" : @{@"a" : @1, @"b" : @0},
928+
@"doc2" : @{@"b" : @1},
929+
@"doc3" : @{@"a" : @3, @"b" : @2},
930+
@"doc4" : @{@"a" : @1, @"b" : @3},
931+
@"doc5" : @{@"a" : @1},
932+
@"doc6" : @{@"a" : @2}
933+
}];
934+
935+
// a==2 || b in [2,3]
936+
FIRFilter *filter1 = [FIRFilter orFilterWithFilters:@[
937+
[FIRFilter filterWhereField:@"a" isEqualTo:@2], [FIRFilter filterWhereField:@"b" in:@[ @2, @3 ]]
938+
]];
939+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter1]
940+
matchesResult:@[ @"doc3", @"doc4", @"doc6" ]];
941+
942+
// a==2 || b not-in [2,3]
943+
// Has implicit orderBy b.
944+
FIRFilter *filter2 = [FIRFilter orFilterWithFilters:@[
945+
[FIRFilter filterWhereField:@"a" isEqualTo:@2], [FIRFilter filterWhereField:@"b"
946+
notIn:@[ @2, @3 ]]
947+
]];
948+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter2]
949+
matchesResult:@[ @"doc1", @"doc2" ]];
950+
}
951+
952+
- (void)testOrQueriesWithArrayMembership {
953+
FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
954+
@"doc1" : @{@"a" : @1, @"b" : @[ @0 ]},
955+
@"doc2" : @{@"b" : @[ @1 ]},
956+
@"doc3" : @{@"a" : @3, @"b" : @[ @2, @7 ]},
957+
@"doc4" : @{@"a" : @1, @"b" : @[ @3, @7 ]},
958+
@"doc5" : @{@"a" : @1},
959+
@"doc6" : @{@"a" : @2}
960+
}];
961+
962+
// a==2 || b array-contains 7
963+
FIRFilter *filter1 = [FIRFilter orFilterWithFilters:@[
964+
[FIRFilter filterWhereField:@"a" isEqualTo:@2], [FIRFilter filterWhereField:@"b"
965+
arrayContains:@7]
966+
]];
967+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter1]
968+
matchesResult:@[ @"doc3", @"doc4", @"doc6" ]];
969+
970+
// a==2 || b array-contains-any [0, 3]
971+
FIRFilter *filter2 = [FIRFilter orFilterWithFilters:@[
972+
[FIRFilter filterWhereField:@"a" isEqualTo:@2], [FIRFilter filterWhereField:@"b"
973+
arrayContainsAny:@[ @0, @3 ]]
974+
]];
975+
[self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter2]
976+
matchesResult:@[ @"doc1", @"doc4", @"doc6" ]];
977+
}
978+
807979
@end

‎Firestore/Example/Tests/Integration/API/FIRValidationTests.mm

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,14 @@
1717
#import <FirebaseFirestore/FirebaseFirestore.h>
1818

1919
#import <XCTest/XCTest.h>
20-
2120
#include <limits>
2221

2322
#import "FirebaseCore/Extension/FIROptionsInternal.h"
24-
#import "Firestore/Source/API/FIRFieldValue+Internal.h"
25-
#import "Firestore/Source/API/FIRQuery+Internal.h"
26-
2723
#import "Firestore/Example/Tests/Util/FSTHelpers.h"
2824
#import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h"
29-
25+
#import "Firestore/Source/API/FIRFieldValue+Internal.h"
3026
#import "Firestore/Source/API/FIRFilter+Internal.h"
27+
#import "Firestore/Source/API/FIRQuery+Internal.h"
3128
#include "Firestore/core/test/unit/testutil/app_testing.h"
3229

3330
using firebase::firestore::testutil::AppForUnitTesting;

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

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,14 @@ Query Query::AddingFilter(Filter filter) const {
172172
explicit_order_bys_[0].field() == *new_inequality_field,
173173
"First orderBy must match inequality field");
174174

175-
return Query(path_, collection_group_, filters_.push_back(std::move(filter)),
176-
explicit_order_bys_, limit_, limit_type_, start_at_, end_at_);
175+
return {path_,
176+
collection_group_,
177+
filters_.push_back(std::move(filter)),
178+
explicit_order_bys_,
179+
limit_,
180+
limit_type_,
181+
start_at_,
182+
end_at_};
177183
}
178184

179185
Query Query::AddingOrderBy(OrderBy order_by) const {
@@ -185,34 +191,37 @@ Query Query::AddingOrderBy(OrderBy order_by) const {
185191
"First OrderBy must match inequality field.");
186192
}
187193

188-
return Query(path_, collection_group_, filters_,
189-
explicit_order_bys_.push_back(std::move(order_by)), limit_,
190-
limit_type_, start_at_, end_at_);
194+
return {path_, collection_group_,
195+
filters_, explicit_order_bys_.push_back(std::move(order_by)),
196+
limit_, limit_type_,
197+
start_at_, end_at_};
191198
}
192199

193200
Query Query::WithLimitToFirst(int32_t limit) const {
194-
return Query(path_, collection_group_, filters_, explicit_order_bys_, limit,
195-
LimitType::First, start_at_, end_at_);
201+
return {path_, collection_group_, filters_, explicit_order_bys_,
202+
limit, LimitType::First, start_at_, end_at_};
196203
}
197204

198205
Query Query::WithLimitToLast(int32_t limit) const {
199-
return Query(path_, collection_group_, filters_, explicit_order_bys_, limit,
200-
LimitType::Last, start_at_, end_at_);
206+
return {path_, collection_group_, filters_, explicit_order_bys_,
207+
limit, LimitType::Last, start_at_, end_at_};
201208
}
202209

203210
Query Query::StartingAt(Bound bound) const {
204-
return Query(path_, collection_group_, filters_, explicit_order_bys_, limit_,
205-
limit_type_, std::move(bound), end_at_);
211+
return {path_, collection_group_, filters_, explicit_order_bys_,
212+
limit_, limit_type_, std::move(bound), end_at_};
206213
}
207214

208215
Query Query::EndingAt(Bound bound) const {
209-
return Query(path_, collection_group_, filters_, explicit_order_bys_, limit_,
210-
limit_type_, start_at_, std::move(bound));
216+
return {path_, collection_group_, filters_, explicit_order_bys_,
217+
limit_, limit_type_, start_at_, std::move(bound)};
211218
}
212219

213220
Query Query::AsCollectionQueryAtPath(ResourcePath path) const {
214-
return Query(path, /*collection_group=*/nullptr, filters_,
215-
explicit_order_bys_, limit_, limit_type_, start_at_, end_at_);
221+
return {std::move(path), /*collection_group=*/nullptr,
222+
filters_, explicit_order_bys_,
223+
limit_, limit_type_,
224+
start_at_, end_at_};
216225
}
217226

218227
// MARK: - Matching
@@ -246,7 +255,14 @@ bool Query::MatchesFilters(const Document& doc) const {
246255
}
247256

248257
bool Query::MatchesOrderBy(const Document& doc) const {
249-
for (const OrderBy& order_by : explicit_order_bys_) {
258+
// We must use `order_bys()` to get the list of all orderBys
259+
// (both implicit and explicit). Note that for OR queries, orderBy applies to
260+
// all disjunction terms and implicit orderBys must be taken into account.
261+
// For example, the query "a > 1 || b == 1" has an implicit "orderBy a" due
262+
// to the inequality, and is evaluated as "a > 1 orderBy a || b == 1 orderBy
263+
// a". A document with content of {b:1} matches the filters, but does not
264+
// match the orderBy because it's missing the field 'a'.
265+
for (const OrderBy& order_by : order_bys()) {
250266
const FieldPath& field_path = order_by.field();
251267
// order by key always matches
252268
if (field_path != FieldPath::KeyFieldPath() &&

‎Firestore/core/src/core/target.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,10 @@ class Target {
9999
return limit_;
100100
}
101101

102+
bool HasLimit() const {
103+
return limit_ != kNoLimit;
104+
}
105+
102106
const absl::optional<Bound>& start_at() const {
103107
return start_at_;
104108
}

‎Firestore/core/src/local/index_manager.h

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -112,13 +112,6 @@ class IndexManager {
112112
/** Returns all configured field indexes. */
113113
virtual std::vector<model::FieldIndex> GetFieldIndexes() const = 0;
114114

115-
/**
116-
* Returns an index that can be used to serve the provided target. Returns
117-
* `nullopt` if no index is configured.
118-
*/
119-
virtual absl::optional<model::FieldIndex> GetFieldIndex(
120-
const core::Target& target) const = 0;
121-
122115
/**
123116
* Iterates over all field indexes that are used to serve the given target,
124117
* and returns the minimum offset of them all. Asserts that the target can be

‎Firestore/core/src/local/leveldb_index_manager.cc

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -508,7 +508,9 @@ model::IndexOffset LevelDbIndexManager::GetMinOffset(
508508
IndexManager::IndexType LevelDbIndexManager::GetIndexType(
509509
const core::Target& target) {
510510
IndexManager::IndexType result = IndexManager::IndexType::FULL;
511-
for (const Target& sub_target : GetSubTargets(target)) {
511+
const auto sub_targets = GetSubTargets(target);
512+
513+
for (const Target& sub_target : sub_targets) {
512514
absl::optional<model::FieldIndex> index = GetFieldIndex(sub_target);
513515
if (!index) {
514516
result = IndexManager::IndexType::NONE;
@@ -519,6 +521,16 @@ IndexManager::IndexType LevelDbIndexManager::GetIndexType(
519521
result = IndexManager::IndexType::PARTIAL;
520522
}
521523
}
524+
525+
// OR queries have more than one sub-target (one sub-target per DNF term).
526+
// We currently consider OR queries that have a `limit` to have a partial
527+
// index. For such queries we perform sorting and apply the limit in memory as
528+
// a post-processing step.
529+
if (target.HasLimit() && sub_targets.size() > 1U &&
530+
result == IndexManager::IndexType::FULL) {
531+
result = IndexManager::IndexType::PARTIAL;
532+
}
533+
522534
return result;
523535
}
524536

‎Firestore/core/src/local/leveldb_index_manager.h

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,6 @@ class LevelDbIndexManager : public IndexManager {
6969

7070
std::vector<model::FieldIndex> GetFieldIndexes() const override;
7171

72-
absl::optional<model::FieldIndex> GetFieldIndex(
73-
const core::Target& target) const override;
74-
7572
model::IndexOffset GetMinOffset(const core::Target& target) override;
7673

7774
model::IndexOffset GetMinOffset(
@@ -201,6 +198,13 @@ class LevelDbIndexManager : public IndexManager {
201198
const index::IndexEntry& upper_bound,
202199
std::vector<index::IndexEntry> not_in_bounds) const;
203200

201+
/**
202+
* Returns an index that can be used to serve the provided target. Returns
203+
* `nullopt` if no index is configured.
204+
*/
205+
absl::optional<model::FieldIndex> GetFieldIndex(
206+
const core::Target& target) const;
207+
204208
// The LevelDbIndexManager is owned by LevelDbPersistence.
205209
LevelDbPersistence* db_;
206210

‎Firestore/core/src/local/memory_index_manager.cc

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,6 @@ std::vector<model::FieldIndex> MemoryIndexManager::GetFieldIndexes() const {
8989
return {};
9090
}
9191

92-
absl::optional<model::FieldIndex> MemoryIndexManager::GetFieldIndex(
93-
const core::Target& target) const {
94-
(void)target;
95-
return absl::nullopt;
96-
}
97-
9892
model::IndexOffset MemoryIndexManager::GetMinOffset(const core::Target&) {
9993
return model::IndexOffset::None();
10094
}

‎Firestore/core/src/local/memory_index_manager.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,6 @@ class MemoryIndexManager : public IndexManager {
6767

6868
std::vector<model::FieldIndex> GetFieldIndexes() const override;
6969

70-
absl::optional<model::FieldIndex> GetFieldIndex(
71-
const core::Target& target) const override;
72-
7370
model::IndexOffset GetMinOffset(const core::Target&) override;
7471

7572
model::IndexOffset GetMinOffset(const std::string&) const override;

0 commit comments

Comments
 (0)