From 53eb2f830b799aee0f6a6b5e11d21903892c5abf Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 27 Jul 2023 11:59:48 -0400 Subject: [PATCH 01/46] initial code --- .../Integration/API/FIRValidationTests.mm | 62 ++++------- Firestore/core/src/api/query_core.cc | 45 -------- Firestore/core/src/api/query_core.h | 3 - Firestore/core/src/core/composite_filter.cc | 12 +++ Firestore/core/src/core/composite_filter.h | 1 + Firestore/core/src/core/field_filter.cc | 10 ++ Firestore/core/src/core/field_filter.h | 1 + Firestore/core/src/core/filter.h | 12 +++ Firestore/core/src/core/query.cc | 102 ++++++++---------- Firestore/core/src/core/query.h | 2 + .../core/src/model/target_index_matcher.cc | 5 +- 11 files changed, 108 insertions(+), 147 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm index 4e12795b540..e5c086f4f4a 100644 --- a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm @@ -623,14 +623,11 @@ - (void)testQueriesUsingInAndDocumentIdMustHaveProperDocumentReferencesInArray { reason); } -- (void)testInvalidQueryFilters { - FIRCollectionReference *collection = [self collectionRef]; +/** Multiple Inequality tests*/ +- (void)testMultipleInequalityOnDifferentFields { + FIRCollectionReference *collection = [self collectionRef]; // Multiple inequalities, one of which is inside a nested composite filter. - NSString *reason = @"Invalid Query. All where filters with an inequality (notEqual, lessThan, " - "lessThanOrEqual, greaterThan, or greaterThanOrEqual) must be on the same " - "field. But you have inequality filters on 'c' and 'r'"; - NSArray *array1 = @[ [FIRFilter andFilterWithFilters:@[ [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c" @@ -642,23 +639,15 @@ - (void)testInvalidQueryFilters { ]] ]; - FSTAssertThrows( - [[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]] queryWhereField:@"r" - isGreaterThan:@"s"], - reason); + XCTAssertNoThrow([[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]] + queryWhereField:@"r" + isGreaterThan:@"s"]); - // OrderBy and inequality on different fields. Inequality inside a nested composite filter. - reason = @"Invalid query. You have a where filter with an inequality (notEqual, lessThan, " - "lessThanOrEqual, greaterThan, or greaterThanOrEqual) on field 'c' and so you must " - "also use 'c' as your first queryOrderedBy field, but your first queryOrderedBy is " - "currently on field 'r' instead."; - - FSTAssertThrows([[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]] - queryOrderedByField:@"r"], - reason); + XCTAssertNoThrow([[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]] + queryOrderedByField:@"r"], ); // Conflicting operations within a composite filter. - reason = @"Invalid Query. You cannot use 'notIn' filters with 'in' filters."; + NSString *reason = @"Invalid Query. You cannot use 'notIn' filters with 'in' filters."; NSArray *array2 = @[ [FIRFilter andFilterWithFilters:@[ @@ -672,6 +661,11 @@ - (void)testInvalidQueryFilters { ]; FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array2]], reason); +} + +- (void)testInvalidQueryFilters { + FIRCollectionReference *collection = [self collectionRef]; + NSString *reason = @"Invalid Query. You cannot use 'notIn' filters with 'in' filters."; // Conflicting operations between a field filter and a composite filter. NSArray *array3 = @[ @@ -713,23 +707,15 @@ - (void)testQueryInequalityFieldMustMatchFirstOrderByField { FIRCollectionReference *coll = [self.db collectionWithPath:@"collection"]; FIRQuery *base = [coll queryWhereField:@"x" isGreaterThanOrEqualTo:@32]; - FSTAssertThrows([base queryWhereField:@"y" isLessThan:@"cat"], - @"Invalid Query. All where filters with an inequality (notEqual, lessThan, " - "lessThanOrEqual, greaterThan, or greaterThanOrEqual) must be on the same " - "field. But you have inequality filters on 'x' and 'y'"); + XCTAssertNoThrow([base queryWhereField:@"y" isLessThan:@"cat"]); - NSString *reason = - @"Invalid query. You have a where filter with " - "an inequality (notEqual, lessThan, lessThanOrEqual, greaterThan, or greaterThanOrEqual) " - "on field 'x' and so you must also use 'x' as your first queryOrderedBy field, " - "but your first queryOrderedBy is currently on field 'y' instead."; - FSTAssertThrows([base queryOrderedByField:@"y"], reason); - FSTAssertThrows([[coll queryOrderedByField:@"y"] queryWhereField:@"x" isGreaterThan:@32], reason); - FSTAssertThrows([[base queryOrderedByField:@"y"] queryOrderedByField:@"x"], reason); - FSTAssertThrows([[[coll queryOrderedByField:@"y"] queryOrderedByField:@"x"] queryWhereField:@"x" - isGreaterThan:@32], - reason); - FSTAssertThrows([[coll queryOrderedByField:@"y"] queryWhereField:@"x" isNotEqualTo:@32], reason); + XCTAssertNoThrow([base queryOrderedByField:@"y"]); + XCTAssertNoThrow([[coll queryOrderedByField:@"y"] queryWhereField:@"x" isGreaterThan:@32]); + XCTAssertNoThrow([[base queryOrderedByField:@"y"] queryOrderedByField:@"x"]); + XCTAssertNoThrow([[[coll queryOrderedByField:@"y"] queryOrderedByField:@"x"] + queryWhereField:@"x" + isGreaterThan:@32]); + XCTAssertNoThrow([[coll queryOrderedByField:@"y"] queryWhereField:@"x" isNotEqualTo:@32]); XCTAssertNoThrow([base queryWhereField:@"x" isLessThanOrEqualTo:@"cat"], @"Same inequality fields work"); @@ -766,9 +752,7 @@ - (void)testQueriesWithMultipleNotEqualAndInequalitiesFail { FSTAssertThrows([[coll queryWhereField:@"x" isNotEqualTo:@1] queryWhereField:@"y" isNotEqualTo:@2], - @"Invalid Query. All where filters with an inequality (notEqual, lessThan, " - "lessThanOrEqual, greaterThan, or greaterThanOrEqual) must be on " - "the same field. But you have inequality filters on 'x' and 'y'"); + @"Invalid Query. You cannot use more than one 'notEqual' filter."); } - (void)testQueriesWithNotEqualAndNotInFiltersFail { diff --git a/Firestore/core/src/api/query_core.cc b/Firestore/core/src/api/query_core.cc index a7e9d52f9f0..f66676dffe3 100644 --- a/Firestore/core/src/api/query_core.cc +++ b/Firestore/core/src/api/query_core.cc @@ -277,7 +277,6 @@ Query Query::OrderBy(FieldPath field_path, bool descending) const { } Query Query::OrderBy(FieldPath field_path, Direction direction) const { - ValidateNewOrderByPath(field_path); if (query_.start_at()) { ThrowInvalidArgument( "Invalid query. You must not specify a starting point " @@ -320,26 +319,6 @@ Query Query::EndAt(Bound bound) const { void Query::ValidateNewFieldFilter(const core::Query& query, const FieldFilter& field_filter) const { - if (field_filter.IsInequality()) { - const FieldPath* existing_inequality = query.InequalityFilterField(); - const FieldPath& new_inequality = field_filter.field(); - - if (existing_inequality && *existing_inequality != new_inequality) { - ThrowInvalidArgument( - "Invalid Query. All where filters with an inequality (notEqual, " - "lessThan, lessThanOrEqual, greaterThan, or greaterThanOrEqual) " - "must be on the same field. But you have inequality filters on " - "'%s' and '%s'", - existing_inequality->CanonicalString(), - new_inequality.CanonicalString()); - } - - const FieldPath* first_order_by_field = query.FirstOrderByField(); - if (first_order_by_field) { - ValidateOrderByField(*first_order_by_field, field_filter.field()); - } - } - Operator filter_op = field_filter.op(); absl::optional conflicting_op = query.FindOpInsideFilters(ConflictingOps(filter_op)); @@ -367,30 +346,6 @@ void Query::ValidateNewFilter(const Filter& filter) const { } } -void Query::ValidateNewOrderByPath(const FieldPath& field_path) const { - if (!query_.FirstOrderByField()) { - // This is the first order by. It must match any inequality. - const FieldPath* inequality_field = query_.InequalityFilterField(); - if (inequality_field) { - ValidateOrderByField(field_path, *inequality_field); - } - } -} - -void Query::ValidateOrderByField(const FieldPath& order_by_field, - const FieldPath& inequality_field) const { - if (order_by_field != inequality_field) { - ThrowInvalidArgument( - "Invalid query. You have a where filter with an inequality " - "(notEqual, lessThan, lessThanOrEqual, greaterThan, or " - "greaterThanOrEqual) on field '%s' and so you must also use '%s' as " - "your first queryOrderedBy field, but your first queryOrderedBy is " - "currently on field '%s' instead.", - inequality_field.CanonicalString(), inequality_field.CanonicalString(), - order_by_field.CanonicalString()); - } -} - void Query::ValidateHasExplicitOrderByForLimitToLast() const { if (query_.has_limit_to_last() && query_.explicit_order_bys().empty()) { ThrowInvalidArgument( diff --git a/Firestore/core/src/api/query_core.h b/Firestore/core/src/api/query_core.h index b85238a7eea..f6588cc7927 100644 --- a/Firestore/core/src/api/query_core.h +++ b/Firestore/core/src/api/query_core.h @@ -215,9 +215,6 @@ class Query { void ValidateNewFilter(const core::Filter& filter) const; void ValidateNewFieldFilter(const core::Query& query, const core::FieldFilter& filter) const; - void ValidateNewOrderByPath(const model::FieldPath& field_path) const; - void ValidateOrderByField(const model::FieldPath& order_by_field, - const model::FieldPath& inequality_field) const; void ValidateHasExplicitOrderByForLimitToLast() const; /** * Validates that the value passed into a disjunctive filter satisfies all diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index 6b6ac7607b3..38a398c5e9b 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -152,6 +152,18 @@ const model::FieldPath* CompositeFilter::Rep::GetFirstInequalityField() const { return nullptr; } +const std::vector& CompositeFilter::Rep::GetInequalityFilters() + const { + if (Filter::Rep::memoized_inequality_filters_.empty()) { + for (const auto& filter : GetFlattenedFilters()) { + std::copy(filter.GetInequalityFilters().begin(), + filter.GetInequalityFilters().end(), + std::back_inserter(Filter::Rep::memoized_inequality_filters_)); + } + } + return Filter::Rep::memoized_inequality_filters_; +} + const std::vector& CompositeFilter::Rep::GetFlattenedFilters() const { if (Filter::Rep::memoized_flattened_filters_.empty() && !filters().empty()) { diff --git a/Firestore/core/src/core/composite_filter.h b/Firestore/core/src/core/composite_filter.h index d16975fdac2..0031fbae8bf 100644 --- a/Firestore/core/src/core/composite_filter.h +++ b/Firestore/core/src/core/composite_filter.h @@ -139,6 +139,7 @@ class CompositeFilter : public Filter { } const std::vector& GetFlattenedFilters() const override; + const std::vector& GetInequalityFilters() const override; const model::FieldPath* GetFirstInequalityField() const override; diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index b6686465501..a5077d99464 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -210,6 +210,16 @@ const model::FieldPath* FieldFilter::Rep::GetFirstInequalityField() const { return nullptr; } +const std::vector& FieldFilter::Rep::GetInequalityFilters() const { + // This is already a field filter, so we return a vector of size one. + if (IsInequality() && Filter::Rep::memoized_inequality_filters_.empty()) { + Filter::Rep::memoized_inequality_filters_ = std::vector{ + FieldFilter(std::make_shared(*this))}; + } + + return Filter::Rep::memoized_inequality_filters_; +} + } // namespace core } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/core/field_filter.h b/Firestore/core/src/core/field_filter.h index a0fb692e44d..2b8f02d9a5c 100644 --- a/Firestore/core/src/core/field_filter.h +++ b/Firestore/core/src/core/field_filter.h @@ -118,6 +118,7 @@ class FieldFilter : public Filter { } const model::FieldPath* GetFirstInequalityField() const override; + const std::vector& GetInequalityFilters() const override; const std::vector& GetFlattenedFilters() const override; diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 1785ff5a9d0..bfaded237e4 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -103,6 +103,14 @@ class Filter { return rep_->GetFirstInequalityField(); } + /** + * Returns the first inequality filter contained within this filter. + * Returns empty vector if it does not contain any inequalities. + */ + const std::vector& GetInequalityFilters() const { + return rep_->GetInequalityFilters(); + } + /** * Returns a list of all field filters that are contained within this filter. */ @@ -154,6 +162,8 @@ class Filter { virtual bool IsEmpty() const = 0; virtual const model::FieldPath* GetFirstInequalityField() const = 0; + virtual const std::vector& GetInequalityFilters() const = 0; + // virtual const std::vector GetInequalityFilters() const = 0; virtual const std::vector& GetFlattenedFilters() const = 0; @@ -164,6 +174,8 @@ class Filter { * traversing the tree of filters contained in this composite filter. */ mutable std::vector memoized_flattened_filters_; + + mutable std::vector memoized_inequality_filters_; }; explicit Filter(std::shared_ptr&& rep) : rep_(rep) { diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index 890411eb095..f006b3f1712 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -77,6 +77,21 @@ const FieldPath* Query::InequalityFilterField() const { return nullptr; } +const std::set Query::InequalityFilterFields() const { + std::set result; + for (const Filter& filter : filters_) { + const std::vector& inequalityFilters = + filter.GetInequalityFilters(); + for (const FieldFilter& subFilter : inequalityFilters) { + { + result.insert(subFilter.field()); + } + } + } + + return result; +} + absl::optional Query::FindOpInsideFilters( const std::vector& ops) const { for (const auto& filter : filters_) { @@ -91,51 +106,42 @@ absl::optional Query::FindOpInsideFilters( const std::vector& Query::order_bys() const { if (memoized_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_ = { - OrderBy(FieldPath::KeyFieldPath(), Direction::Ascending), - }; - } else { - memoized_order_bys_ = { - OrderBy(*inequality_field, Direction::Ascending), - OrderBy(FieldPath::KeyFieldPath(), Direction::Ascending), - }; - } - } else { - HARD_ASSERT( - !inequality_field || *inequality_field == *first_order_by_field, - "First orderBy %s should match inequality field %s.", - first_order_by_field->CanonicalString(), - inequality_field->CanonicalString()); - - std::vector result = explicit_order_bys_; - - bool found_explicit_key_order = false; - for (const OrderBy& order_by : explicit_order_bys_) { - if (order_by.field().IsKeyFieldPath()) { - found_explicit_key_order = true; - break; - } - } + // Any explicit order by fields should be added as is. + std::vector result = explicit_order_bys_; + std::set fieldsNormalized; + for (const OrderBy& order_by : explicit_order_bys_) { + fieldsNormalized.insert(order_by.field()); + } - if (!found_explicit_key_order) { - // The direction of the implicit key ordering always matches the - // direction of the last explicit sort order - Direction last_direction = explicit_order_bys_.empty() - ? Direction::Ascending - : explicit_order_bys_.back().direction(); - result.emplace_back(FieldPath::KeyFieldPath(), last_direction); + // The direction of the implicit key ordering always matches the + // direction of the last explicit sort order + Direction last_direction = explicit_order_bys_.empty() + ? Direction::Ascending + : explicit_order_bys_.back().direction(); + + // Any inequality fields not explicitly ordered should be implicitly ordered + // in a lexicographical order. When there are multiple inequality filters on + // the same field, the field should be added only once. Note: + // `SortedSet` sorts the key field before other fields. However, + // we want the key field to be sorted last. + const std::set inequality_fields = + InequalityFilterFields(); + + for (const model::FieldPath& field : inequality_fields) { + if (fieldsNormalized.find(field) == fieldsNormalized.end() && + !field.IsKeyFieldPath()) { + result.push_back(OrderBy(field, last_direction)); } + } - memoized_order_bys_ = std::move(result); + if (fieldsNormalized.find(FieldPath::KeyFieldPath()) == + fieldsNormalized.end()) { + result.emplace_back(FieldPath::KeyFieldPath(), last_direction); } + + memoized_order_bys_ = std::move(result); } + return memoized_order_bys_; } @@ -162,16 +168,6 @@ int32_t Query::limit() const { Query Query::AddingFilter(Filter filter) const { HARD_ASSERT(!IsDocumentQuery(), "No filter is allowed for document query"); - const FieldPath* new_inequality_field = filter.GetFirstInequalityField(); - const FieldPath* query_inequality_field = InequalityFilterField(); - HARD_ASSERT(!query_inequality_field || !new_inequality_field || - *query_inequality_field == *new_inequality_field, - "Query must only have one inequality field."); - - HARD_ASSERT(explicit_order_bys_.empty() || !new_inequality_field || - explicit_order_bys_[0].field() == *new_inequality_field, - "First orderBy must match inequality field"); - std::vector filters_copy(filters_); filters_copy.push_back(std::move(filter)); @@ -188,12 +184,6 @@ Query Query::AddingFilter(Filter filter) const { Query Query::AddingOrderBy(OrderBy order_by) const { HARD_ASSERT(!IsDocumentQuery(), "No ordering is allowed for document query"); - if (explicit_order_bys_.empty()) { - const FieldPath* inequality = InequalityFilterField(); - HARD_ASSERT(inequality == nullptr || *inequality == order_by.field(), - "First OrderBy must match inequality field."); - } - std::vector order_bys_copy(explicit_order_bys_); order_bys_copy.push_back(std::move(order_by)); diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index 6ecf305ffa3..c7402c9a3be 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -117,6 +117,8 @@ class Query { */ const model::FieldPath* InequalityFilterField() const; + const std::set InequalityFilterFields() const; + /** * Checks if any of the provided filter operators are included in the query * and returns the first one that is, or null if none are. diff --git a/Firestore/core/src/model/target_index_matcher.cc b/Firestore/core/src/model/target_index_matcher.cc index 62491c539ae..d17baf6417a 100644 --- a/Firestore/core/src/model/target_index_matcher.cc +++ b/Firestore/core/src/model/target_index_matcher.cc @@ -38,10 +38,7 @@ TargetIndexMatcher::TargetIndexMatcher(const core::Target& target) { for (const Filter& filter : target.filters()) { FieldFilter field_filter(filter); - if (field_filter.IsInequality()) { - HARD_ASSERT(!inequality_filter_.has_value() || - inequality_filter_->field() == field_filter.field(), - "Only a single inequality is supported"); + if (field_filter.IsInequality() && !inequality_filter_.has_value()) { inequality_filter_ = field_filter; } else { equality_filters_.push_back(field_filter); From 95850ffaa0932ab2150fcd92420debd933db09cd Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 27 Jul 2023 17:35:20 -0400 Subject: [PATCH 02/46] add integration tests --- .../Tests/Integration/API/FIRQueryTests.mm | 509 ++++++++++++++++++ 1 file changed, 509 insertions(+) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 4923299b83d..a52c26d0649 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -24,6 +24,9 @@ #include "Firestore/core/test/unit/testutil/testing_hooks_util.h" +#import "Firestore/Source/API/FIRAggregateQuerySnapshot+Internal.h" +#import "Firestore/Source/API/FIRQuery+Internal.h" + namespace { NSArray *SortedStringsNotIn(NSSet *set, NSSet *remove) { @@ -1192,6 +1195,512 @@ - (void)testOrderByEquality { matchesResult:@[ @"doc6", @"doc3" ]]; } +// Multiple Inequality +- (void)testMultipleInequalityOnDifferentFields { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @0, @"v" : @0}, + @"doc2" : @{@"key" : @"b", @"sort" : @3, @"v" : @1}, + @"doc3" : @{@"key" : @"c", @"sort" : @1, @"v" : @3}, + @"doc4" : @{@"key" : @"d", @"sort" : @2, @"v" : @2} + }]; + + // Multiple inequality fields + FIRQuerySnapshot *snapshot = [self + readDocumentSetForRef:[[[collRef queryWhereField:@"key" isNotEqualTo:@"a"] + queryWhereField:@"sort" + isLessThanOrEqualTo:@2] queryWhereField:@"v" isGreaterThan:@2]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc3" ])); + + // Duplicate inequality fields + snapshot = [self readDocumentSetForRef:[[[collRef queryWhereField:@"key" isNotEqualTo:@"a"] + queryWhereField:@"sort" + isLessThanOrEqualTo:@2] queryWhereField:@"sort" + isGreaterThan:@1]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc4" ])); + + // With multiple IN + snapshot = [self + readDocumentSetForRef:[[[[collRef queryWhereField:@"key" + isGreaterThanOrEqualTo:@"a"] queryWhereField:@"sort" + isLessThanOrEqualTo:@2] + queryWhereField:@"v" + in:@[ @2, @3, @4 ]] queryWhereField:@"sort" + in:@[ @2, @3 ]]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc4" ])); + + // With NOT-IN + snapshot = [self readDocumentSetForRef:[[[collRef queryWhereField:@"key" + isGreaterThanOrEqualTo:@"a"] queryWhereField:@"sort" + isLessThanOrEqualTo:@2] + queryWhereField:@"v" + notIn:@[ @2, @4, @5 ]]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc1", @"doc3" ])); + + // With orderby + snapshot = [self readDocumentSetForRef:[[[collRef queryWhereField:@"key" + isGreaterThanOrEqualTo:@"a"] queryWhereField:@"sort" + isLessThanOrEqualTo:@2] + queryOrderedByField:@"v" + descending:YES]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc3", @"doc4", @"doc1" ])); + + // With limit + snapshot = [self readDocumentSetForRef:[[[[collRef queryWhereField:@"key" + isGreaterThanOrEqualTo:@"a"] queryWhereField:@"sort" + isLessThanOrEqualTo:@2] + queryOrderedByField:@"v" + descending:YES] queryLimitedTo:2]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc3", @"doc4" ])); + + // With limitedToLast + snapshot = [self readDocumentSetForRef:[[[[collRef queryWhereField:@"key" + isGreaterThanOrEqualTo:@"a"] queryWhereField:@"sort" + isLessThanOrEqualTo:@2] + queryOrderedByField:@"v" + descending:YES] queryLimitedToLast:2]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc4", @"doc1" ])); +} + +- (void)testMultipleInequalityOnUnaryValues { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @0, @"v" : @0}, + @"doc2" : @{@"key" : @"b", @"sort" : @(NAN), @"v" : @1}, + @"doc3" : @{@"key" : @"c", @"sort" : [NSNull null], @"v" : @3}, + @"doc4" : @{@"key" : @"d", @"v" : @0}, + @"doc5" : @{@"key" : @"e", @"sort" : @1}, + @"doc6" : @{@"key" : @"f", @"sort" : @1, @"v" : @1} + }]; + + FIRQuerySnapshot *snapshot = + [self readDocumentSetForRef:[[collRef queryWhereField:@"key" + isNotEqualTo:@"a"] queryWhereField:@"sort" + isLessThanOrEqualTo:@2]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc5", @"doc6" ])); + + snapshot = [self readDocumentSetForRef:[[[collRef queryWhereField:@"key" isNotEqualTo:@"a"] + queryWhereField:@"sort" + isLessThanOrEqualTo:@2] queryWhereField:@"v" + isLessThanOrEqualTo:@1]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc6" ])); +} +- (void)testMultipleInequalityWithArrayMembership { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @0, @"v" : @[ @0 ]}, + @"doc2" : @{@"key" : @"b", @"sort" : @1, @"v" : @[ @0, @1, @3 ]}, + @"doc3" : @{@"key" : @"c", @"sort" : @1, @"v" : @[]}, + @"doc4" : @{@"key" : @"d", @"sort" : @2, @"v" : @[ @1 ]}, + @"doc5" : @{@"key" : @"e", @"sort" : @3, @"v" : @[ @2, @4 ]}, + @"doc6" : @{@"key" : @"f", @"sort" : @4, @"v" : @[ @(NAN) ]}, + @"doc7" : @{@"key" : @"g", @"sort" : @4, @"v" : @[ [NSNull null] ]} + + }]; + + FIRQuerySnapshot *snapshot = [self + readDocumentSetForRef:[[[collRef queryWhereField:@"key" isNotEqualTo:@"a"] + queryWhereField:@"sort" + isGreaterThanOrEqualTo:@1] queryWhereField:@"v" arrayContains:@0]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc2" ])); + + snapshot = [self readDocumentSetForRef:[[[collRef queryWhereField:@"key" + isNotEqualTo:@"a"] queryWhereField:@"sort" + isGreaterThanOrEqualTo:@1] + queryWhereField:@"v" + arrayContainsAny:@[ @0, @1 ]]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc2", @"doc4" ])); +} + +- (NSDictionary *)nestedData:(int)number { + return @{ + @"name" : [NSString stringWithFormat:@"room %d", number], + @"metadata" : @{@"createdAt" : @(number)}, + @"field" : [NSString stringWithFormat:@"field %d", number], + @"field.dot" : @(number), + @"field\\slash" : @(number) + }; +} + +- (void)testMultipleInequalityWithNestedField { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : [self nestedData:400], + @"doc2" : [self nestedData:200], + @"doc3" : [self nestedData:100], + @"doc4" : [self nestedData:300] + }]; + + FIRQuerySnapshot *snapshot = [self + readDocumentSetForRef:[[[[collRef queryWhereField:@"metadata.createdAt" + isLessThanOrEqualTo:@500] queryWhereField:@"metadata.createdAt" + isGreaterThan:@100] + queryWhereField:@"name" + isNotEqualTo:@"room 200"] queryOrderedByField:@"name" + descending:NO]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc4", @"doc1" ])); + + snapshot = + [self readDocumentSetForRef:[[[[collRef queryWhereField:@"field" + isGreaterThanOrEqualTo:@"field 100"] + queryWhereFieldPath:[[FIRFieldPath alloc] + initWithFields:@[ @"field.dot" ]] + isNotEqualTo:@300] queryWhereField:@"field\\slash" + isLessThan:@400] + queryOrderedByField:@"name" + descending:YES]]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc2", @"doc3" ])); +} + +- (void)testMultipleInequalityWithCompositeFilters { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @0, @"v" : @5}, + @"doc2" : @{@"key" : @"aa", @"sort" : @4, @"v" : @4}, + @"doc3" : @{@"key" : @"c", @"sort" : @3, @"v" : @3}, + @"doc4" : @{@"key" : @"b", @"sort" : @2, @"v" : @2}, + @"doc5" : @{@"key" : @"b", @"sort" : @2, @"v" : @1}, + @"doc6" : @{@"key" : @"b", @"sort" : @0, @"v" : @0} + }]; + + FIRQuerySnapshot *snapshot = [self + readDocumentSetForRef:[collRef queryWhereFilter:[FIRFilter orFilterWithFilters:@[ + [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"key" isEqualTo:@"b"], + [FIRFilter filterWhereField:@"sort" isLessThanOrEqualTo:@2] + ]], + [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"key" isNotEqualTo:@"b"], + [FIRFilter filterWhereField:@"v" isGreaterThan:@4] + ]] + ]]]]; + // Implicitly ordered by: 'key' asc, 'sort' asc, 'v' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), + (@[ @"doc1", @"doc6", @"doc5", @"doc4" ])); + + snapshot = [self + readDocumentSetForRef:[[[collRef queryWhereFilter:[FIRFilter orFilterWithFilters:@[ + [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"key" isEqualTo:@"b"], + [FIRFilter filterWhereField:@"sort" + isLessThanOrEqualTo:@2] + ]], + [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"key" isNotEqualTo:@"b"], + [FIRFilter filterWhereField:@"v" isGreaterThan:@4] + ]] + ]]] queryOrderedByField:@"sort" + descending:YES] queryOrderedByField:@"key"]]; + // Ordered by: 'sort' desc, 'key' asc, 'v' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), + (@[ @"doc5", @"doc4", @"doc1", @"doc6" ])); + + snapshot = [self + readDocumentSetForRef:[collRef + queryWhereFilter:[FIRFilter andFilterWithFilters:@[ + + [FIRFilter orFilterWithFilters:@[ + [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"key" isEqualTo:@"b"], + [FIRFilter filterWhereField:@"sort" isLessThanOrEqualTo:@4] + ]], + [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"key" isNotEqualTo:@"b"], + [FIRFilter filterWhereField:@"v" isGreaterThanOrEqualTo:@4] + ]] + ]], + + [FIRFilter orFilterWithFilters:@[ + [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"key" isGreaterThan:@"b"], + [FIRFilter filterWhereField:@"sort" isGreaterThanOrEqualTo:@1] + ]], + [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"key" isLessThan:@"b"], + [FIRFilter filterWhereField:@"v" isGreaterThan:@0] + ]] + ]] + + ]]]]; + // Implicitly ordered by: 'key' asc, 'sort' asc, 'v' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc1", @"doc2" ])); +} + +- (void)testMultipleInequalityFieldsWillBeImplicitlyOrderedLexicographically { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @0, @"v" : @5}, + @"doc2" : @{@"key" : @"aa", @"sort" : @4, @"v" : @4}, + @"doc3" : @{@"key" : @"b", @"sort" : @3, @"v" : @3}, + @"doc4" : @{@"key" : @"b", @"sort" : @2, @"v" : @2}, + @"doc5" : @{@"key" : @"b", @"sort" : @2, @"v" : @1}, + @"doc6" : @{@"key" : @"b", @"sort" : @0, @"v" : @0} + }]; + + FIRQuerySnapshot *snapshot = [self + readDocumentSetForRef:[[[collRef queryWhereField:@"key" isNotEqualTo:@"a"] + queryWhereField:@"sort" + isGreaterThan:@1] queryWhereField:@"v" in:@[ @1, @2, @3, @4 ]]]; + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), + (@[ @"doc2", @"doc4", @"doc5", @"doc3" ])); + + snapshot = [self readDocumentSetForRef:[[[collRef queryWhereField:@"sort" + isGreaterThan:@1] queryWhereField:@"key" + isNotEqualTo:@"a"] + queryWhereField:@"v" + in:@[ @1, @2, @3, @4 ]]]; + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), + (@[ @"doc2", @"doc4", @"doc5", @"doc3" ])); +} + +- (void)testMultipleInequalityWithMultipleExplicitOrderBy { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @5, @"v" : @0}, + @"doc2" : @{@"key" : @"aa", @"sort" : @4, @"v" : @0}, + @"doc3" : @{@"key" : @"b", @"sort" : @3, @"v" : @1}, + @"doc4" : @{@"key" : @"b", @"sort" : @2, @"v" : @1}, + @"doc5" : @{@"key" : @"bb", @"sort" : @1, @"v" : @1}, + @"doc6" : @{@"key" : @"c", @"sort" : @0, @"v" : @2} + }]; + + FIRQuerySnapshot *snapshot = [self + readDocumentSetForRef:[[[collRef queryWhereField:@"key" isGreaterThan:@"a"] + queryWhereField:@"sort" + isGreaterThanOrEqualTo:@1] queryOrderedByField:@"v" descending:NO]]; + // Ordered by: 'v' asc, 'key' asc, 'sort' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), + (@[ @"doc2", @"doc4", @"doc3", @"doc5" ])); + + snapshot = [self readDocumentSetForRef:[[[[collRef queryWhereField:@"key" + isGreaterThan:@"a"] queryWhereField:@"sort" + isGreaterThanOrEqualTo:@1] + queryOrderedByField:@"v" + descending:NO] queryOrderedByField:@"sort" + descending:NO]]; + // Ordered by: 'v asc, 'sort' asc, 'key' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), + (@[ @"doc2", @"doc5", @"doc4", @"doc3" ])); + + snapshot = [self readDocumentSetForRef:[[[collRef queryWhereField:@"key" isGreaterThan:@"a"] + queryWhereField:@"sort" + isGreaterThanOrEqualTo:@1] queryOrderedByField:@"v" + descending:YES]]; + // Implicit order by matches the direction of last explicit order by. + // Ordered by: 'v' desc, 'key' desc, 'sort' desc, __name__ desc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), + (@[ @"doc5", @"doc3", @"doc4", @"doc2" ])); + + snapshot = [self readDocumentSetForRef:[[[[collRef queryWhereField:@"key" + isGreaterThan:@"a"] queryWhereField:@"sort" + isGreaterThanOrEqualTo:@1] + queryOrderedByField:@"v" + descending:YES] queryOrderedByField:@"sort" + descending:NO]]; + // Ordered by: 'v desc, 'sort' asc, 'key' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), + (@[ @"doc5", @"doc4", @"doc3", @"doc2" ])); +} + +- (void)testMultipleInequalityInAggregateQuery { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @5, @"v" : @0}, + @"doc2" : @{@"key" : @"aa", @"sort" : @4, @"v" : @0}, + @"doc3" : @{@"key" : @"b", @"sort" : @3, @"v" : @1}, + @"doc4" : @{@"key" : @"b", @"sort" : @2, @"v" : @1}, + @"doc5" : @{@"key" : @"bb", @"sort" : @1, @"v" : @1}, + }]; + + FIRAggregateQuerySnapshot *snapshot = + [self readSnapshotForAggregate:[[[[collRef queryWhereField:@"key" isGreaterThan:@"a"] + queryWhereField:@"sort" + isGreaterThanOrEqualTo:@1] queryOrderedByField:@"v" + descending:NO] + aggregate:@[ + [FIRAggregateField aggregateFieldForCount], + [FIRAggregateField aggregateFieldForSumOfField:@"sort"], + [FIRAggregateField aggregateFieldForAverageOfField:@"v"] + ]]]; + XCTAssertEqual([snapshot count], [NSNumber numberWithLong:4L]); + + snapshot = [self + readSnapshotForAggregate:[[[[collRef queryWhereField:@"key" isGreaterThan:@"a"] + queryWhereField:@"sort" + isGreaterThanOrEqualTo:@1] queryWhereField:@"v" isNotEqualTo:@0] + aggregate:@[ + [FIRAggregateField aggregateFieldForCount], + [FIRAggregateField aggregateFieldForSumOfField:@"sort"], + [FIRAggregateField aggregateFieldForAverageOfField:@"v"], + ]]]; + XCTAssertEqual([snapshot valueForAggregation:[FIRAggregateField aggregateFieldForCount]], + [NSNumber numberWithLong:3L]); + XCTAssertEqual( + [[snapshot valueForAggregation:[FIRAggregateField aggregateFieldForSumOfField:@"sort"]] + longValue], + 6L); + XCTAssertEqual( + [snapshot valueForAggregation:[FIRAggregateField aggregateFieldForAverageOfField:@"v"]], + [NSNumber numberWithDouble:1.0]); +} + +- (void)testMultipleInequalityFieldsWithDocumentKey { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @5}, + @"doc2" : @{@"key" : @"aa", @"sort" : @4}, + @"doc3" : @{@"key" : @"b", @"sort" : @3}, + @"doc4" : @{@"key" : @"b", @"sort" : @2}, + @"doc5" : @{@"key" : @"bb", @"sort" : @1} + }]; + + FIRQuerySnapshot *snapshot = [self + readDocumentSetForRef:[[[collRef queryWhereField:@"sort" + isGreaterThan:@1] queryWhereField:@"key" isNotEqualTo:@"a"] + queryWhereFieldPath:[FIRFieldPath documentID] + isLessThan:@"doc5"]]; + // Document Key in inequality field will implicitly ordered to the last. + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc2", @"doc4", @"doc3" ])); + + snapshot = [self readDocumentSetForRef:[[[collRef queryWhereFieldPath:[FIRFieldPath documentID] + isLessThan:@"doc5"] + queryWhereField:@"sort" + isGreaterThan:@1] queryWhereField:@"key" + isNotEqualTo:@"a"]]; + // Changing filters order will not effect implicit order. + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc2", @"doc4", @"doc3" ])); + + snapshot = [self + readDocumentSetForRef:[[[[collRef queryWhereFieldPath:[FIRFieldPath documentID] + isLessThan:@"doc5"] queryWhereField:@"sort" + isGreaterThan:@1] + queryWhereField:@"key" + isNotEqualTo:@"a"] queryOrderedByField:@"sort" descending:YES]]; + // Ordered by: 'sort' desc,'key' desc, __name__ desc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc2", @"doc3", @"doc4" ])); +} + +- (void)testMultipleInequalityReadFromCacheWhenOffline { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"key" : @"a", @"sort" : @1}, + @"doc2" : @{@"key" : @"aa", @"sort" : @4}, + @"doc3" : @{@"key" : @"b", @"sort" : @3}, + @"doc4" : @{@"key" : @"b", @"sort" : @2}, + }]; + + FIRQuery *query = [[collRef queryWhereField:@"key" isNotEqualTo:@"a"] queryWhereField:@"sort" + isLessThanOrEqualTo:@3]; + + // populate the cache. + FIRQuerySnapshot *snapshot = [self readDocumentSetForRef:query]; + XCTAssertEqual(snapshot.count, 2L); + XCTAssertEqual(snapshot.metadata.isFromCache, NO); + + [self disableNetwork]; + + snapshot = [self readDocumentSetForRef:query]; + XCTAssertEqual(snapshot.count, 2L); + XCTAssertEqual(snapshot.metadata.isFromCache, YES); + // Implicitly ordered by: 'key' asc, 'sort' asc, __name__ asc + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc4", @"doc3" ])); +} + +- (void)testMultipleInequalityFromCacheAndFromServer { + // TODO(MIEQ): Enable this test against production when possible. + XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], + "Skip this test if running against production because multiple inequality is " + "not supported yet."); + + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"doc1" : @{@"a" : @1, @"b" : @0}, + @"doc2" : @{@"a" : @2, @"b" : @1}, + @"doc3" : @{@"a" : @3, @"b" : @2}, + @"doc4" : @{@"a" : @1, @"b" : @3}, + @"doc5" : @{@"a" : @1, @"b" : @1}, + + }]; + + // implicit AND: a != 1 && b < 2 + FIRQuery *query = [[collRef queryWhereField:@"a" isNotEqualTo:@1] queryWhereField:@"b" + isLessThan:@2]; + [self checkOnlineAndOfflineQuery:query matchesResult:@[ @"doc2" ]]; + + // explicit AND: a != 1 && b < 2 + FIRFilter *filter = [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"a" isNotEqualTo:@1], [FIRFilter filterWhereField:@"b" + isLessThan:@2] + ]]; + [self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter] matchesResult:@[ @"doc2" ]]; + + // explicit AND: a < 3 && b not-in [2, 3] + // Implicitly ordered by: a asc, __name__ asc + filter = [FIRFilter andFilterWithFilters:@[ + [FIRFilter filterWhereField:@"a" isLessThan:@3], [FIRFilter filterWhereField:@"b" + notIn:@[ @2, @3 ]] + ]]; + [self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter] + matchesResult:@[ @"doc1", @"doc5", @"doc2" ]]; + + // a <3 && b != 0, ordered by: b desc, a desc, __name__ desc + query = [[[[collRef queryWhereField:@"a" isLessThan:@3] queryWhereField:@"b" isNotEqualTo:@0] + queryOrderedByField:@"b" + descending:YES] queryLimitedTo:2]; + [self checkOnlineAndOfflineQuery:query matchesResult:@[ @"doc4", @"doc2" ]]; + + // explicit OR: a>2 || b<1. + filter = [FIRFilter orFilterWithFilters:@[ + [FIRFilter filterWhereField:@"a" isGreaterThan:@2], [FIRFilter filterWhereField:@"b" + isLessThan:@1] + ]]; + [self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter] + matchesResult:@[ @"doc1", @"doc3" ]]; +} + - (void)testResumingAQueryShouldUseBloomFilterToAvoidFullRequery { using firebase::firestore::testutil::CaptureExistenceFilterMismatches; using firebase::firestore::util::TestingHooks; From 14514debdadb886c02d9afb95a247107ea85d74e Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 27 Jul 2023 17:44:30 -0400 Subject: [PATCH 03/46] remove single inequality functions --- Firestore/core/src/core/composite_filter.cc | 11 ----------- Firestore/core/src/core/composite_filter.h | 3 +-- Firestore/core/src/core/field_filter.cc | 7 ------- Firestore/core/src/core/field_filter.h | 1 - Firestore/core/src/core/filter.h | 9 --------- Firestore/core/src/core/query.cc | 18 ------------------ Firestore/core/src/core/query.h | 3 --- 7 files changed, 1 insertion(+), 51 deletions(-) diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index 38a398c5e9b..4db67f976f2 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -141,17 +141,6 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter( return nullptr; } -const model::FieldPath* CompositeFilter::Rep::GetFirstInequalityField() const { - CheckFunction condition = [](const FieldFilter& field_filter) { - return field_filter.IsInequality(); - }; - const FieldFilter* found = FindFirstMatchingFilter(condition); - if (found) { - return &(found->field()); - } - return nullptr; -} - const std::vector& CompositeFilter::Rep::GetInequalityFilters() const { if (Filter::Rep::memoized_inequality_filters_.empty()) { diff --git a/Firestore/core/src/core/composite_filter.h b/Firestore/core/src/core/composite_filter.h index 0031fbae8bf..fc17802aa89 100644 --- a/Firestore/core/src/core/composite_filter.h +++ b/Firestore/core/src/core/composite_filter.h @@ -139,9 +139,8 @@ class CompositeFilter : public Filter { } const std::vector& GetFlattenedFilters() const override; - const std::vector& GetInequalityFilters() const override; - const model::FieldPath* GetFirstInequalityField() const override; + const std::vector& GetInequalityFilters() const override; std::vector GetFilters() const override { return filters(); diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index a5077d99464..a402364da4a 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -203,13 +203,6 @@ bool FieldFilter::Rep::Equals(const Filter::Rep& other) const { *value_rhs_ == *other_rep.value_rhs_; } -const model::FieldPath* FieldFilter::Rep::GetFirstInequalityField() const { - if (IsInequality()) { - return &field(); - } - return nullptr; -} - const std::vector& FieldFilter::Rep::GetInequalityFilters() const { // This is already a field filter, so we return a vector of size one. if (IsInequality() && Filter::Rep::memoized_inequality_filters_.empty()) { diff --git a/Firestore/core/src/core/field_filter.h b/Firestore/core/src/core/field_filter.h index 2b8f02d9a5c..e55e2946abd 100644 --- a/Firestore/core/src/core/field_filter.h +++ b/Firestore/core/src/core/field_filter.h @@ -117,7 +117,6 @@ class FieldFilter : public Filter { return false; } - const model::FieldPath* GetFirstInequalityField() const override; const std::vector& GetInequalityFilters() const override; const std::vector& GetFlattenedFilters() const override; diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index bfaded237e4..2021a1603a5 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -95,14 +95,6 @@ class Filter { return rep_->IsEmpty(); } - /** - * Returns the first inequality filter contained within this filter. - * Returns nullptr if it does not contain any inequalities. - */ - const model::FieldPath* GetFirstInequalityField() const { - return rep_->GetFirstInequalityField(); - } - /** * Returns the first inequality filter contained within this filter. * Returns empty vector if it does not contain any inequalities. @@ -161,7 +153,6 @@ class Filter { virtual bool IsEmpty() const = 0; - virtual const model::FieldPath* GetFirstInequalityField() const = 0; virtual const std::vector& GetInequalityFilters() const = 0; // virtual const std::vector GetInequalityFilters() const = 0; diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index f006b3f1712..a2e31130788 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -67,16 +67,6 @@ bool Query::MatchesAllDocuments() const { explicit_order_bys_.front().field().IsKeyFieldPath())); } -const FieldPath* Query::InequalityFilterField() const { - for (const Filter& filter : filters_) { - const FieldPath* found = filter.GetFirstInequalityField(); - if (found) { - return found; - } - } - return nullptr; -} - const std::set Query::InequalityFilterFields() const { std::set result; for (const Filter& filter : filters_) { @@ -145,14 +135,6 @@ const std::vector& Query::order_bys() const { return memoized_order_bys_; } -const FieldPath* Query::FirstOrderByField() const { - if (explicit_order_bys_.empty()) { - return nullptr; - } - - return &explicit_order_bys_.front().field(); -} - LimitType Query::limit_type() const { return limit_type_; } diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index c7402c9a3be..d9057c3a151 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -145,9 +145,6 @@ class Query { */ const std::vector& order_bys() const; - /** Returns the first field in an order-by constraint, or nullptr if none. */ - const model::FieldPath* FirstOrderByField() const; - bool has_limit() const { return limit_ != Target::kNoLimit; } From 34dd2e9ecfda27cb2a44652b18c220020358547c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 27 Jul 2023 23:17:34 -0400 Subject: [PATCH 04/46] include set --- Firestore/core/src/core/query.h | 1 + 1 file changed, 1 insertion(+) diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index d9057c3a151..89714cfc71a 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include From 1141850ed36a18c05756cdb7dc5fc49ddd1c7300 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 28 Jul 2023 13:33:12 -0400 Subject: [PATCH 05/46] update target index matcher --- .../Tests/Integration/API/FIRQueryTests.mm | 2 +- Firestore/core/src/core/query.cc | 8 +++---- .../core/src/model/target_index_matcher.cc | 22 +++++++++++++------ .../core/src/model/target_index_matcher.h | 14 +++++++++++- 4 files changed, 33 insertions(+), 13 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index a52c26d0649..88232e4a975 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1266,7 +1266,7 @@ - (void)testMultipleInequalityOnDifferentFields { XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"doc4", @"doc1" ])); } -- (void)testMultipleInequalityOnUnaryValues { +- (void)testMultipleInequalityOnSpecialValues { // TODO(MIEQ): Enable this test against production when possible. XCTSkipIf(![FSTIntegrationTestCase isRunningAgainstEmulator], "Skip this test if running against production because multiple inequality is " diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index a2e31130788..ce749d914eb 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -74,7 +74,7 @@ const std::set Query::InequalityFilterFields() const { filter.GetInequalityFilters(); for (const FieldFilter& subFilter : inequalityFilters) { { - result.insert(subFilter.field()); + result.emplace(subFilter.field()); } } } @@ -103,8 +103,8 @@ const std::vector& Query::order_bys() const { fieldsNormalized.insert(order_by.field()); } - // The direction of the implicit key ordering always matches the - // direction of the last explicit sort order + // The order of the implicit ordering always matches the last explicit order + // by. Direction last_direction = explicit_order_bys_.empty() ? Direction::Ascending : explicit_order_bys_.back().direction(); @@ -126,7 +126,7 @@ const std::vector& Query::order_bys() const { if (fieldsNormalized.find(FieldPath::KeyFieldPath()) == fieldsNormalized.end()) { - result.emplace_back(FieldPath::KeyFieldPath(), last_direction); + result.push_back(OrderBy(FieldPath::KeyFieldPath(), last_direction)); } memoized_order_bys_ = std::move(result); diff --git a/Firestore/core/src/model/target_index_matcher.cc b/Firestore/core/src/model/target_index_matcher.cc index d17baf6417a..2c89d61adc4 100644 --- a/Firestore/core/src/model/target_index_matcher.cc +++ b/Firestore/core/src/model/target_index_matcher.cc @@ -34,12 +34,11 @@ TargetIndexMatcher::TargetIndexMatcher(const core::Target& target) { ? *target.collection_group() : target.path().last_segment(); order_bys_ = target.order_bys(); - inequality_filter_ = absl::nullopt; for (const Filter& filter : target.filters()) { FieldFilter field_filter(filter); - if (field_filter.IsInequality() && !inequality_filter_.has_value()) { - inequality_filter_ = field_filter; + if (field_filter.IsInequality()) { + inequality_filters_.emplace(field_filter); } else { equality_filters_.push_back(field_filter); } @@ -85,13 +84,22 @@ bool TargetIndexMatcher::ServedByIndex(const model::FieldIndex& index) { // `order_bys_` has at least one element. auto order_by_iter = order_bys_.begin(); - if (inequality_filter_.has_value()) { + if (!inequality_filters_.empty()) { + if (inequality_filters_.size() > 1) { + // Only single inequality is supported for now. + return false; + } + + // Only a single inequality is currently supported. Get the only entry in + // the set. + const FieldFilter& inequality_filter = *inequality_filters_.begin(); + // If there is an inequality filter and the field was not in one of the // equality filters above, the next segment must match both the filter // and the first orderBy clause. - if (equality_segments.count( - inequality_filter_.value().field().CanonicalString()) == 0) { - if (!MatchesFilter(inequality_filter_, segments[segment_index]) || + if (equality_segments.count(inequality_filter.field().CanonicalString()) == + 0) { + if (!MatchesFilter(inequality_filter, segments[segment_index]) || !MatchesOrderBy(*(order_by_iter++), segments[segment_index])) { return false; } diff --git a/Firestore/core/src/model/target_index_matcher.h b/Firestore/core/src/model/target_index_matcher.h index 5f7c3f46a8a..f82b171b80a 100644 --- a/Firestore/core/src/model/target_index_matcher.h +++ b/Firestore/core/src/model/target_index_matcher.h @@ -17,6 +17,7 @@ #ifndef FIRESTORE_CORE_SRC_MODEL_TARGET_INDEX_MATCHER_H_ #define FIRESTORE_CORE_SRC_MODEL_TARGET_INDEX_MATCHER_H_ +#include #include #include @@ -88,10 +89,21 @@ class TargetIndexMatcher { bool MatchesOrderBy(const core::OrderBy& order_by, const model::Segment& segment); + // Custom functor comparator for FieldFilter + struct FieldFilterComparator { + bool operator()(const core::FieldFilter& filter1, + const core::FieldFilter& filter2) const { + return filter1.field() < filter2.field(); + } + }; + // The collection ID (or collection group) of the query target. std::string collection_id_; - absl::optional inequality_filter_; + // The inequality filters of the target (if it exists). + // Note: The sort on FieldFilters is not required. We are comparing the + // inequality Filters based on its field path difference only. + std::set inequality_filters_; std::vector equality_filters_; std::vector order_bys_; }; From 088733b69f5e3239b36ba21543b8ae966f81865e Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 1 Aug 2023 17:12:00 -0400 Subject: [PATCH 06/46] update code --- Firestore/core/src/core/composite_filter.cc | 12 ------------ Firestore/core/src/core/composite_filter.h | 2 -- Firestore/core/src/core/field_filter.cc | 10 ---------- Firestore/core/src/core/field_filter.h | 2 -- Firestore/core/src/core/filter.h | 13 ------------- Firestore/core/src/core/query.cc | 7 ++----- 6 files changed, 2 insertions(+), 44 deletions(-) diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index 4db67f976f2..63491d1209c 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -141,18 +141,6 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter( return nullptr; } -const std::vector& CompositeFilter::Rep::GetInequalityFilters() - const { - if (Filter::Rep::memoized_inequality_filters_.empty()) { - for (const auto& filter : GetFlattenedFilters()) { - std::copy(filter.GetInequalityFilters().begin(), - filter.GetInequalityFilters().end(), - std::back_inserter(Filter::Rep::memoized_inequality_filters_)); - } - } - return Filter::Rep::memoized_inequality_filters_; -} - const std::vector& CompositeFilter::Rep::GetFlattenedFilters() const { if (Filter::Rep::memoized_flattened_filters_.empty() && !filters().empty()) { diff --git a/Firestore/core/src/core/composite_filter.h b/Firestore/core/src/core/composite_filter.h index fc17802aa89..24671d3e44e 100644 --- a/Firestore/core/src/core/composite_filter.h +++ b/Firestore/core/src/core/composite_filter.h @@ -140,8 +140,6 @@ class CompositeFilter : public Filter { const std::vector& GetFlattenedFilters() const override; - const std::vector& GetInequalityFilters() const override; - std::vector GetFilters() const override { return filters(); } diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index a402364da4a..14441486ac9 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -203,16 +203,6 @@ bool FieldFilter::Rep::Equals(const Filter::Rep& other) const { *value_rhs_ == *other_rep.value_rhs_; } -const std::vector& FieldFilter::Rep::GetInequalityFilters() const { - // This is already a field filter, so we return a vector of size one. - if (IsInequality() && Filter::Rep::memoized_inequality_filters_.empty()) { - Filter::Rep::memoized_inequality_filters_ = std::vector{ - FieldFilter(std::make_shared(*this))}; - } - - return Filter::Rep::memoized_inequality_filters_; -} - } // namespace core } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/core/field_filter.h b/Firestore/core/src/core/field_filter.h index e55e2946abd..48219f222f2 100644 --- a/Firestore/core/src/core/field_filter.h +++ b/Firestore/core/src/core/field_filter.h @@ -117,8 +117,6 @@ class FieldFilter : public Filter { return false; } - const std::vector& GetInequalityFilters() const override; - const std::vector& GetFlattenedFilters() const override; std::vector GetFilters() const override; diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 2021a1603a5..164ecb828a4 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -95,14 +95,6 @@ class Filter { return rep_->IsEmpty(); } - /** - * Returns the first inequality filter contained within this filter. - * Returns empty vector if it does not contain any inequalities. - */ - const std::vector& GetInequalityFilters() const { - return rep_->GetInequalityFilters(); - } - /** * Returns a list of all field filters that are contained within this filter. */ @@ -153,9 +145,6 @@ class Filter { virtual bool IsEmpty() const = 0; - virtual const std::vector& GetInequalityFilters() const = 0; - // virtual const std::vector GetInequalityFilters() const = 0; - virtual const std::vector& GetFlattenedFilters() const = 0; virtual std::vector GetFilters() const = 0; @@ -165,8 +154,6 @@ class Filter { * traversing the tree of filters contained in this composite filter. */ mutable std::vector memoized_flattened_filters_; - - mutable std::vector memoized_inequality_filters_; }; explicit Filter(std::shared_ptr&& rep) : rep_(rep) { diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index ce749d914eb..0f99fa65911 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -70,15 +70,12 @@ bool Query::MatchesAllDocuments() const { const std::set Query::InequalityFilterFields() const { std::set result; for (const Filter& filter : filters_) { - const std::vector& inequalityFilters = - filter.GetInequalityFilters(); - for (const FieldFilter& subFilter : inequalityFilters) { - { + for (const FieldFilter& subFilter : filter.GetFlattenedFilters()) { + if (subFilter.IsInequality()) { result.emplace(subFilter.field()); } } } - return result; } From daca2294d4b0b58c929cb7fb003421b5e92897b6 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 2 Aug 2023 12:42:31 -0400 Subject: [PATCH 07/46] add implicit order by unit tests --- Firestore/core/test/unit/core/query_test.cc | 82 +++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/Firestore/core/test/unit/core/query_test.cc b/Firestore/core/test/unit/core/query_test.cc index cf27f5ea759..b26a6519cb0 100644 --- a/Firestore/core/test/unit/core/query_test.cc +++ b/Firestore/core/test/unit/core/query_test.cc @@ -835,6 +835,88 @@ TEST(QueryTest, ImplicitOrderBy) { })); } +TEST(QueryTest, ImplicitOrderByInMultipleInequality) { + auto base_query = testutil::Query("foo"); + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) + .AddingFilter(testutil::Filter("aa", ">", 5)) + .AddingFilter(testutil::Filter("b", "<=", 5)) + .AddingFilter(testutil::Filter("A", ">=", 5)) + .order_bys(), + (std::vector{ + testutil::OrderBy("A", "asc"), + testutil::OrderBy("a", "asc"), + testutil::OrderBy("aa", "asc"), + testutil::OrderBy("b", "asc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), + })); + + // numbers + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) + .AddingFilter(testutil::Filter("1", ">", 5)) + .AddingFilter(testutil::Filter("19", "<=", 5)) + .AddingFilter(testutil::Filter("2", ">=", 5)) + .order_bys(), + (std::vector{ + testutil::OrderBy("1", "asc"), + testutil::OrderBy("19", "asc"), + testutil::OrderBy("2", "asc"), + testutil::OrderBy("a", "asc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), + })); + + // nested fields + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) + .AddingFilter(testutil::Filter("aa", ">", 5)) + .AddingFilter(testutil::Filter("a.a", "<=", 5)) + .order_bys(), + (std::vector{ + testutil::OrderBy("a", "asc"), + testutil::OrderBy("a.a", "asc"), + testutil::OrderBy("aa", "asc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), + })); + + // special characters + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) + .AddingFilter(testutil::Filter("_a", ">", 5)) + .AddingFilter(testutil::Filter("a.a", "<=", 5)) + .order_bys(), + (std::vector{ + testutil::OrderBy("_a", "asc"), + testutil::OrderBy("a", "asc"), + testutil::OrderBy("a.a", "asc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), + })); + + // field name with dot + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) + .AddingFilter(testutil::Filter("a.z", ">", 5)) + .AddingFilter(testutil::Filter(("`a.a`"), "<=", 5)) + .order_bys(), + (std::vector{ + testutil::OrderBy("a", "asc"), + testutil::OrderBy("a.z", "asc"), + testutil::OrderBy(("`a.a`"), "asc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), + })); + + // composite filter + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) + .AddingFilter( + AndFilters({OrFilters({testutil::Filter("b", ">=", 1), + testutil::Filter("c", "<=", 0)}), + OrFilters({testutil::Filter("d", ">", 3), + testutil::Filter("e", "==", 2)})})) + .order_bys(), + (std::vector{ + testutil::OrderBy("a", "asc"), + testutil::OrderBy("b", "asc"), + testutil::OrderBy("c", "asc"), + testutil::OrderBy("d", "asc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), + })); +} + MATCHER_P(HasCanonicalId, expected, "") { const std::string& actual = arg.CanonicalId(); *result_listener << "which has CanonicalId " << actual; From 2d5839a48e8cce1191fc446ca2ea7f8ed3f89e73 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 2 Aug 2023 14:29:20 -0400 Subject: [PATCH 08/46] fix typo --- .../Integration/API/FIRValidationTests.mm | 79 +++++-------------- Firestore/core/src/core/query.cc | 1 + .../core/src/model/target_index_matcher.h | 6 +- 3 files changed, 25 insertions(+), 61 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm index e5c086f4f4a..84795801cb7 100644 --- a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm @@ -623,87 +623,62 @@ - (void)testQueriesUsingInAndDocumentIdMustHaveProperDocumentReferencesInArray { reason); } -/** Multiple Inequality tests*/ - -- (void)testMultipleInequalityOnDifferentFields { +- (void)testInvalidQueryFilters { FIRCollectionReference *collection = [self collectionRef]; - // Multiple inequalities, one of which is inside a nested composite filter. + NSString *reason = @"Invalid Query. You cannot use 'notIn' filters with 'in' filters."; NSArray *array1 = @[ [FIRFilter andFilterWithFilters:@[ [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c" - isGreaterThan:@"d"] + in:@[ @"d", @"e" ]] ]], [FIRFilter andFilterWithFilters:@[ [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"g" - isEqualTo:@"h"] + notIn:@[ @"h", @"i" ]] ]] ]; + FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]], reason); - XCTAssertNoThrow([[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]] - queryWhereField:@"r" - isGreaterThan:@"s"]); - - XCTAssertNoThrow([[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array1]] - queryOrderedByField:@"r"], ); - - // Conflicting operations within a composite filter. - NSString *reason = @"Invalid Query. You cannot use 'notIn' filters with 'in' filters."; - + reason = @"Invalid Query. You cannot use 'notIn' filters with 'notEqual' filters."; NSArray *array2 = @[ [FIRFilter andFilterWithFilters:@[ [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c" - in:@[ @"d", @"e" ]] + isNotEqualTo:@"d"] ]], [FIRFilter andFilterWithFilters:@[ - [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"c" - notIn:@[ @"f", @"g" ]] + [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"g" + notIn:@[ @"h", @"i" ]] ]] ]; - FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array2]], reason); -} - -- (void)testInvalidQueryFilters { - FIRCollectionReference *collection = [self collectionRef]; - NSString *reason = @"Invalid Query. You cannot use 'notIn' filters with 'in' filters."; - // Conflicting operations between a field filter and a composite filter. + reason = @"Invalid Query. You cannot use more than one 'notIn' filter."; NSArray *array3 = @[ [FIRFilter andFilterWithFilters:@[ [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c" - in:@[ @"d", @"e" ]] + notIn:@[ @"d", @"e" ]] ]], [FIRFilter andFilterWithFilters:@[ [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"g" - isEqualTo:@"h"] + notIn:@[ @"h", @"i" ]] ]] ]; + FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array3]], reason); - NSArray *array4 = @[ @"j", @"k" ]; - - FSTAssertThrows( - [[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array3]] queryWhereField:@"i" - notIn:array4], - reason); - - // Conflicting operations between two composite filters. - NSArray *array5 = @[ + reason = @"Invalid Query. You cannot use more than one 'notEqual' filter."; + NSArray *array4 = @[ [FIRFilter andFilterWithFilters:@[ - [FIRFilter filterWhereField:@"i" isEqualTo:@"j"], [FIRFilter filterWhereField:@"l" - notIn:@[ @"m", @"n" ]] + [FIRFilter filterWhereField:@"a" isEqualTo:@"b"], [FIRFilter filterWhereField:@"c" + isNotEqualTo:@"d"] ]], [FIRFilter andFilterWithFilters:@[ - [FIRFilter filterWhereField:@"o" isEqualTo:@"p"], [FIRFilter filterWhereField:@"q" - isEqualTo:@"r"] + [FIRFilter filterWhereField:@"e" isEqualTo:@"f"], [FIRFilter filterWhereField:@"g" + isNotEqualTo:@"h"] ]] ]; - - FSTAssertThrows([[collection queryWhereFilter:[FIRFilter orFilterWithFilters:array3]] - queryWhereFilter:[FIRFilter orFilterWithFilters:array5]], - reason); + FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array4]], reason); } -- (void)testQueryInequalityFieldMustMatchFirstOrderByField { +- (void)testQueryInequalityFieldWithMultipleOrderByField { FIRCollectionReference *coll = [self.db collectionWithPath:@"collection"]; FIRQuery *base = [coll queryWhereField:@"x" isGreaterThanOrEqualTo:@32]; @@ -743,18 +718,6 @@ - (void)testQueryInequalityFieldMustMatchFirstOrderByField { @"array_contains different than orderBy works."); } -- (void)testQueriesWithMultipleNotEqualAndInequalitiesFail { - FIRCollectionReference *coll = [self.db collectionWithPath:@"collection"]; - - FSTAssertThrows([[coll queryWhereField:@"x" isNotEqualTo:@1] queryWhereField:@"x" - isNotEqualTo:@2], - @"Invalid Query. You cannot use more than one 'notEqual' filter."); - - FSTAssertThrows([[coll queryWhereField:@"x" isNotEqualTo:@1] queryWhereField:@"y" - isNotEqualTo:@2], - @"Invalid Query. You cannot use more than one 'notEqual' filter."); -} - - (void)testQueriesWithNotEqualAndNotInFiltersFail { FIRCollectionReference *coll = [self.db collectionWithPath:@"collection"]; diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index 0f99fa65911..e1e4c4642aa 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -121,6 +121,7 @@ const std::vector& Query::order_bys() const { } } + // Add the document key field to the last if it is not explicitly ordered. if (fieldsNormalized.find(FieldPath::KeyFieldPath()) == fieldsNormalized.end()) { result.push_back(OrderBy(FieldPath::KeyFieldPath(), last_direction)); diff --git a/Firestore/core/src/model/target_index_matcher.h b/Firestore/core/src/model/target_index_matcher.h index f82b171b80a..1319b41506d 100644 --- a/Firestore/core/src/model/target_index_matcher.h +++ b/Firestore/core/src/model/target_index_matcher.h @@ -89,7 +89,7 @@ class TargetIndexMatcher { bool MatchesOrderBy(const core::OrderBy& order_by, const model::Segment& segment); - // Custom functor comparator for FieldFilter + // Custom comparator for FieldFilter based on its Field property. struct FieldFilterComparator { bool operator()(const core::FieldFilter& filter1, const core::FieldFilter& filter2) const { @@ -101,8 +101,8 @@ class TargetIndexMatcher { std::string collection_id_; // The inequality filters of the target (if it exists). - // Note: The sort on FieldFilters is not required. We are comparing the - // inequality Filters based on its field path difference only. + // Note: The sort on FieldFilters is not required. We are using comparator to + // differentiate inequality Filters based on its field path only. std::set inequality_filters_; std::vector equality_filters_; std::vector order_bys_; From ec3ef2943990c117f91d739a3539e8a1bb5bdde9 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 2 Aug 2023 14:50:12 -0400 Subject: [PATCH 09/46] fix typo --- Firestore/Example/Tests/Integration/API/FIRValidationTests.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm index 84795801cb7..87dd9c5ec08 100644 --- a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm @@ -678,7 +678,7 @@ - (void)testInvalidQueryFilters { FSTAssertThrows([collection queryWhereFilter:[FIRFilter orFilterWithFilters:array4]], reason); } -- (void)testQueryInequalityFieldWithMultipleOrderByField { +- (void)testQueryInequalityFieldWithMultipleOrderByFields { FIRCollectionReference *coll = [self.db collectionWithPath:@"collection"]; FIRQuery *base = [coll queryWhereField:@"x" isGreaterThanOrEqualTo:@32]; From 8656303affeba7ea21ac7991516dbed470b5b00b Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 2 Aug 2023 14:57:55 -0400 Subject: [PATCH 10/46] Update query.cc --- Firestore/core/src/core/query.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index e1e4c4642aa..9a785dd9cf3 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -109,8 +109,8 @@ const std::vector& Query::order_bys() const { // Any inequality fields not explicitly ordered should be implicitly ordered // in a lexicographical order. When there are multiple inequality filters on // the same field, the field should be added only once. Note: - // `SortedSet` sorts the key field before other fields. However, - // we want the key field to be sorted last. + // `std::set` sorts the key field before other fields. + // However, we want the key field to be sorted last. const std::set inequality_fields = InequalityFilterFields(); From 09adc0083a61bc154155d8b072bf488b0b591414 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 11 Aug 2023 10:56:49 -0400 Subject: [PATCH 11/46] resolve comments --- Firestore/CHANGELOG.md | 1 + Firestore/Example/Tests/Integration/API/FIRQueryTests.mm | 1 + Firestore/core/src/core/query.h | 3 +++ 3 files changed, 5 insertions(+) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index 37b5bbd9cd3..5ac9d0828e8 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -12,6 +12,7 @@ ``` To go back to using the binary distribution of Firestore, quit Xcode and open Xcode like normal, without the environment variable. (#11492) +- [feature] Added support for having multiple inequality fields in compound queries. (#11626) # 10.11.0 - [feature] Expose MultiDb API for public preview. (#10465) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 8004a707063..a24ad563ce7 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -24,6 +24,7 @@ #include "Firestore/core/test/unit/testutil/testing_hooks_util.h" +// TODO(MIEQ) update these imports with public imports when aggregate types are public #import "Firestore/Source/API/FIRAggregateQuerySnapshot+Internal.h" #import "Firestore/Source/API/FIRQuery+Internal.h" diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index 89714cfc71a..260e13d6fb4 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -118,6 +118,9 @@ class Query { */ const model::FieldPath* InequalityFilterField() const; + /** + * Returns the sorted set of inequality filter fields used in this query. + */ const std::set InequalityFilterFields() const; /** From e9722622f39e3412bbceae534059dc049a76974c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 11 Aug 2023 11:55:07 -0400 Subject: [PATCH 12/46] Update query_test.cc --- Firestore/core/test/unit/core/query_test.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Firestore/core/test/unit/core/query_test.cc b/Firestore/core/test/unit/core/query_test.cc index 180e2c415c7..eea99384201 100644 --- a/Firestore/core/test/unit/core/query_test.cc +++ b/Firestore/core/test/unit/core/query_test.cc @@ -842,7 +842,7 @@ TEST(QueryTest, ImplicitOrderByInMultipleInequality) { .AddingFilter(testutil::Filter("aa", ">", 5)) .AddingFilter(testutil::Filter("b", "<=", 5)) .AddingFilter(testutil::Filter("A", ">=", 5)) - .order_bys(), + .normalized_order_bys(), (std::vector{ testutil::OrderBy("A", "asc"), testutil::OrderBy("a", "asc"), @@ -856,7 +856,7 @@ TEST(QueryTest, ImplicitOrderByInMultipleInequality) { .AddingFilter(testutil::Filter("1", ">", 5)) .AddingFilter(testutil::Filter("19", "<=", 5)) .AddingFilter(testutil::Filter("2", ">=", 5)) - .order_bys(), + .normalized_order_bys(), (std::vector{ testutil::OrderBy("1", "asc"), testutil::OrderBy("19", "asc"), @@ -869,7 +869,7 @@ TEST(QueryTest, ImplicitOrderByInMultipleInequality) { ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) .AddingFilter(testutil::Filter("aa", ">", 5)) .AddingFilter(testutil::Filter("a.a", "<=", 5)) - .order_bys(), + .normalized_order_bys(), (std::vector{ testutil::OrderBy("a", "asc"), testutil::OrderBy("a.a", "asc"), @@ -881,7 +881,7 @@ TEST(QueryTest, ImplicitOrderByInMultipleInequality) { ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) .AddingFilter(testutil::Filter("_a", ">", 5)) .AddingFilter(testutil::Filter("a.a", "<=", 5)) - .order_bys(), + .normalized_order_bys(), (std::vector{ testutil::OrderBy("_a", "asc"), testutil::OrderBy("a", "asc"), @@ -893,7 +893,7 @@ TEST(QueryTest, ImplicitOrderByInMultipleInequality) { ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) .AddingFilter(testutil::Filter("a.z", ">", 5)) .AddingFilter(testutil::Filter(("`a.a`"), "<=", 5)) - .order_bys(), + .normalized_order_bys(), (std::vector{ testutil::OrderBy("a", "asc"), testutil::OrderBy("a.z", "asc"), @@ -908,7 +908,7 @@ TEST(QueryTest, ImplicitOrderByInMultipleInequality) { testutil::Filter("c", "<=", 0)}), OrFilters({testutil::Filter("d", ">", 3), testutil::Filter("e", "==", 2)})})) - .order_bys(), + .normalized_order_bys(), (std::vector{ testutil::OrderBy("a", "asc"), testutil::OrderBy("b", "asc"), From e5d926e1cc89a4fa33663371354679491aa9a83a Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 11 Aug 2023 16:36:07 -0400 Subject: [PATCH 13/46] Update CHANGELOG.md --- Firestore/CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index 5ac9d0828e8..81639bfd0bf 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,3 +1,6 @@ +# Unreleased +- [feature] Added support for having multiple inequality fields in compound queries. (#11626) + # 10.12.0 - [feature] Implemented an optimization in the local cache synchronization logic that reduces the number of billed document reads when documents were deleted @@ -12,7 +15,6 @@ ``` To go back to using the binary distribution of Firestore, quit Xcode and open Xcode like normal, without the environment variable. (#11492) -- [feature] Added support for having multiple inequality fields in compound queries. (#11626) # 10.11.0 - [feature] Expose MultiDb API for public preview. (#10465) From e0b09a9ded9bc2d3f3e2dec321a1cbd0934825fa Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 17 Aug 2023 13:33:16 -0400 Subject: [PATCH 14/46] add more unit tests for orderby normalization --- Firestore/core/test/unit/core/query_test.cc | 39 +++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/Firestore/core/test/unit/core/query_test.cc b/Firestore/core/test/unit/core/query_test.cc index eea99384201..c71590a8f33 100644 --- a/Firestore/core/test/unit/core/query_test.cc +++ b/Firestore/core/test/unit/core/query_test.cc @@ -839,6 +839,7 @@ TEST(QueryTest, ImplicitOrderBy) { TEST(QueryTest, ImplicitOrderByInMultipleInequality) { auto base_query = testutil::Query("foo"); ASSERT_EQ(base_query.AddingFilter(testutil::Filter("a", "<", 5)) + .AddingFilter(testutil::Filter("a", ">=", 5)) .AddingFilter(testutil::Filter("aa", ">", 5)) .AddingFilter(testutil::Filter("b", "<=", 5)) .AddingFilter(testutil::Filter("A", ">=", 5)) @@ -916,6 +917,44 @@ TEST(QueryTest, ImplicitOrderByInMultipleInequality) { testutil::OrderBy("d", "asc"), testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), })); + + // OrderBy + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("b", "<", 5)) + .AddingFilter(testutil::Filter("a", ">", 5)) + .AddingFilter(testutil::Filter(("z"), "<=", 5)) + .AddingOrderBy(testutil::OrderBy("z")) + .normalized_order_bys(), + (std::vector{ + testutil::OrderBy("z", "asc"), + testutil::OrderBy("a", "asc"), + testutil::OrderBy("b", "asc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), + })); + + // last explicit order by direction + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("b", "<", 5)) + .AddingFilter(testutil::Filter("a", ">", 5)) + .AddingOrderBy(testutil::OrderBy("z", "desc")) + .normalized_order_bys(), + (std::vector{ + testutil::OrderBy("z", "desc"), + testutil::OrderBy("a", "desc"), + testutil::OrderBy("b", "desc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "desc"), + })); + + ASSERT_EQ(base_query.AddingFilter(testutil::Filter("b", "<", 5)) + .AddingFilter(testutil::Filter("a", ">", 5)) + .AddingOrderBy(testutil::OrderBy("z", "desc")) + .AddingOrderBy(testutil::OrderBy("c")) + .normalized_order_bys(), + (std::vector{ + testutil::OrderBy("z", "desc"), + testutil::OrderBy("c", "asc"), + testutil::OrderBy("a", "asc"), + testutil::OrderBy("b", "asc"), + testutil::OrderBy(FieldPath::kDocumentKeyPath, "asc"), + })); } MATCHER_P(HasCanonicalId, expected, "") { From bc0759d08726209fd6e4d8ab4aa6740a46214fd6 Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Wed, 30 Aug 2023 18:22:41 -0400 Subject: [PATCH 15/46] CSI compatibility --- .../core/src/local/leveldb_index_manager.cc | 5 ++- .../core/src/model/target_index_matcher.cc | 16 +++++---- .../core/src/model/target_index_matcher.h | 11 +++++-- .../unit/local/leveldb_local_store_test.cc | 33 +++++++++++++++++++ .../unit/model/target_index_matcher_test.cc | 17 ++++++++-- 5 files changed, 70 insertions(+), 12 deletions(-) diff --git a/Firestore/core/src/local/leveldb_index_manager.cc b/Firestore/core/src/local/leveldb_index_manager.cc index ca8412744d0..eb41c79ad0b 100644 --- a/Firestore/core/src/local/leveldb_index_manager.cc +++ b/Firestore/core/src/local/leveldb_index_manager.cc @@ -480,7 +480,10 @@ void LevelDbIndexManager::CreateTargetIndexes(const core::Target& target) { if (type == IndexManager::IndexType::NONE || type == IndexManager::IndexType::PARTIAL) { TargetIndexMatcher targetIndexMatcher(subTarget); - AddFieldIndex(targetIndexMatcher.BuildTargetIndex()); + auto const field_index = targetIndexMatcher.BuildTargetIndex(); + if (field_index.has_value()) { + AddFieldIndex(field_index.value()); + } } } } diff --git a/Firestore/core/src/model/target_index_matcher.cc b/Firestore/core/src/model/target_index_matcher.cc index 01c84fb50b1..803dcc460c5 100644 --- a/Firestore/core/src/model/target_index_matcher.cc +++ b/Firestore/core/src/model/target_index_matcher.cc @@ -51,6 +51,11 @@ bool TargetIndexMatcher::ServedByIndex(const model::FieldIndex& index) { HARD_ASSERT(index.collection_group() == collection_id_, "Collection IDs do not match"); + if (HasMultipleInequality()) { + // Only single inequality is supported for now. + return false; + } + // If there is an array element, find a matching filter. const auto& array_segment = index.GetArraySegment(); if (array_segment.has_value() && @@ -87,11 +92,6 @@ bool TargetIndexMatcher::ServedByIndex(const model::FieldIndex& index) { auto order_by_iter = order_bys_.begin(); if (!inequality_filters_.empty()) { - if (inequality_filters_.size() > 1) { - // Only single inequality is supported for now. - return false; - } - // Only a single inequality is currently supported. Get the only entry in // the set. const FieldFilter& inequality_filter = *inequality_filters_.begin(); @@ -123,7 +123,11 @@ bool TargetIndexMatcher::ServedByIndex(const model::FieldIndex& index) { return true; } -model::FieldIndex TargetIndexMatcher::BuildTargetIndex() { +absl::optional TargetIndexMatcher::BuildTargetIndex() { + if (HasMultipleInequality()) { + return {}; + } + // We want to make sure only one segment created for one field. For example, // in case like a == 3 and a > 2, Index: {a ASCENDING} will only be created // once. diff --git a/Firestore/core/src/model/target_index_matcher.h b/Firestore/core/src/model/target_index_matcher.h index 16f9a334345..f76b8092a0b 100644 --- a/Firestore/core/src/model/target_index_matcher.h +++ b/Firestore/core/src/model/target_index_matcher.h @@ -55,6 +55,10 @@ class TargetIndexMatcher { public: explicit TargetIndexMatcher(const core::Target& target); + inline bool HasMultipleInequality() const { + return inequality_filters_.size() > 1U; + } + /** * Returns whether the index can be used to serve the TargetIndexMatcher's * target. @@ -78,8 +82,11 @@ class TargetIndexMatcher { */ bool ServedByIndex(const model::FieldIndex& index); - /** Returns a full matched field index for this target. */ - model::FieldIndex BuildTargetIndex(); + /** + * Returns a full matched field index for this target. Currently multiple + * inequality query is not supported so function returns null. + */ + absl::optional BuildTargetIndex(); private: bool HasMatchingEqualityFilter(const model::Segment& segment); diff --git a/Firestore/core/test/unit/local/leveldb_local_store_test.cc b/Firestore/core/test/unit/local/leveldb_local_store_test.cc index fb233f12fd3..039cd5cc264 100644 --- a/Firestore/core/test/unit/local/leveldb_local_store_test.cc +++ b/Firestore/core/test/unit/local/leveldb_local_store_test.cc @@ -764,6 +764,39 @@ TEST_F(LevelDbLocalStoreTest, IndexAutoCreationWorksWithMutation) { FSTAssertQueryReturned("coll/a", "coll/f"); } +TEST_F(LevelDbLocalStoreTest, IndexAutoCreationDoesnotWorkWithMultipleInequality) { + core::Query query = testutil::Query("coll").AddingFilter(Filter("field1", "<", 5)).AddingFilter(Filter("field2", "<", 5)); + int target_id = AllocateQuery(query); + + SetIndexAutoCreationEnabled(true); + SetMinCollectionSizeToAutoCreateIndex(0); + SetRelativeIndexReadCostPerDocument(2); + + ApplyRemoteEvent(AddedRemoteEvent( + Doc("coll/a", 10, Map("field1", 1, "field2", 2)), {target_id})); + ApplyRemoteEvent( + AddedRemoteEvent(Doc("coll/b", 10, Map("field1", 8, "field2", 2)), {target_id})); + ApplyRemoteEvent( + AddedRemoteEvent(Doc("coll/c", 10, Map("field1", "string", "field2", 2)), {target_id})); + ApplyRemoteEvent(AddedRemoteEvent( + Doc("coll/d", 10, Map("field1", 2)), {target_id})); + ApplyRemoteEvent(AddedRemoteEvent( + Doc("coll/e", 10, Map("field1", 4, "field2", 4)), {target_id})); + + // First time query is running without indexes. + // Based on current heuristic, collection document counts (5) > 2 * resultSize (2). + // Full matched index will not be created since FieldIndex does not support multiple inequality. + ExecuteQuery(query); + FSTAssertRemoteDocumentsRead(/* byKey= */ 0, /* byCollection= */ 2); + FSTAssertQueryReturned("coll/a", "coll/e"); + + BackfillIndexes(); + + ExecuteQuery(query); + FSTAssertRemoteDocumentsRead(/* byKey= */ 0, /* byCollection= */ 2); + FSTAssertQueryReturned("coll/a", "coll/e"); +} + } // namespace local } // namespace firestore } // namespace firebase diff --git a/Firestore/core/test/unit/model/target_index_matcher_test.cc b/Firestore/core/test/unit/model/target_index_matcher_test.cc index f07b7bfc66d..ae8edc766a6 100644 --- a/Firestore/core/test/unit/model/target_index_matcher_test.cc +++ b/Firestore/core/test/unit/model/target_index_matcher_test.cc @@ -139,10 +139,12 @@ void ValidateDoesNotServeTarget(const core::Query& query, void ValidateBuildTargetIndexCreateFullMatchIndex(const core::Query& query) { const core::Target& target = query.ToTarget(); TargetIndexMatcher matcher(target); - FieldIndex expected_index = matcher.BuildTargetIndex(); - EXPECT_TRUE(matcher.ServedByIndex(expected_index)); + EXPECT_FALSE(matcher.HasMultipleInequality()); + absl::optional expected_index = matcher.BuildTargetIndex(); + EXPECT_TRUE(expected_index.has_value()); + EXPECT_TRUE(matcher.ServedByIndex(expected_index.value())); // Check the index created is a FULL MATCH index - EXPECT_TRUE(expected_index.segments().size() >= target.GetSegmentCount()); + EXPECT_TRUE(expected_index.value().segments().size() >= target.GetSegmentCount()); } TEST(TargetIndexMatcher, CanUseMergeJoin) { @@ -728,6 +730,15 @@ TEST(TargetIndexMatcher, BuildTargetIndexWithInAndOrderBySameField) { ValidateBuildTargetIndexCreateFullMatchIndex(query); } +TEST(TargetIndexMatcher, BuildTargetIndexReturnsNullForMultipleInequality) { + auto query = testutil::Query("collId").AddingFilter(Filter("a", ">=", 1)).AddingFilter(Filter("b", "<=", 10)); + const core::Target& target = query.ToTarget(); + TargetIndexMatcher matcher(target); + EXPECT_TRUE(matcher.HasMultipleInequality()); + absl::optional expected_index = matcher.BuildTargetIndex(); + EXPECT_FALSE(expected_index.has_value()); +} + } // namespace } // namespace model } // namespace firestore From 49898b62fa909a239c749c3aa2982c7b247c3d2d Mon Sep 17 00:00:00 2001 From: cherylEnkidu Date: Thu, 31 Aug 2023 14:02:06 -0400 Subject: [PATCH 16/46] Address feedback get from web review --- .../core/src/model/target_index_matcher.cc | 1 + .../unit/local/leveldb_local_store_test.cc | 22 +++++++++++-------- .../unit/model/target_index_matcher_test.cc | 17 ++++++++------ 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/Firestore/core/src/model/target_index_matcher.cc b/Firestore/core/src/model/target_index_matcher.cc index 803dcc460c5..83e94d4793c 100644 --- a/Firestore/core/src/model/target_index_matcher.cc +++ b/Firestore/core/src/model/target_index_matcher.cc @@ -53,6 +53,7 @@ bool TargetIndexMatcher::ServedByIndex(const model::FieldIndex& index) { if (HasMultipleInequality()) { // Only single inequality is supported for now. + // TODO(Add support for multiple inequality query): b/298441043 return false; } diff --git a/Firestore/core/test/unit/local/leveldb_local_store_test.cc b/Firestore/core/test/unit/local/leveldb_local_store_test.cc index 039cd5cc264..85e4286698b 100644 --- a/Firestore/core/test/unit/local/leveldb_local_store_test.cc +++ b/Firestore/core/test/unit/local/leveldb_local_store_test.cc @@ -764,8 +764,11 @@ TEST_F(LevelDbLocalStoreTest, IndexAutoCreationWorksWithMutation) { FSTAssertQueryReturned("coll/a", "coll/f"); } -TEST_F(LevelDbLocalStoreTest, IndexAutoCreationDoesnotWorkWithMultipleInequality) { - core::Query query = testutil::Query("coll").AddingFilter(Filter("field1", "<", 5)).AddingFilter(Filter("field2", "<", 5)); +TEST_F(LevelDbLocalStoreTest, + IndexAutoCreationDoesnotWorkWithMultipleInequality) { + core::Query query = testutil::Query("coll") + .AddingFilter(Filter("field1", "<", 5)) + .AddingFilter(Filter("field2", "<", 5)); int target_id = AllocateQuery(query); SetIndexAutoCreationEnabled(true); @@ -774,18 +777,19 @@ TEST_F(LevelDbLocalStoreTest, IndexAutoCreationDoesnotWorkWithMultipleInequality ApplyRemoteEvent(AddedRemoteEvent( Doc("coll/a", 10, Map("field1", 1, "field2", 2)), {target_id})); - ApplyRemoteEvent( - AddedRemoteEvent(Doc("coll/b", 10, Map("field1", 8, "field2", 2)), {target_id})); - ApplyRemoteEvent( - AddedRemoteEvent(Doc("coll/c", 10, Map("field1", "string", "field2", 2)), {target_id})); ApplyRemoteEvent(AddedRemoteEvent( - Doc("coll/d", 10, Map("field1", 2)), {target_id})); + Doc("coll/b", 10, Map("field1", 8, "field2", 2)), {target_id})); + ApplyRemoteEvent(AddedRemoteEvent( + Doc("coll/c", 10, Map("field1", "string", "field2", 2)), {target_id})); + ApplyRemoteEvent( + AddedRemoteEvent(Doc("coll/d", 10, Map("field1", 2)), {target_id})); ApplyRemoteEvent(AddedRemoteEvent( Doc("coll/e", 10, Map("field1", 4, "field2", 4)), {target_id})); // First time query is running without indexes. - // Based on current heuristic, collection document counts (5) > 2 * resultSize (2). - // Full matched index will not be created since FieldIndex does not support multiple inequality. + // Based on current heuristic, collection document counts (5) > 2 * resultSize + // (2). Full matched index will not be created since FieldIndex does not + // support multiple inequality. ExecuteQuery(query); FSTAssertRemoteDocumentsRead(/* byKey= */ 0, /* byCollection= */ 2); FSTAssertQueryReturned("coll/a", "coll/e"); diff --git a/Firestore/core/test/unit/model/target_index_matcher_test.cc b/Firestore/core/test/unit/model/target_index_matcher_test.cc index ae8edc766a6..1e7cbc25f4d 100644 --- a/Firestore/core/test/unit/model/target_index_matcher_test.cc +++ b/Firestore/core/test/unit/model/target_index_matcher_test.cc @@ -140,11 +140,12 @@ void ValidateBuildTargetIndexCreateFullMatchIndex(const core::Query& query) { const core::Target& target = query.ToTarget(); TargetIndexMatcher matcher(target); EXPECT_FALSE(matcher.HasMultipleInequality()); - absl::optional expected_index = matcher.BuildTargetIndex(); - EXPECT_TRUE(expected_index.has_value()); - EXPECT_TRUE(matcher.ServedByIndex(expected_index.value())); + absl::optional actual_index = matcher.BuildTargetIndex(); + EXPECT_TRUE(actual_index.has_value()); + EXPECT_TRUE(matcher.ServedByIndex(actual_index.value())); // Check the index created is a FULL MATCH index - EXPECT_TRUE(expected_index.value().segments().size() >= target.GetSegmentCount()); + EXPECT_TRUE(actual_index.value().segments().size() >= + target.GetSegmentCount()); } TEST(TargetIndexMatcher, CanUseMergeJoin) { @@ -731,12 +732,14 @@ TEST(TargetIndexMatcher, BuildTargetIndexWithInAndOrderBySameField) { } TEST(TargetIndexMatcher, BuildTargetIndexReturnsNullForMultipleInequality) { - auto query = testutil::Query("collId").AddingFilter(Filter("a", ">=", 1)).AddingFilter(Filter("b", "<=", 10)); + auto query = testutil::Query("collId") + .AddingFilter(Filter("a", ">=", 1)) + .AddingFilter(Filter("b", "<=", 10)); const core::Target& target = query.ToTarget(); TargetIndexMatcher matcher(target); EXPECT_TRUE(matcher.HasMultipleInequality()); - absl::optional expected_index = matcher.BuildTargetIndex(); - EXPECT_FALSE(expected_index.has_value()); + absl::optional actual_index = matcher.BuildTargetIndex(); + EXPECT_FALSE(actual_index.has_value()); } } // namespace From a573b6add9cecb3544ec787f753f16e96afbb18e Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Thu, 31 Aug 2023 18:50:28 +0000 Subject: [PATCH 17/46] resolve comments --- Firestore/core/src/model/target_index_matcher.cc | 2 +- Firestore/core/test/unit/model/target_index_matcher_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/core/src/model/target_index_matcher.cc b/Firestore/core/src/model/target_index_matcher.cc index 66aca51ff33..8a4d3aac851 100644 --- a/Firestore/core/src/model/target_index_matcher.cc +++ b/Firestore/core/src/model/target_index_matcher.cc @@ -40,7 +40,7 @@ TargetIndexMatcher::TargetIndexMatcher(const core::Target& target) { for (const Filter& filter : target.filters()) { FieldFilter field_filter(filter); if (field_filter.IsInequality()) { - inequality_filters_.emplace(field_filter); + inequality_filters_.insert(field_filter); } else { equality_filters_.push_back(field_filter); } diff --git a/Firestore/core/test/unit/model/target_index_matcher_test.cc b/Firestore/core/test/unit/model/target_index_matcher_test.cc index 1e7cbc25f4d..f94644882e3 100644 --- a/Firestore/core/test/unit/model/target_index_matcher_test.cc +++ b/Firestore/core/test/unit/model/target_index_matcher_test.cc @@ -141,7 +141,7 @@ void ValidateBuildTargetIndexCreateFullMatchIndex(const core::Query& query) { TargetIndexMatcher matcher(target); EXPECT_FALSE(matcher.HasMultipleInequality()); absl::optional actual_index = matcher.BuildTargetIndex(); - EXPECT_TRUE(actual_index.has_value()); + ASSERT_TRUE(actual_index.has_value()); EXPECT_TRUE(matcher.ServedByIndex(actual_index.value())); // Check the index created is a FULL MATCH index EXPECT_TRUE(actual_index.value().segments().size() >= From 3caa241e4d13cca38f5890aad87c36ecc316e114 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 5 Sep 2023 16:33:06 -0400 Subject: [PATCH 18/46] use call_once() to fix the data race on Filter::Rep::memoized_flattened_filters_ --- Firestore/core/src/core/composite_filter.cc | 8 ++++---- Firestore/core/src/core/field_filter.cc | 8 ++++---- Firestore/core/src/core/filter.cc | 12 ++++++++++++ Firestore/core/src/core/filter.h | 16 ++++++++++++++-- 4 files changed, 34 insertions(+), 10 deletions(-) diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index 63491d1209c..871f10787a7 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -143,14 +143,14 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter( const std::vector& CompositeFilter::Rep::GetFlattenedFilters() const { - if (Filter::Rep::memoized_flattened_filters_.empty() && !filters().empty()) { + std::call_once(memoized_flattened_filters_->once, [this]() { for (const auto& filter : filters()) { std::copy(filter.GetFlattenedFilters().begin(), filter.GetFlattenedFilters().end(), - std::back_inserter(Filter::Rep::memoized_flattened_filters_)); + std::back_inserter(this->memoized_flattened_filters_->filters)); } - } - return Filter::Rep::memoized_flattened_filters_; + }); + return memoized_flattened_filters_->filters; } } // namespace core diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index 14441486ac9..0240500cddc 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -124,11 +124,11 @@ FieldFilter::FieldFilter(std::shared_ptr rep) const std::vector& FieldFilter::Rep::GetFlattenedFilters() const { // This is already a field filter, so we return a vector of size one. - if (Filter::Rep::memoized_flattened_filters_.empty()) { - Filter::Rep::memoized_flattened_filters_ = std::vector{ + std::call_once(memoized_flattened_filters_->once, [this]() { + memoized_flattened_filters_->filters = std::vector{ FieldFilter(std::make_shared(*this))}; - } - return Filter::Rep::memoized_flattened_filters_; + }); + return memoized_flattened_filters_->filters; } std::vector FieldFilter::Rep::GetFilters() const { diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index e180a8ce568..e2b3755125b 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -16,6 +16,8 @@ #include "Firestore/core/src/core/filter.h" +#include "Firestore/core/src/core/field_filter.h" + #include namespace firebase { @@ -32,6 +34,16 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) { return os << filter.ToString(); } +Filter::Rep::Rep() : memoized_flattened_filters_(std::make_shared()) { +} + +Filter::Rep::~Rep() { + // Add a comment about WHY this call_once here is necessary (i.e. so that the + // destructor synchronizes with some other thread that potentially won the + // race to set memoized_flattened_filters_.filters). + std::call_once(memoized_flattened_filters_->once, []{}); +} + } // namespace core } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 164ecb828a4..5ca5b5f01dc 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -114,7 +115,8 @@ class Filter { protected: class Rep { public: - virtual ~Rep() = default; + Rep(); + virtual ~Rep(); virtual Type type() const { return Type::kFilter; @@ -149,11 +151,21 @@ class Filter { virtual std::vector GetFilters() const = 0; + struct MemoizedFlattenedFilters { + std::once_flag once; + std::vector filters; + }; + /** * Memoized list of all field filters that can be found by * traversing the tree of filters contained in this composite filter. + * + * Use a std::shared_ptr rather than using + * MemoizedFlattenedFilters directly so that it is copyable + * (MemoizedFlattenedFilters is not copyable because of its std::once_flag + * member variable). */ - mutable std::vector memoized_flattened_filters_; + mutable std::shared_ptr memoized_flattened_filters_; }; explicit Filter(std::shared_ptr&& rep) : rep_(rep) { From 6dee927d93b13888de7f9efa272ccf1b1c057efb Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 5 Sep 2023 16:47:04 -0400 Subject: [PATCH 19/46] format --- Firestore/core/src/core/filter.cc | 6 ++++-- Firestore/core/src/core/filter.h | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index e2b3755125b..2f1028acfb7 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -34,14 +34,16 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) { return os << filter.ToString(); } -Filter::Rep::Rep() : memoized_flattened_filters_(std::make_shared()) { +Filter::Rep::Rep() + : memoized_flattened_filters_( + std::make_shared()) { } Filter::Rep::~Rep() { // Add a comment about WHY this call_once here is necessary (i.e. so that the // destructor synchronizes with some other thread that potentially won the // race to set memoized_flattened_filters_.filters). - std::call_once(memoized_flattened_filters_->once, []{}); + std::call_once(memoized_flattened_filters_->once, [] {}); } } // namespace core diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 5ca5b5f01dc..2dde738e0ab 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -165,7 +165,8 @@ class Filter { * (MemoizedFlattenedFilters is not copyable because of its std::once_flag * member variable). */ - mutable std::shared_ptr memoized_flattened_filters_; + mutable std::shared_ptr + memoized_flattened_filters_; }; explicit Filter(std::shared_ptr&& rep) : rep_(rep) { From e1709c4ee388513e610489fc033282e653ca56a4 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 5 Sep 2023 21:53:18 -0400 Subject: [PATCH 20/46] fix broken test by adding the new ThreadSafeMemoizer class --- Firestore/core/src/core/composite_filter.cc | 5 ++-- Firestore/core/src/core/field_filter.cc | 6 ++--- Firestore/core/src/core/filter.cc | 20 +++++++++------- Firestore/core/src/core/filter.h | 26 ++++++++++++--------- 4 files changed, 31 insertions(+), 26 deletions(-) diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index 871f10787a7..2fd379cf75e 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -143,14 +143,13 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter( const std::vector& CompositeFilter::Rep::GetFlattenedFilters() const { - std::call_once(memoized_flattened_filters_->once, [this]() { + return memoized_flattened_filters_->memoize([&](std::vector& flattened_filters) { for (const auto& filter : filters()) { std::copy(filter.GetFlattenedFilters().begin(), filter.GetFlattenedFilters().end(), - std::back_inserter(this->memoized_flattened_filters_->filters)); + std::back_inserter(flattened_filters)); } }); - return memoized_flattened_filters_->filters; } } // namespace core diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index 0240500cddc..e5e7e107935 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -124,11 +124,9 @@ FieldFilter::FieldFilter(std::shared_ptr rep) const std::vector& FieldFilter::Rep::GetFlattenedFilters() const { // This is already a field filter, so we return a vector of size one. - std::call_once(memoized_flattened_filters_->once, [this]() { - memoized_flattened_filters_->filters = std::vector{ - FieldFilter(std::make_shared(*this))}; + return memoized_flattened_filters_->memoize([&](std::vector& flattened_filters) { + flattened_filters = std::vector{FieldFilter(std::make_shared(*this))}; }); - return memoized_flattened_filters_->filters; } std::vector FieldFilter::Rep::GetFilters() const { diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index 2f1028acfb7..9393241b4f5 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -34,16 +34,20 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) { return os << filter.ToString(); } -Filter::Rep::Rep() - : memoized_flattened_filters_( - std::make_shared()) { +Filter::Rep::Rep() : memoized_flattened_filters_(std::make_shared()) { } -Filter::Rep::~Rep() { - // Add a comment about WHY this call_once here is necessary (i.e. so that the - // destructor synchronizes with some other thread that potentially won the - // race to set memoized_flattened_filters_.filters). - std::call_once(memoized_flattened_filters_->once, [] {}); +Filter::Rep::ThreadSafeMemoizer::~ThreadSafeMemoizer() { + // Call `std::call_once` in order to synchronize with the thread that called + // `memoize()` that actually populated `filters_`. Without this invocation, + // there is a data race between the destructor destroying `filters_` and the + // thread whose `memoize()` call populated `filters_`. + std::call_once(once_, [&](){}); +} + +const std::vector& Filter::Rep::ThreadSafeMemoizer::memoize(std::function&)> func) { + std::call_once(once_, [&]() { func(filters_); }); + return filters_; } } // namespace core diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 2dde738e0ab..ad166bff2b7 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -17,9 +17,9 @@ #ifndef FIRESTORE_CORE_SRC_CORE_FILTER_H_ #define FIRESTORE_CORE_SRC_CORE_FILTER_H_ +#include #include #include -#include #include #include @@ -116,7 +116,7 @@ class Filter { class Rep { public: Rep(); - virtual ~Rep(); + virtual ~Rep() = default; virtual Type type() const { return Type::kFilter; @@ -151,22 +151,26 @@ class Filter { virtual std::vector GetFilters() const = 0; - struct MemoizedFlattenedFilters { - std::once_flag once; - std::vector filters; + class ThreadSafeMemoizer { + public: + ~ThreadSafeMemoizer(); + const std::vector& memoize(std::function&)>); + + private: + std::once_flag once_; + std::vector filters_; }; /** * Memoized list of all field filters that can be found by * traversing the tree of filters contained in this composite filter. * - * Use a std::shared_ptr rather than using - * MemoizedFlattenedFilters directly so that it is copyable - * (MemoizedFlattenedFilters is not copyable because of its std::once_flag - * member variable). + * Use a `std::shared_ptr` rather than using + * `ThreadSafeMemoizer` directly so that it is copyable + * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` + * member variable, which is not copyable). */ - mutable std::shared_ptr - memoized_flattened_filters_; + mutable std::shared_ptr memoized_flattened_filters_; }; explicit Filter(std::shared_ptr&& rep) : rep_(rep) { From 781675d44e90b0818a0bd475244c6eb27942c406 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 5 Sep 2023 21:56:06 -0400 Subject: [PATCH 21/46] ./scripts/style.sh --- Firestore/core/src/core/composite_filter.cc | 15 ++++++++------- Firestore/core/src/core/field_filter.cc | 8 +++++--- Firestore/core/src/core/filter.cc | 8 +++++--- Firestore/core/src/core/filter.h | 15 ++++++++------- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index 2fd379cf75e..3d131e67887 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -143,13 +143,14 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter( const std::vector& CompositeFilter::Rep::GetFlattenedFilters() const { - return memoized_flattened_filters_->memoize([&](std::vector& flattened_filters) { - for (const auto& filter : filters()) { - std::copy(filter.GetFlattenedFilters().begin(), - filter.GetFlattenedFilters().end(), - std::back_inserter(flattened_filters)); - } - }); + return memoized_flattened_filters_->memoize( + [&](std::vector& flattened_filters) { + for (const auto& filter : filters()) { + std::copy(filter.GetFlattenedFilters().begin(), + filter.GetFlattenedFilters().end(), + std::back_inserter(flattened_filters)); + } + }); } } // namespace core diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index e5e7e107935..8a386281c2c 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -124,9 +124,11 @@ FieldFilter::FieldFilter(std::shared_ptr rep) const std::vector& FieldFilter::Rep::GetFlattenedFilters() const { // This is already a field filter, so we return a vector of size one. - return memoized_flattened_filters_->memoize([&](std::vector& flattened_filters) { - flattened_filters = std::vector{FieldFilter(std::make_shared(*this))}; - }); + return memoized_flattened_filters_->memoize( + [&](std::vector& flattened_filters) { + flattened_filters = std::vector{ + FieldFilter(std::make_shared(*this))}; + }); } std::vector FieldFilter::Rep::GetFilters() const { diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index 9393241b4f5..525639f75fd 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -34,7 +34,8 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) { return os << filter.ToString(); } -Filter::Rep::Rep() : memoized_flattened_filters_(std::make_shared()) { +Filter::Rep::Rep() + : memoized_flattened_filters_(std::make_shared()) { } Filter::Rep::ThreadSafeMemoizer::~ThreadSafeMemoizer() { @@ -42,10 +43,11 @@ Filter::Rep::ThreadSafeMemoizer::~ThreadSafeMemoizer() { // `memoize()` that actually populated `filters_`. Without this invocation, // there is a data race between the destructor destroying `filters_` and the // thread whose `memoize()` call populated `filters_`. - std::call_once(once_, [&](){}); + std::call_once(once_, [&]() {}); } -const std::vector& Filter::Rep::ThreadSafeMemoizer::memoize(std::function&)> func) { +const std::vector& Filter::Rep::ThreadSafeMemoizer::memoize( + std::function&)> func) { std::call_once(once_, [&]() { func(filters_); }); return filters_; } diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index ad166bff2b7..76c344f4aab 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -152,13 +152,14 @@ class Filter { virtual std::vector GetFilters() const = 0; class ThreadSafeMemoizer { - public: - ~ThreadSafeMemoizer(); - const std::vector& memoize(std::function&)>); - - private: - std::once_flag once_; - std::vector filters_; + public: + ~ThreadSafeMemoizer(); + const std::vector& memoize( + std::function&)>); + + private: + std::once_flag once_; + std::vector filters_; }; /** From 1a3769776b5fad2ac4946692e233df0eb7cd8302 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 6 Sep 2023 01:01:02 -0400 Subject: [PATCH 22/46] filter.h: add missing #include --- Firestore/core/src/core/filter.h | 1 + 1 file changed, 1 insertion(+) diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 76c344f4aab..6f540f38d53 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include From 3395ff9e7572e8242c222df532671c39ff8a8ce0 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 6 Sep 2023 02:00:51 -0400 Subject: [PATCH 23/46] filter.h: add NOLINT(build/c++11) comment to the line #include --- Firestore/core/src/core/filter.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 6f540f38d53..7874f1ddf1e 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -20,7 +20,7 @@ #include #include #include -#include +#include // NOLINT(build/c++11) #include #include From 2c7a5358ae495fe515599ad660d8e5b9348ffed1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 6 Sep 2023 02:01:45 -0400 Subject: [PATCH 24/46] filter.cc: fix #include order for lint --- Firestore/core/src/core/filter.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index 525639f75fd..ffbae842217 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -16,10 +16,10 @@ #include "Firestore/core/src/core/filter.h" -#include "Firestore/core/src/core/field_filter.h" - #include +#include "Firestore/core/src/core/field_filter.h" + namespace firebase { namespace firestore { namespace core { From e672782d1e67c3cf40db5e13f48911c1447feb60 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 6 Sep 2023 02:22:31 -0400 Subject: [PATCH 25/46] ThreadSafeMemoizer cleanup --- Firestore/core/src/core/composite_filter.cc | 16 +++++++-------- Firestore/core/src/core/field_filter.cc | 9 ++++----- Firestore/core/src/core/filter.cc | 12 +++++------ Firestore/core/src/core/filter.h | 22 +++++++++++++++++++-- 4 files changed, 38 insertions(+), 21 deletions(-) diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index 3d131e67887..7bbb81abc57 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -143,14 +143,14 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter( const std::vector& CompositeFilter::Rep::GetFlattenedFilters() const { - return memoized_flattened_filters_->memoize( - [&](std::vector& flattened_filters) { - for (const auto& filter : filters()) { - std::copy(filter.GetFlattenedFilters().begin(), - filter.GetFlattenedFilters().end(), - std::back_inserter(flattened_filters)); - } - }); + return memoized_flattened_filters_->memoize([&]() { + std::vector flattened_filters; + for (const auto& filter : filters()) + std::copy(filter.GetFlattenedFilters().begin(), + filter.GetFlattenedFilters().end(), + std::back_inserter(flattened_filters)); + return flattened_filters; + }); } } // namespace core diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index 8a386281c2c..b68956c8a52 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -124,11 +124,10 @@ FieldFilter::FieldFilter(std::shared_ptr rep) const std::vector& FieldFilter::Rep::GetFlattenedFilters() const { // This is already a field filter, so we return a vector of size one. - return memoized_flattened_filters_->memoize( - [&](std::vector& flattened_filters) { - flattened_filters = std::vector{ - FieldFilter(std::make_shared(*this))}; - }); + return memoized_flattened_filters_->memoize([&]() { + return std::vector{ + FieldFilter(std::make_shared(*this))}; + }); } std::vector FieldFilter::Rep::GetFilters() const { diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index ffbae842217..8977eafbc48 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -39,16 +39,16 @@ Filter::Rep::Rep() } Filter::Rep::ThreadSafeMemoizer::~ThreadSafeMemoizer() { - // Call `std::call_once` in order to synchronize with the thread that called - // `memoize()` that actually populated `filters_`. Without this invocation, - // there is a data race between the destructor destroying `filters_` and the - // thread whose `memoize()` call populated `filters_`. + // Call `std::call_once` in order to synchronize with the "active" invocation + // of `memoize()`. Without this synchronization, there is a data race between + // this destructor, which "reads" `filters_` to destroy it, and the write to + // `filters_` done by the "active" invocation of `memoize()`. std::call_once(once_, [&]() {}); } const std::vector& Filter::Rep::ThreadSafeMemoizer::memoize( - std::function&)> func) { - std::call_once(once_, [&]() { func(filters_); }); + std::function()> func) { + std::call_once(once_, [&]() { filters_ = func(); }); return filters_; } diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 7874f1ddf1e..eef30e36c05 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -152,11 +152,29 @@ class Filter { virtual std::vector GetFilters() const = 0; + /** + * Stores a memoized value in a manner that is safe to be shared between + * multiple threads. + */ class ThreadSafeMemoizer { public: ~ThreadSafeMemoizer(); + + /** + * Memoize a value. + * + * The std::function object specified by the first invocation of this + * function (the "active" invocation) will be invoked synchronously. + * None of the std::function objects specified by the subsequent + * invocations of this function (the "passive" invocations) will be + * invoked. All invocations, both "active" and "passive", will return a + * reference to the std::vector created by copying the return value from + * the std::function specified by the "active" invocation. It is, + * therefore, the "active" invocation's job to return the std::vector + * to memoize. + */ const std::vector& memoize( - std::function&)>); + std::function()>); private: std::once_flag once_; @@ -168,7 +186,7 @@ class Filter { * traversing the tree of filters contained in this composite filter. * * Use a `std::shared_ptr` rather than using - * `ThreadSafeMemoizer` directly so that it is copyable + * `ThreadSafeMemoizer` directly so that this class is copyable * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` * member variable, which is not copyable). */ From 3e02a4d909173525dea1097a1a42b3ae8d963d7f Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 6 Sep 2023 11:01:29 -0400 Subject: [PATCH 26/46] port Denver's code from MIEQ --- Firestore/core/src/core/composite_filter.cc | 13 ++++--- Firestore/core/src/core/field_filter.cc | 7 ++-- Firestore/core/src/core/filter.cc | 19 ++++++++++ Firestore/core/src/core/filter.h | 39 ++++++++++++++++++++- 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index 6b6ac7607b3..b0472539d0f 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -154,16 +154,15 @@ const model::FieldPath* CompositeFilter::Rep::GetFirstInequalityField() const { const std::vector& CompositeFilter::Rep::GetFlattenedFilters() const { - if (Filter::Rep::memoized_flattened_filters_.empty() && !filters().empty()) { - for (const auto& filter : filters()) { + return memoized_flattened_filters_->memoize([&]() { + std::vector flattened_filters; + for (const auto& filter : filters()) std::copy(filter.GetFlattenedFilters().begin(), filter.GetFlattenedFilters().end(), - std::back_inserter(Filter::Rep::memoized_flattened_filters_)); - } - } - return Filter::Rep::memoized_flattened_filters_; + std::back_inserter(flattened_filters)); + return flattened_filters; + }); } - } // namespace core } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/core/field_filter.cc b/Firestore/core/src/core/field_filter.cc index b6686465501..eb882e0db77 100644 --- a/Firestore/core/src/core/field_filter.cc +++ b/Firestore/core/src/core/field_filter.cc @@ -124,11 +124,10 @@ FieldFilter::FieldFilter(std::shared_ptr rep) const std::vector& FieldFilter::Rep::GetFlattenedFilters() const { // This is already a field filter, so we return a vector of size one. - if (Filter::Rep::memoized_flattened_filters_.empty()) { - Filter::Rep::memoized_flattened_filters_ = std::vector{ + return memoized_flattened_filters_->memoize([&]() { + return std::vector{ FieldFilter(std::make_shared(*this))}; - } - return Filter::Rep::memoized_flattened_filters_; + }); } std::vector FieldFilter::Rep::GetFilters() const { diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index e180a8ce568..2151731f53a 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -15,6 +15,7 @@ */ #include "Firestore/core/src/core/filter.h" +#include "Firestore/core/src/core/field_filter.h" #include @@ -32,6 +33,24 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) { return os << filter.ToString(); } +Filter::Rep::Rep() + : memoized_flattened_filters_(std::make_shared()) { +} + +Filter::Rep::ThreadSafeMemoizer::~ThreadSafeMemoizer() { + // Call `std::call_once` in order to synchronize with the "active" invocation + // of `memoize()`. Without this synchronization, there is a data race between + // this destructor, which "reads" `filters_` to destroy it, and the write to + // `filters_` done by the "active" invocation of `memoize()`. + std::call_once(once_, [&]() {}); +} + +const std::vector& Filter::Rep::ThreadSafeMemoizer::memoize( + std::function()> func) { + std::call_once(once_, [&]() { filters_ = func(); }); + return filters_; +} + } // namespace core } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 1785ff5a9d0..66cc33d47ce 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -19,6 +19,7 @@ #include #include +#include // NOLINT(build/c++11) #include #include @@ -122,6 +123,8 @@ class Filter { protected: class Rep { public: + Rep(); + virtual ~Rep() = default; virtual Type type() const { @@ -159,11 +162,45 @@ class Filter { virtual std::vector GetFilters() const = 0; + /** + * Stores a memoized value in a manner that is safe to be shared between + * multiple threads. + */ + class ThreadSafeMemoizer { + public: + ~ThreadSafeMemoizer(); + + /** + * Memoize a value. + * + * The std::function object specified by the first invocation of this + * function (the "active" invocation) will be invoked synchronously. + * None of the std::function objects specified by the subsequent + * invocations of this function (the "passive" invocations) will be + * invoked. All invocations, both "active" and "passive", will return a + * reference to the std::vector created by copying the return value from + * the std::function specified by the "active" invocation. It is, + * therefore, the "active" invocation's job to return the std::vector + * to memoize. + */ + const std::vector& memoize( + std::function()>); + + private: + std::once_flag once_; + std::vector filters_; + }; + /** * Memoized list of all field filters that can be found by * traversing the tree of filters contained in this composite filter. + * + * Use a `std::shared_ptr` rather than using + * `ThreadSafeMemoizer` directly so that this class is copyable + * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` + * member variable, which is not copyable). */ - mutable std::vector memoized_flattened_filters_; + mutable std::shared_ptr memoized_flattened_filters_; }; explicit Filter(std::shared_ptr&& rep) : rep_(rep) { From ad1537974805abefb5498a99babe61cb605c5b6a Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 6 Sep 2023 12:22:08 -0400 Subject: [PATCH 27/46] not working --- Firestore/core/src/core/filter.cc | 18 +----- Firestore/core/src/core/filter.h | 34 +---------- .../core/src/core/thread_safe_memoizer.cc | 41 +++++++++++++ .../core/src/core/thread_safe_memoizer.h | 61 +++++++++++++++++++ 4 files changed, 108 insertions(+), 46 deletions(-) create mode 100644 Firestore/core/src/core/thread_safe_memoizer.cc create mode 100644 Firestore/core/src/core/thread_safe_memoizer.h diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index 2151731f53a..d51c90ab773 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -16,6 +16,7 @@ #include "Firestore/core/src/core/filter.h" #include "Firestore/core/src/core/field_filter.h" +#include "Firestore/core/src/core/thread_safe_memoizer.h" #include @@ -34,21 +35,8 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) { } Filter::Rep::Rep() - : memoized_flattened_filters_(std::make_shared()) { -} - -Filter::Rep::ThreadSafeMemoizer::~ThreadSafeMemoizer() { - // Call `std::call_once` in order to synchronize with the "active" invocation - // of `memoize()`. Without this synchronization, there is a data race between - // this destructor, which "reads" `filters_` to destroy it, and the write to - // `filters_` done by the "active" invocation of `memoize()`. - std::call_once(once_, [&]() {}); -} - -const std::vector& Filter::Rep::ThreadSafeMemoizer::memoize( - std::function()> func) { - std::call_once(once_, [&]() { filters_ = func(); }); - return filters_; + : memoized_flattened_filters_( + std::make_shared>()) { } } // namespace core diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 66cc33d47ce..c1bdfc03764 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -23,6 +23,7 @@ #include #include +#include "Firestore/core/src/core/thread_safe_memoizer.h" #include "Firestore/core/src/model/model_fwd.h" namespace firebase { @@ -124,7 +125,6 @@ class Filter { class Rep { public: Rep(); - virtual ~Rep() = default; virtual Type type() const { @@ -162,35 +162,6 @@ class Filter { virtual std::vector GetFilters() const = 0; - /** - * Stores a memoized value in a manner that is safe to be shared between - * multiple threads. - */ - class ThreadSafeMemoizer { - public: - ~ThreadSafeMemoizer(); - - /** - * Memoize a value. - * - * The std::function object specified by the first invocation of this - * function (the "active" invocation) will be invoked synchronously. - * None of the std::function objects specified by the subsequent - * invocations of this function (the "passive" invocations) will be - * invoked. All invocations, both "active" and "passive", will return a - * reference to the std::vector created by copying the return value from - * the std::function specified by the "active" invocation. It is, - * therefore, the "active" invocation's job to return the std::vector - * to memoize. - */ - const std::vector& memoize( - std::function()>); - - private: - std::once_flag once_; - std::vector filters_; - }; - /** * Memoized list of all field filters that can be found by * traversing the tree of filters contained in this composite filter. @@ -200,7 +171,8 @@ class Filter { * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` * member variable, which is not copyable). */ - mutable std::shared_ptr memoized_flattened_filters_; + mutable std::shared_ptr> + memoized_flattened_filters_; }; explicit Filter(std::shared_ptr&& rep) : rep_(rep) { diff --git a/Firestore/core/src/core/thread_safe_memoizer.cc b/Firestore/core/src/core/thread_safe_memoizer.cc new file mode 100644 index 00000000000..4350d11977a --- /dev/null +++ b/Firestore/core/src/core/thread_safe_memoizer.cc @@ -0,0 +1,41 @@ +/* + * Copyright 2023 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "thread_safe_memoizer.h" + +namespace firebase { +namespace firestore { +namespace core { + +template +ThreadSafeMemoizer::~ThreadSafeMemoizer() { + // Call `std::call_once` in order to synchronize with the "active" invocation + // of `memoize()`. Without this synchronization, there is a data race between + // this destructor, which "reads" `filters_` to destroy it, and the write to + // `filters_` done by the "active" invocation of `memoize()`. + std::call_once(once_, [&]() {}); +} + +template +const std::vector& ThreadSafeMemoizer::memoize( + std::function()> func) { + std::call_once(once_, [&]() { filters_ = func(); }); + return filters_; +} + +} // namespace core +} // namespace firestore +} // namespace firebase diff --git a/Firestore/core/src/core/thread_safe_memoizer.h b/Firestore/core/src/core/thread_safe_memoizer.h new file mode 100644 index 00000000000..8cc0f84431a --- /dev/null +++ b/Firestore/core/src/core/thread_safe_memoizer.h @@ -0,0 +1,61 @@ +/* + * Copyright 2023 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef THREAD_SAFE_MEMOIZER_H_ +#define THREAD_SAFE_MEMOIZER_H_ + +#include +#include +#include + +namespace firebase { +namespace firestore { +namespace core { + +/** + * Stores a memoized value in a manner that is safe to be shared between + * multiple threads. + */ +template +class ThreadSafeMemoizer { + public: + ~ThreadSafeMemoizer(); + + /** + * Memoize a value. + * + * The std::function object specified by the first invocation of this + * function (the "active" invocation) will be invoked synchronously. + * None of the std::function objects specified by the subsequent + * invocations of this function (the "passive" invocations) will be + * invoked. All invocations, both "active" and "passive", will return a + * reference to the std::vector created by copying the return value from + * the std::function specified by the "active" invocation. It is, + * therefore, the "active" invocation's job to return the std::vector + * to memoize. + */ + const std::vector& memoize(std::function()>); + + private: + std::once_flag once_; + std::vector filters_; +}; + +} // namespace core +} // namespace firestore +} // namespace firebase + +#endif // THREAD_SAFE_MEMOIZER_H_ From 21ad6313df2575d83abe0b009f5443a3625523d8 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 6 Sep 2023 12:28:09 -0400 Subject: [PATCH 28/46] fix the compile error --- .../core/src/core/thread_safe_memoizer.cc | 41 ------------------- .../core/src/core/thread_safe_memoizer.h | 13 +++++- 2 files changed, 11 insertions(+), 43 deletions(-) delete mode 100644 Firestore/core/src/core/thread_safe_memoizer.cc diff --git a/Firestore/core/src/core/thread_safe_memoizer.cc b/Firestore/core/src/core/thread_safe_memoizer.cc deleted file mode 100644 index 4350d11977a..00000000000 --- a/Firestore/core/src/core/thread_safe_memoizer.cc +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright 2023 Google - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "thread_safe_memoizer.h" - -namespace firebase { -namespace firestore { -namespace core { - -template -ThreadSafeMemoizer::~ThreadSafeMemoizer() { - // Call `std::call_once` in order to synchronize with the "active" invocation - // of `memoize()`. Without this synchronization, there is a data race between - // this destructor, which "reads" `filters_` to destroy it, and the write to - // `filters_` done by the "active" invocation of `memoize()`. - std::call_once(once_, [&]() {}); -} - -template -const std::vector& ThreadSafeMemoizer::memoize( - std::function()> func) { - std::call_once(once_, [&]() { filters_ = func(); }); - return filters_; -} - -} // namespace core -} // namespace firestore -} // namespace firebase diff --git a/Firestore/core/src/core/thread_safe_memoizer.h b/Firestore/core/src/core/thread_safe_memoizer.h index 8cc0f84431a..949ea1c6dde 100644 --- a/Firestore/core/src/core/thread_safe_memoizer.h +++ b/Firestore/core/src/core/thread_safe_memoizer.h @@ -32,7 +32,13 @@ namespace core { template class ThreadSafeMemoizer { public: - ~ThreadSafeMemoizer(); + ~ThreadSafeMemoizer() { + // Call `std::call_once` in order to synchronize with the "active" + // invocation of `memoize()`. Without this synchronization, there is a data + // race between this destructor, which "reads" `filters_` to destroy it, and + // the write to `filters_` done by the "active" invocation of `memoize()`. + std::call_once(once_, [&]() {}); + }; /** * Memoize a value. @@ -47,7 +53,10 @@ class ThreadSafeMemoizer { * therefore, the "active" invocation's job to return the std::vector * to memoize. */ - const std::vector& memoize(std::function()>); + const std::vector& memoize(std::function()> func) { + std::call_once(once_, [&]() { filters_ = func(); }); + return filters_; + }; private: std::once_flag once_; From a7fa0e32a0efcda6fd4f36c76ece15633f73597b Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 6 Sep 2023 13:44:48 -0400 Subject: [PATCH 29/46] make memoized_normalized_order_bys_ use new class --- Firestore/core/src/core/query.cc | 18 +++++++------- Firestore/core/src/core/query.h | 13 ++++++++-- .../core/src/core/thread_safe_memoizer.h | 24 +++++++++---------- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index d6a48bb9c69..442a7357b97 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -90,19 +90,21 @@ absl::optional Query::FindOpInsideFilters( } const std::vector& Query::normalized_order_bys() const { - if (memoized_normalized_order_bys_.empty()) { + return memoized_normalized_order_bys_->memoize([&]() { + std::vector result; 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_normalized_order_bys_ = { + result = { OrderBy(FieldPath::KeyFieldPath(), Direction::Ascending), }; } else { - memoized_normalized_order_bys_ = { + result = { OrderBy(*inequality_field, Direction::Ascending), OrderBy(FieldPath::KeyFieldPath(), Direction::Ascending), }; @@ -114,8 +116,7 @@ const std::vector& Query::normalized_order_bys() const { first_order_by_field->CanonicalString(), inequality_field->CanonicalString()); - std::vector result = explicit_order_bys_; - + result = explicit_order_bys_; bool found_explicit_key_order = false; for (const OrderBy& order_by : explicit_order_bys_) { if (order_by.field().IsKeyFieldPath()) { @@ -132,11 +133,10 @@ const std::vector& Query::normalized_order_bys() const { : explicit_order_bys_.back().direction(); result.emplace_back(FieldPath::KeyFieldPath(), last_direction); } - - memoized_normalized_order_bys_ = std::move(result); } - } - return memoized_normalized_order_bys_; + + return result; + }); } const FieldPath* Query::FirstOrderByField() const { diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index d85d99a4824..9e44261a681 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -277,8 +277,17 @@ class Query { // sort at the end. std::vector explicit_order_bys_; - // The memoized list of sort orders. - mutable std::vector memoized_normalized_order_bys_; + /** + * The memoized list of sort orders. + * + * Use a `std::shared_ptr` rather than using + * `ThreadSafeMemoizer` directly so that this class is copyable + * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` + * member variable, which is not copyable). + */ + mutable std::shared_ptr> + memoized_normalized_order_bys_ = + std::make_shared>(); int32_t limit_ = Target::kNoLimit; LimitType limit_type_ = LimitType::None; diff --git a/Firestore/core/src/core/thread_safe_memoizer.h b/Firestore/core/src/core/thread_safe_memoizer.h index 949ea1c6dde..a73d4056983 100644 --- a/Firestore/core/src/core/thread_safe_memoizer.h +++ b/Firestore/core/src/core/thread_safe_memoizer.h @@ -14,11 +14,10 @@ * limitations under the License. */ -#ifndef THREAD_SAFE_MEMOIZER_H_ -#define THREAD_SAFE_MEMOIZER_H_ +#ifndef FIRESTORE_CORE_SRC_CORE_THREAD_SAFE_MEMOIZER_H_ +#define FIRESTORE_CORE_SRC_CORE_THREAD_SAFE_MEMOIZER_H_ -#include -#include +#include // NOLINT(build/c++11) #include namespace firebase { @@ -35,10 +34,11 @@ class ThreadSafeMemoizer { ~ThreadSafeMemoizer() { // Call `std::call_once` in order to synchronize with the "active" // invocation of `memoize()`. Without this synchronization, there is a data - // race between this destructor, which "reads" `filters_` to destroy it, and - // the write to `filters_` done by the "active" invocation of `memoize()`. + // race between this destructor, which "reads" `memoized_value_` to destroy + // it, and the write to `memoized_value_` done by the "active" invocation of + // `memoize()`. std::call_once(once_, [&]() {}); - }; + } /** * Memoize a value. @@ -54,17 +54,17 @@ class ThreadSafeMemoizer { * to memoize. */ const std::vector& memoize(std::function()> func) { - std::call_once(once_, [&]() { filters_ = func(); }); - return filters_; - }; + std::call_once(once_, [&]() { memoized_value_ = func(); }); + return memoized_value_; + } private: std::once_flag once_; - std::vector filters_; + std::vector memoized_value_; }; } // namespace core } // namespace firestore } // namespace firebase -#endif // THREAD_SAFE_MEMOIZER_H_ +#endif // FIRESTORE_CORE_SRC_CORE_THREAD_SAFE_MEMOIZER_H_ \ No newline at end of file From 51e08c6f925d5025a1bec5c3fabb96b6be030e34 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 6 Sep 2023 13:59:38 -0400 Subject: [PATCH 30/46] move to util folder --- Firestore/core/src/core/filter.cc | 5 +++-- Firestore/core/src/core/filter.h | 5 +++-- .../core/src/{core => util}/thread_safe_memoizer.h | 10 +++++----- 3 files changed, 11 insertions(+), 9 deletions(-) rename Firestore/core/src/{core => util}/thread_safe_memoizer.h (91%) diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index d51c90ab773..8ef426177c9 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -15,11 +15,12 @@ */ #include "Firestore/core/src/core/filter.h" -#include "Firestore/core/src/core/field_filter.h" -#include "Firestore/core/src/core/thread_safe_memoizer.h" #include +#include "Firestore/core/src/core/field_filter.h" +#include "Firestore/core/src/util/thread_safe_memoizer.h" + namespace firebase { namespace firestore { namespace core { diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index c1bdfc03764..d375115a507 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -23,12 +23,13 @@ #include #include -#include "Firestore/core/src/core/thread_safe_memoizer.h" #include "Firestore/core/src/model/model_fwd.h" +#include "Firestore/core/src/util/thread_safe_memoizer.h" + +using firebase::firestore::util::ThreadSafeMemoizer; namespace firebase { namespace firestore { - namespace core { class FieldFilter; diff --git a/Firestore/core/src/core/thread_safe_memoizer.h b/Firestore/core/src/util/thread_safe_memoizer.h similarity index 91% rename from Firestore/core/src/core/thread_safe_memoizer.h rename to Firestore/core/src/util/thread_safe_memoizer.h index a73d4056983..e84187c60aa 100644 --- a/Firestore/core/src/core/thread_safe_memoizer.h +++ b/Firestore/core/src/util/thread_safe_memoizer.h @@ -14,15 +14,15 @@ * limitations under the License. */ -#ifndef FIRESTORE_CORE_SRC_CORE_THREAD_SAFE_MEMOIZER_H_ -#define FIRESTORE_CORE_SRC_CORE_THREAD_SAFE_MEMOIZER_H_ +#ifndef FIRESTORE_CORE_SRC_UTIL_THREAD_SAFE_MEMOIZER_H_ +#define FIRESTORE_CORE_SRC_UTIL_THREAD_SAFE_MEMOIZER_H_ #include // NOLINT(build/c++11) #include namespace firebase { namespace firestore { -namespace core { +namespace util { /** * Stores a memoized value in a manner that is safe to be shared between @@ -63,8 +63,8 @@ class ThreadSafeMemoizer { std::vector memoized_value_; }; -} // namespace core +} // namespace util } // namespace firestore } // namespace firebase -#endif // FIRESTORE_CORE_SRC_CORE_THREAD_SAFE_MEMOIZER_H_ \ No newline at end of file +#endif // FIRESTORE_CORE_SRC_UTIL_THREAD_SAFE_MEMOIZER_H_ From f2fd8f4697ca6ef8d6122626837e22e20cf7c10b Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 6 Sep 2023 14:21:08 -0400 Subject: [PATCH 31/46] format --- Firestore/core/src/core/composite_filter.cc | 1 + Firestore/core/src/core/filter.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Firestore/core/src/core/composite_filter.cc b/Firestore/core/src/core/composite_filter.cc index b0472539d0f..5c917d4e2a1 100644 --- a/Firestore/core/src/core/composite_filter.cc +++ b/Firestore/core/src/core/composite_filter.cc @@ -163,6 +163,7 @@ const std::vector& CompositeFilter::Rep::GetFlattenedFilters() return flattened_filters; }); } + } // namespace core } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index d375115a507..b4b2c61cd07 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -19,7 +19,6 @@ #include #include -#include // NOLINT(build/c++11) #include #include @@ -126,6 +125,7 @@ class Filter { class Rep { public: Rep(); + virtual ~Rep() = default; virtual Type type() const { From 3ef704473a198327a1144945a849c330adf69b22 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 7 Sep 2023 10:50:45 -0400 Subject: [PATCH 32/46] change the ThreadSafeMemoizer param type --- Firestore/core/src/core/filter.cc | 2 +- Firestore/core/src/core/filter.h | 2 +- Firestore/core/src/core/query.h | 4 ++-- Firestore/core/src/util/thread_safe_memoizer.h | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index 8ef426177c9..a576b02c439 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -37,7 +37,7 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) { Filter::Rep::Rep() : memoized_flattened_filters_( - std::make_shared>()) { + std::make_shared>>()) { } } // namespace core diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index b4b2c61cd07..ec7927f167f 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -172,7 +172,7 @@ class Filter { * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` * member variable, which is not copyable). */ - mutable std::shared_ptr> + mutable std::shared_ptr>> memoized_flattened_filters_; }; diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index 9e44261a681..3997da19889 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -285,9 +285,9 @@ class Query { * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` * member variable, which is not copyable). */ - mutable std::shared_ptr> + mutable std::shared_ptr>> memoized_normalized_order_bys_ = - std::make_shared>(); + std::make_shared>>(); int32_t limit_ = Target::kNoLimit; LimitType limit_type_ = LimitType::None; diff --git a/Firestore/core/src/util/thread_safe_memoizer.h b/Firestore/core/src/util/thread_safe_memoizer.h index e84187c60aa..1603d4b6159 100644 --- a/Firestore/core/src/util/thread_safe_memoizer.h +++ b/Firestore/core/src/util/thread_safe_memoizer.h @@ -53,14 +53,14 @@ class ThreadSafeMemoizer { * therefore, the "active" invocation's job to return the std::vector * to memoize. */ - const std::vector& memoize(std::function()> func) { + const T& memoize(std::function func) { std::call_once(once_, [&]() { memoized_value_ = func(); }); return memoized_value_; } private: std::once_flag once_; - std::vector memoized_value_; + T memoized_value_; }; } // namespace util From f58c8cf844c3d4a67e959f91ac4f8921e25e2ab4 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 7 Sep 2023 13:53:42 -0400 Subject: [PATCH 33/46] update test comment --- Firestore/Example/Tests/Integration/API/FIRQueryTests.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 801835a0fa4..4925146589d 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1653,7 +1653,7 @@ - (void)testMultipleInequalityFromCacheAndFromServer { [self checkOnlineAndOfflineQuery:[collRef queryWhereFilter:filter] matchesResult:@[ @"doc2" ]]; // explicit AND: a < 3 && b not-in [2, 3] - // Implicitly ordered by: a asc, __name__ asc + // Implicitly ordered by: a asc, b asc, __name__ asc filter = [FIRFilter andFilterWithFilters:@[ [FIRFilter filterWhereField:@"a" isLessThan:@3], [FIRFilter filterWhereField:@"b" notIn:@[ @2, @3 ]] From 2fe530a8ee4f66e170624fe6711ff708b95d9f0f Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 7 Sep 2023 15:19:56 -0400 Subject: [PATCH 34/46] update memoized_target and memoized_aggregate_target --- Firestore/core/src/core/query.cc | 14 ++++---------- Firestore/core/src/core/query.h | 21 ++++++++++++++++----- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index 442a7357b97..c65d4edf348 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -329,19 +329,13 @@ std::string Query::ToString() const { } const Target& Query::ToTarget() const& { - if (memoized_target == nullptr) { - memoized_target = ToTarget(normalized_order_bys()); - } - - return *memoized_target; + return memoized_target_->memoize( + [&]() { return *ToTarget(normalized_order_bys()); }); } const Target& Query::ToAggregateTarget() const& { - if (memoized_aggregate_target == nullptr) { - memoized_aggregate_target = ToTarget(explicit_order_bys_); - } - - return *memoized_aggregate_target; + return memoized_aggregate_target_->memoize( + [&]() { return *ToTarget(explicit_order_bys_); }); } const std::shared_ptr Query::ToTarget( diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index 3997da19889..7e71ba2b9ea 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -295,16 +295,27 @@ class Query { absl::optional start_at_; absl::optional end_at_; + const std::shared_ptr ToTarget( + const std::vector& order_bys) const&; + + /** + * Use a `std::shared_ptr` rather than using + * `ThreadSafeMemoizer` directly so that this class is copyable + * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` + * member variable, which is not copyable). + */ + // The corresponding Target of this Query instance. - mutable std::shared_ptr memoized_target; + mutable std::shared_ptr> memoized_target_ = + std::make_shared>(); // 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 memoized_aggregate_target; - - const std::shared_ptr ToTarget( - const std::vector& order_bys) const&; + // mutable std::shared_ptr memoized_aggregate_target; + mutable std::shared_ptr> + memoized_aggregate_target_ = + std::make_shared>(); }; bool operator==(const Query& lhs, const Query& rhs); From 5c614fc037ee9fadfecedd38fd1f4d5c95c23827 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 7 Sep 2023 15:30:03 -0400 Subject: [PATCH 35/46] format --- Firestore/core/src/core/query.h | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index 7e71ba2b9ea..bb4014a440c 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -277,18 +277,6 @@ class Query { // sort at the end. std::vector explicit_order_bys_; - /** - * The memoized list of sort orders. - * - * Use a `std::shared_ptr` rather than using - * `ThreadSafeMemoizer` directly so that this class is copyable - * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` - * member variable, which is not copyable). - */ - mutable std::shared_ptr>> - memoized_normalized_order_bys_ = - std::make_shared>>(); - int32_t limit_ = Target::kNoLimit; LimitType limit_type_ = LimitType::None; @@ -299,12 +287,17 @@ class Query { const std::vector& order_bys) const&; /** - * Use a `std::shared_ptr` rather than using - * `ThreadSafeMemoizer` directly so that this class is copyable + * For properties below, use a `std::shared_ptr` rather + * than using `ThreadSafeMemoizer` directly so that this class is copyable * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` * member variable, which is not copyable). */ + // The memoized list of sort orders. + mutable std::shared_ptr>> + memoized_normalized_order_bys_ = + std::make_shared>>(); + // The corresponding Target of this Query instance. mutable std::shared_ptr> memoized_target_ = std::make_shared>(); @@ -312,7 +305,7 @@ class Query { // 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 memoized_aggregate_target; + // mutable std::shared_ptr memoized_aggregate_target_; mutable std::shared_ptr> memoized_aggregate_target_ = std::make_shared>(); From 365550811d7a7d2c79c8c43deb22bc23cf46ac6e Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 7 Sep 2023 16:40:16 -0400 Subject: [PATCH 36/46] resolve comments --- Firestore/core/src/core/query.cc | 11 +++++------ Firestore/core/src/core/query.h | 4 +--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index c65d4edf348..c984f456708 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -330,16 +330,15 @@ std::string Query::ToString() const { const Target& Query::ToTarget() const& { return memoized_target_->memoize( - [&]() { return *ToTarget(normalized_order_bys()); }); + [&]() { return ToTarget(normalized_order_bys()); }); } const Target& Query::ToAggregateTarget() const& { return memoized_aggregate_target_->memoize( - [&]() { return *ToTarget(explicit_order_bys_); }); + [&]() { return ToTarget(explicit_order_bys_); }); } -const std::shared_ptr Query::ToTarget( - const std::vector& order_bys) const& { +Target Query::ToTarget(const std::vector& order_bys) const { if (limit_type_ == LimitType::Last) { // Flip the orderBy directions since we want the last results std::vector new_order_bys; @@ -362,11 +361,11 @@ const std::shared_ptr Query::ToTarget( Target target(path(), collection_group(), filters(), new_order_bys, limit_, new_start_at, new_end_at); - return std::make_shared(std::move(target)); + return target; } else { Target target(path(), collection_group(), filters(), order_bys, limit_, start_at(), end_at()); - return std::make_shared(std::move(target)); + return target; } } diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index bb4014a440c..8315437d79a 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -283,8 +283,7 @@ class Query { absl::optional start_at_; absl::optional end_at_; - const std::shared_ptr ToTarget( - const std::vector& order_bys) const&; + Target ToTarget(const std::vector& order_bys) const; /** * For properties below, use a `std::shared_ptr` rather @@ -305,7 +304,6 @@ class Query { // 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 memoized_aggregate_target_; mutable std::shared_ptr> memoized_aggregate_target_ = std::make_shared>(); From 8048d9a8a7c3a965564a13424924485b0a70b29d Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 11 Sep 2023 14:49:38 -0400 Subject: [PATCH 37/46] resolve comments --- Firestore/core/src/core/query.cc | 6 +- Firestore/core/src/core/query.h | 22 ++++--- .../core/src/util/thread_safe_memoizer.h | 12 ++++ .../test/unit/util/thread_safe_memoizer.cc | 58 +++++++++++++++++++ 4 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 Firestore/core/test/unit/util/thread_safe_memoizer.cc diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index c984f456708..eb089ff2359 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -359,13 +359,11 @@ Target Query::ToTarget(const std::vector& order_bys) const { start_at_->position(), start_at_->inclusive())} : absl::nullopt; - Target target(path(), collection_group(), filters(), new_order_bys, limit_, + return Target(path(), collection_group(), filters(), new_order_bys, limit_, new_start_at, new_end_at); - return target; } else { - Target target(path(), collection_group(), filters(), order_bys, limit_, + return Target(path(), collection_group(), filters(), order_bys, limit_, start_at(), end_at()); - return target; } } diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index 8315437d79a..13e27eb8238 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -285,28 +285,26 @@ class Query { Target ToTarget(const std::vector& order_bys) const; - /** - * For properties below, use a `std::shared_ptr` rather - * than using `ThreadSafeMemoizer` directly so that this class is copyable - * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` - * member variable, which is not copyable). - */ + // For properties below, use a `std::shared_ptr` rather + // than using `ThreadSafeMemoizer` directly so that this class is copyable + // (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` + // member variable, which is not copyable). // The memoized list of sort orders. mutable std::shared_ptr>> - memoized_normalized_order_bys_ = - std::make_shared>>(); + memoized_normalized_order_bys_{ + std::make_shared>>()}; // The corresponding Target of this Query instance. - mutable std::shared_ptr> memoized_target_ = - std::make_shared>(); + mutable std::shared_ptr> memoized_target_{ + std::make_shared>()}; // 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> - memoized_aggregate_target_ = - std::make_shared>(); + memoized_aggregate_target_{ + std::make_shared>()}; }; bool operator==(const Query& lhs, const Query& rhs); diff --git a/Firestore/core/src/util/thread_safe_memoizer.h b/Firestore/core/src/util/thread_safe_memoizer.h index 1603d4b6159..ba06312136c 100644 --- a/Firestore/core/src/util/thread_safe_memoizer.h +++ b/Firestore/core/src/util/thread_safe_memoizer.h @@ -17,6 +17,7 @@ #ifndef FIRESTORE_CORE_SRC_UTIL_THREAD_SAFE_MEMOIZER_H_ #define FIRESTORE_CORE_SRC_UTIL_THREAD_SAFE_MEMOIZER_H_ +#include #include // NOLINT(build/c++11) #include @@ -27,10 +28,14 @@ namespace util { /** * Stores a memoized value in a manner that is safe to be shared between * multiple threads. + * + * TODO(b/299933587) Make `ThreadSafeMemoizer` copyable and moveable. */ template class ThreadSafeMemoizer { public: + ThreadSafeMemoizer() = default; + ~ThreadSafeMemoizer() { // Call `std::call_once` in order to synchronize with the "active" // invocation of `memoize()`. Without this synchronization, there is a data @@ -40,6 +45,13 @@ class ThreadSafeMemoizer { std::call_once(once_, [&]() {}); } + // This class cannot be copied or moved, because it has `std::once_flag` + // member. + ThreadSafeMemoizer(const ThreadSafeMemoizer&) = delete; + ThreadSafeMemoizer(ThreadSafeMemoizer&&) = delete; + ThreadSafeMemoizer& operator=(const ThreadSafeMemoizer&) = delete; + ThreadSafeMemoizer& operator=(ThreadSafeMemoizer&&) = delete; + /** * Memoize a value. * diff --git a/Firestore/core/test/unit/util/thread_safe_memoizer.cc b/Firestore/core/test/unit/util/thread_safe_memoizer.cc new file mode 100644 index 00000000000..2abed8355be --- /dev/null +++ b/Firestore/core/test/unit/util/thread_safe_memoizer.cc @@ -0,0 +1,58 @@ +/* + * Copyright 2023 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/src/util/thread_safe_memoizer.h" + +#include +#include "gtest/gtest.h" + +namespace firebase { +namespace firestore { +namespace util { + +// Define a simple expensive function for testing. +int ExpensiveFunction() { + // Simulate an expensive operation + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + return 42; +} + +TEST(ThreadSafeMemoizerTest, MultiThreadedMemoization) { + const int num_threads = 5; + const int expected_result = 42; + + // Create a thread safe memoizer and multiple threads. + firebase::firestore::util::ThreadSafeMemoizer memoized_result; + std::vector threads; + + for (int i = 0; i < num_threads; ++i) { + threads.emplace_back([&memoized_result, expected_result]() { + const int& actual_result = memoized_result.memoize(ExpensiveFunction); + + // Verify that all threads get the same memoized result. + EXPECT_EQ(actual_result, expected_result); + }); + } + + // Start all threads + for (auto& thread : threads) { + thread.join(); + } +} + +} // namespace util +} // namespace firestore +} // namespace firebase From 2709afd65a98ec985fc89109903c2218639db13c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 11 Sep 2023 14:52:01 -0400 Subject: [PATCH 38/46] Update thread_safe_memoizer.cc --- Firestore/core/test/unit/util/thread_safe_memoizer.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/Firestore/core/test/unit/util/thread_safe_memoizer.cc b/Firestore/core/test/unit/util/thread_safe_memoizer.cc index 2abed8355be..561269c35f0 100644 --- a/Firestore/core/test/unit/util/thread_safe_memoizer.cc +++ b/Firestore/core/test/unit/util/thread_safe_memoizer.cc @@ -23,7 +23,6 @@ namespace firebase { namespace firestore { namespace util { -// Define a simple expensive function for testing. int ExpensiveFunction() { // Simulate an expensive operation std::this_thread::sleep_for(std::chrono::milliseconds(100)); @@ -47,7 +46,6 @@ TEST(ThreadSafeMemoizerTest, MultiThreadedMemoization) { }); } - // Start all threads for (auto& thread : threads) { thread.join(); } From 7d5d11bfc053b6fe25803e670bc54c2e9c4c6c90 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 11 Sep 2023 14:52:53 -0400 Subject: [PATCH 39/46] Update thread_safe_memoizer_test.cc --- .../{thread_safe_memoizer.cc => thread_safe_memoizer_test.cc} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Firestore/core/test/unit/util/{thread_safe_memoizer.cc => thread_safe_memoizer_test.cc} (100%) diff --git a/Firestore/core/test/unit/util/thread_safe_memoizer.cc b/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc similarity index 100% rename from Firestore/core/test/unit/util/thread_safe_memoizer.cc rename to Firestore/core/test/unit/util/thread_safe_memoizer_test.cc From 101b7eea02a611f36904cd23eea399e06b32dcec Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 11 Sep 2023 15:25:02 -0400 Subject: [PATCH 40/46] Update project.pbxproj --- .../Example/Firestore.xcodeproj/project.pbxproj | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index ac9c117034c..cea40812f94 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -73,6 +73,7 @@ 095A878BB33211AB52BFAD9F /* leveldb_document_overlay_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AE89CFF09C6804573841397F /* leveldb_document_overlay_cache_test.cc */; }; 0963F6D7B0F9AE1E24B82866 /* path_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 403DBF6EFB541DFD01582AA3 /* path_test.cc */; }; 096BA3A3703AC1491F281618 /* index.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 395E8B07639E69290A929695 /* index.pb.cc */; }; + 09B83B26E47B6F6668DF54B8 /* thread_safe_memoizer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 1A8141230C7E3986EACEF0B6 /* thread_safe_memoizer_test.cc */; }; 09BE8C01EC33D1FD82262D5D /* aggregate_query_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AF924C79F49F793992A84879 /* aggregate_query_test.cc */; }; 0A4E1B5E3E853763AE6ED7AE /* grpc_stream_tester.cc in Sources */ = {isa = PBXBuildFile; fileRef = 87553338E42B8ECA05BA987E /* grpc_stream_tester.cc */; }; 0A52B47C43B7602EE64F53A7 /* cc_compilation_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 1B342370EAE3AA02393E33EB /* cc_compilation_test.cc */; }; @@ -229,6 +230,7 @@ 205601D1C6A40A4DD3BBAA04 /* target_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 526D755F65AC676234F57125 /* target_test.cc */; }; 20814A477D00EA11D0E76631 /* FIRDocumentSnapshotTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04B202154AA00B64F25 /* FIRDocumentSnapshotTests.mm */; }; 20A26E9D0336F7F32A098D05 /* Pods_Firestore_IntegrationTests_tvOS.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 2220F583583EFC28DE792ABE /* Pods_Firestore_IntegrationTests_tvOS.framework */; }; + 20A93AC59CD5A7AC41F10412 /* thread_safe_memoizer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 1A8141230C7E3986EACEF0B6 /* thread_safe_memoizer_test.cc */; }; 211A60ECA3976D27C0BF59BB /* md5_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 3D050936A2D52257FD17FB6E /* md5_test.cc */; }; 21836C4D9D48F962E7A3A244 /* ordered_code_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB380D03201BC6E400D97691 /* ordered_code_test.cc */; }; 21A2A881F71CB825299DF06E /* hard_assert_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 444B7AB3F5A2929070CB1363 /* hard_assert_test.cc */; }; @@ -500,6 +502,7 @@ 50454F81EC4584D4EB5F5ED5 /* serializer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 61F72C5520BC48FD001A68CB /* serializer_test.cc */; }; 50B749CA98365368AE34B71C /* filter_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = F02F734F272C3C70D1307076 /* filter_test.cc */; }; 50C852E08626CFA7DC889EEA /* field_index_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = BF76A8DA34B5B67B4DD74666 /* field_index_test.cc */; }; + 51018EA27CF914DD1CC79CB3 /* thread_safe_memoizer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 1A8141230C7E3986EACEF0B6 /* thread_safe_memoizer_test.cc */; }; 513D34C9964E8C60C5C2EE1C /* leveldb_bundle_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 8E9CD82E60893DDD7757B798 /* leveldb_bundle_cache_test.cc */; }; 5150E9F256E6E82D6F3CB3F1 /* bundle_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = F7FC06E0A47D393DE1759AE1 /* bundle_cache_test.cc */; }; 518BF03D57FBAD7C632D18F8 /* FIRQueryUnitTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = FF73B39D04D1760190E6B84A /* FIRQueryUnitTests.mm */; }; @@ -662,6 +665,7 @@ 5B4391097A6DF86EC3801DEE /* string_win_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 79507DF8378D3C42F5B36268 /* string_win_test.cc */; }; 5B62003FEA9A3818FDF4E2DD /* document_key_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6152AD5202A5385000E5744 /* document_key_test.cc */; }; 5B89B1BA0AD400D9BF581420 /* listen_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 54DA12A01F315EE100DD57A1 /* listen_spec_test.json */; }; + 5BB33F0BC7960D26062B07D3 /* thread_safe_memoizer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 1A8141230C7E3986EACEF0B6 /* thread_safe_memoizer_test.cc */; }; 5BC8406FD842B2FC2C200B2F /* stream_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 5B5414D28802BC76FDADABD6 /* stream_test.cc */; }; 5BE49546D57C43DDFCDB6FBD /* to_string_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = B68B1E002213A764008977EF /* to_string_apple_test.mm */; }; 5C9B5696644675636A052018 /* token_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A082AFDD981B07B5AD78FDE8 /* token_test.cc */; }; @@ -789,6 +793,7 @@ 6D7F70938662E8CA334F11C2 /* target_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B5C37696557C81A6C2B7271A /* target_cache_test.cc */; }; 6DBB3DB3FD6B4981B7F26A55 /* FIRQuerySnapshotTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04F202154AA00B64F25 /* FIRQuerySnapshotTests.mm */; }; 6DCA8E54E652B78EFF3EEDAC /* XCTestCase+Await.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E0372021401E00B64F25 /* XCTestCase+Await.mm */; }; + 6DFD49CCE2281CE243FEBB63 /* thread_safe_memoizer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 1A8141230C7E3986EACEF0B6 /* thread_safe_memoizer_test.cc */; }; 6E10507432E1D7AE658D16BD /* FSTSpecTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E03020213FFC00B64F25 /* FSTSpecTests.mm */; }; 6E4854B19B120C6F0F8192CC /* FSTAPIHelpers.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04E202154AA00B64F25 /* FSTAPIHelpers.mm */; }; 6E59498D20F55BA800ECD9A5 /* FuzzingResources in Resources */ = {isa = PBXBuildFile; fileRef = 6ED6DEA120F5502700FC6076 /* FuzzingResources */; }; @@ -1162,6 +1167,7 @@ B2554A2BA211D10823646DBE /* Validation_BloomFilterTest_MD5_500_01_bloom_filter_proto.json in Resources */ = {isa = PBXBuildFile; fileRef = 4BD051DBE754950FEAC7A446 /* Validation_BloomFilterTest_MD5_500_01_bloom_filter_proto.json */; }; B28ACC69EB1F232AE612E77B /* async_testing.cc in Sources */ = {isa = PBXBuildFile; fileRef = 872C92ABD71B12784A1C5520 /* async_testing.cc */; }; B2A9965ED0114E39A911FD09 /* Validation_BloomFilterTest_MD5_5000_1_bloom_filter_proto.json in Resources */ = {isa = PBXBuildFile; fileRef = 4375BDCDBCA9938C7F086730 /* Validation_BloomFilterTest_MD5_5000_1_bloom_filter_proto.json */; }; + B31B5E0D4EA72C5916CC71F5 /* thread_safe_memoizer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 1A8141230C7E3986EACEF0B6 /* thread_safe_memoizer_test.cc */; }; B371628DA91E80B64AE53085 /* FIRFieldPathTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04C202154AA00B64F25 /* FIRFieldPathTests.mm */; }; B384E0F90D4CCC15C88CAF30 /* target_index_matcher_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 63136A2371C0C013EC7A540C /* target_index_matcher_test.cc */; }; B3A309CCF5D75A555C7196E1 /* path_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 403DBF6EFB541DFD01582AA3 /* path_test.cc */; }; @@ -1675,6 +1681,7 @@ 132E32997D781B896672D30A /* reference_set_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = reference_set_test.cc; sourceTree = ""; }; 166CE73C03AB4366AAC5201C /* leveldb_index_manager_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = leveldb_index_manager_test.cc; sourceTree = ""; }; 1A7D48A017ECB54FD381D126 /* Validation_BloomFilterTest_MD5_5000_1_membership_test_result.json */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.json; name = Validation_BloomFilterTest_MD5_5000_1_membership_test_result.json; path = bloom_filter_golden_test_data/Validation_BloomFilterTest_MD5_5000_1_membership_test_result.json; sourceTree = ""; }; + 1A8141230C7E3986EACEF0B6 /* thread_safe_memoizer_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; path = thread_safe_memoizer_test.cc; sourceTree = ""; }; 1B342370EAE3AA02393E33EB /* cc_compilation_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; name = cc_compilation_test.cc; path = api/cc_compilation_test.cc; sourceTree = ""; }; 1B9F95EC29FAD3F100EEC075 /* FIRAggregateQueryUnitTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FIRAggregateQueryUnitTests.mm; sourceTree = ""; }; 1C01D8CE367C56BB2624E299 /* index.pb.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; name = index.pb.h; path = admin/index.pb.h; sourceTree = ""; }; @@ -2361,6 +2368,7 @@ 79507DF8378D3C42F5B36268 /* string_win_test.cc */, 899FC22684B0F7BEEAE13527 /* task_test.cc */, A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */, + 1A8141230C7E3986EACEF0B6 /* thread_safe_memoizer_test.cc */, B68B1E002213A764008977EF /* to_string_apple_test.mm */, B696858D2214B53900271095 /* to_string_test.cc */, ); @@ -4260,6 +4268,7 @@ 88929ED628DA8DD9592974ED /* task_test.cc in Sources */, 9B2C6A48A4DBD36080932B4E /* testing_hooks_test.cc in Sources */, 32A95242C56A1A230231DB6A /* testutil.cc in Sources */, + 51018EA27CF914DD1CC79CB3 /* thread_safe_memoizer_test.cc in Sources */, 5497CB78229DECDE000FB92F /* time_testing.cc in Sources */, ACC9369843F5ED3BD2284078 /* timestamp_test.cc in Sources */, 2AAEABFD550255271E3BAC91 /* to_string_apple_test.mm in Sources */, @@ -4477,6 +4486,7 @@ 67CF9FAA890307780731E1DA /* task_test.cc in Sources */, 24B75C63BDCD5551B2F69901 /* testing_hooks_test.cc in Sources */, 8388418F43042605FB9BFB92 /* testutil.cc in Sources */, + 5BB33F0BC7960D26062B07D3 /* thread_safe_memoizer_test.cc in Sources */, 5497CB79229DECDE000FB92F /* time_testing.cc in Sources */, 26CB3D7C871BC56456C6021E /* timestamp_test.cc in Sources */, 5BE49546D57C43DDFCDB6FBD /* to_string_apple_test.mm in Sources */, @@ -4715,6 +4725,7 @@ 76A5447D76F060E996555109 /* task_test.cc in Sources */, D0DA42DC66C4FE508A63B269 /* testing_hooks_test.cc in Sources */, 409C0F2BFC2E1BECFFAC4D32 /* testutil.cc in Sources */, + B31B5E0D4EA72C5916CC71F5 /* thread_safe_memoizer_test.cc in Sources */, 6300709ECDE8E0B5A8645F8D /* time_testing.cc in Sources */, 0CEE93636BA4852D3C5EC428 /* timestamp_test.cc in Sources */, 95DCD082374F871A86EF905F /* to_string_apple_test.mm in Sources */, @@ -4953,6 +4964,7 @@ 93C8F772F4DC5A985FA3D815 /* task_test.cc in Sources */, F6738D3B72352BBEFB87172C /* testing_hooks_test.cc in Sources */, A17DBC8F24127DA8A381F865 /* testutil.cc in Sources */, + 09B83B26E47B6F6668DF54B8 /* thread_safe_memoizer_test.cc in Sources */, A25FF76DEF542E01A2DF3B0E /* time_testing.cc in Sources */, 1E42CD0F60EB22A5D0C86D1F /* timestamp_test.cc in Sources */, F9705E595FC3818F13F6375A /* to_string_apple_test.mm in Sources */, @@ -5180,6 +5192,7 @@ 662793139A36E5CFC935B949 /* task_test.cc in Sources */, F184E5367DF3CA158EDE8532 /* testing_hooks_test.cc in Sources */, 54A0352A20A3B3BD003E0143 /* testutil.cc in Sources */, + 20A93AC59CD5A7AC41F10412 /* thread_safe_memoizer_test.cc in Sources */, 5497CB77229DECDE000FB92F /* time_testing.cc in Sources */, ABF6506C201131F8005F2C74 /* timestamp_test.cc in Sources */, B68B1E012213A765008977EF /* to_string_apple_test.mm in Sources */, @@ -5437,6 +5450,7 @@ C57B15CADD8C3E806B154C19 /* task_test.cc in Sources */, 5360D52DCAD1069B1E4B0B9D /* testing_hooks_test.cc in Sources */, CA989C0E6020C372A62B7062 /* testutil.cc in Sources */, + 6DFD49CCE2281CE243FEBB63 /* thread_safe_memoizer_test.cc in Sources */, 2D220B9ABFA36CD7AC43D0A7 /* time_testing.cc in Sources */, D91D86B29B86A60C05879A48 /* timestamp_test.cc in Sources */, 60260A06871DCB1A5F3448D3 /* to_string_apple_test.mm in Sources */, From 6bc389fedb8f66995bba972083848a0273b56043 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 11 Sep 2023 15:34:13 -0400 Subject: [PATCH 41/46] resolve comments --- Firestore/core/src/core/filter.h | 4 +--- Firestore/core/src/core/query.h | 1 + Firestore/core/test/unit/util/thread_safe_memoizer_test.cc | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index ec7927f167f..1da4888f826 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -25,8 +25,6 @@ #include "Firestore/core/src/model/model_fwd.h" #include "Firestore/core/src/util/thread_safe_memoizer.h" -using firebase::firestore::util::ThreadSafeMemoizer; - namespace firebase { namespace firestore { namespace core { @@ -172,7 +170,7 @@ class Filter { * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag` * member variable, which is not copyable). */ - mutable std::shared_ptr>> + mutable std::shared_ptr>> memoized_flattened_filters_; }; diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index 13e27eb8238..cbb780b3403 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -30,6 +30,7 @@ #include "Firestore/core/src/core/target.h" #include "Firestore/core/src/model/model_fwd.h" #include "Firestore/core/src/model/resource_path.h" +#include "Firestore/core/src/util/thread_safe_memoizer.h" namespace firebase { namespace firestore { diff --git a/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc b/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc index 561269c35f0..2ededc6ee4c 100644 --- a/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc +++ b/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc @@ -16,7 +16,7 @@ #include "Firestore/core/src/util/thread_safe_memoizer.h" -#include +#include // NOLINT(build/c++11) #include "gtest/gtest.h" namespace firebase { From 9db37074cb17d544e020de8ff5792d294b636e91 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 11 Sep 2023 15:56:15 -0400 Subject: [PATCH 42/46] add "util::" --- Firestore/core/src/core/filter.cc | 3 ++- Firestore/core/src/core/query.h | 12 ++++++------ .../core/test/unit/util/thread_safe_memoizer_test.cc | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/Firestore/core/src/core/filter.cc b/Firestore/core/src/core/filter.cc index a576b02c439..a77ccc55e34 100644 --- a/Firestore/core/src/core/filter.cc +++ b/Firestore/core/src/core/filter.cc @@ -37,7 +37,8 @@ std::ostream& operator<<(std::ostream& os, const Filter& filter) { Filter::Rep::Rep() : memoized_flattened_filters_( - std::make_shared>>()) { + std::make_shared< + util::ThreadSafeMemoizer>>()) { } } // namespace core diff --git a/Firestore/core/src/core/query.h b/Firestore/core/src/core/query.h index cbb780b3403..047f2e24bbb 100644 --- a/Firestore/core/src/core/query.h +++ b/Firestore/core/src/core/query.h @@ -292,20 +292,20 @@ class Query { // member variable, which is not copyable). // The memoized list of sort orders. - mutable std::shared_ptr>> + mutable std::shared_ptr>> memoized_normalized_order_bys_{ - std::make_shared>>()}; + std::make_shared>>()}; // The corresponding Target of this Query instance. - mutable std::shared_ptr> memoized_target_{ - std::make_shared>()}; + mutable std::shared_ptr> memoized_target_{ + std::make_shared>()}; // 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> + mutable std::shared_ptr> memoized_aggregate_target_{ - std::make_shared>()}; + std::make_shared>()}; }; bool operator==(const Query& lhs, const Query& rhs); diff --git a/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc b/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc index 2ededc6ee4c..12fab227a57 100644 --- a/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc +++ b/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc @@ -34,7 +34,7 @@ TEST(ThreadSafeMemoizerTest, MultiThreadedMemoization) { const int expected_result = 42; // Create a thread safe memoizer and multiple threads. - firebase::firestore::util::ThreadSafeMemoizer memoized_result; + util::ThreadSafeMemoizer memoized_result; std::vector threads; for (int i = 0; i < num_threads; ++i) { From 1b73d330d6de33848ab9112ed9b32b1356a83e37 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 13 Sep 2023 10:41:21 -0400 Subject: [PATCH 43/46] Update thread_safe_memoizer_test.cc --- .../unit/util/thread_safe_memoizer_test.cc | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc b/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc index 12fab227a57..89a1d94dc25 100644 --- a/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc +++ b/Firestore/core/test/unit/util/thread_safe_memoizer_test.cc @@ -23,27 +23,33 @@ namespace firebase { namespace firestore { namespace util { -int ExpensiveFunction() { - // Simulate an expensive operation - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - return 42; -} - TEST(ThreadSafeMemoizerTest, MultiThreadedMemoization) { + std::atomic global_int{77}; + + auto expensive_lambda = [&]() { + // Simulate an expensive operation + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + // If the lambda gets executed multiple times, threads will see incremented + // `global_int`. + global_int++; + return global_int.load(); + }; + const int num_threads = 5; - const int expected_result = 42; + const int expected_result = 78; // Create a thread safe memoizer and multiple threads. util::ThreadSafeMemoizer memoized_result; std::vector threads; for (int i = 0; i < num_threads; ++i) { - threads.emplace_back([&memoized_result, expected_result]() { - const int& actual_result = memoized_result.memoize(ExpensiveFunction); + threads.emplace_back( + [&memoized_result, expected_result, &expensive_lambda]() { + const int& actual_result = memoized_result.memoize(expensive_lambda); - // Verify that all threads get the same memoized result. - EXPECT_EQ(actual_result, expected_result); - }); + // Verify that all threads get the same memoized result. + EXPECT_EQ(actual_result, expected_result); + }); } for (auto& thread : threads) { From f68ac37accfcb20478d27d93dba25ffba57d5497 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 13 Sep 2023 11:10:47 -0400 Subject: [PATCH 44/46] format --- Firestore/core/src/core/query.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/core/src/core/query.cc b/Firestore/core/src/core/query.cc index b25a23f44fe..5b70ebc322f 100644 --- a/Firestore/core/src/core/query.cc +++ b/Firestore/core/src/core/query.cc @@ -93,7 +93,7 @@ absl::optional Query::FindOpInsideFilters( const std::vector& Query::normalized_order_bys() const { return memoized_normalized_order_bys_->memoize([&]() { -// Any explicit order by fields should be added as is. + // Any explicit order by fields should be added as is. std::vector result = explicit_order_bys_; std::set fieldsNormalized; for (const OrderBy& order_by : explicit_order_bys_) { From 6ba8402b6ab5082a8024e2eeea637b58454b77a3 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 13 Sep 2023 12:35:30 -0400 Subject: [PATCH 45/46] remove duplicated class ThreadSafeMemoizer --- Firestore/core/src/core/filter.h | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 975ae48ecd5..7082bc9771e 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -153,35 +153,6 @@ class Filter { virtual std::vector GetFilters() const = 0; - /** - * Stores a memoized value in a manner that is safe to be shared between - * multiple threads. - */ - class ThreadSafeMemoizer { - public: - ~ThreadSafeMemoizer(); - - /** - * Memoize a value. - * - * The std::function object specified by the first invocation of this - * function (the "active" invocation) will be invoked synchronously. - * None of the std::function objects specified by the subsequent - * invocations of this function (the "passive" invocations) will be - * invoked. All invocations, both "active" and "passive", will return a - * reference to the std::vector created by copying the return value from - * the std::function specified by the "active" invocation. It is, - * therefore, the "active" invocation's job to return the std::vector - * to memoize. - */ - const std::vector& memoize( - std::function()>); - - private: - std::once_flag once_; - std::vector filters_; - }; - /** * Memoized list of all field filters that can be found by * traversing the tree of filters contained in this composite filter. From 9422d07c03f51585e0e640f293e574f899b924b8 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 13 Sep 2023 15:24:00 -0400 Subject: [PATCH 46/46] remove unused imports --- Firestore/core/src/core/filter.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/Firestore/core/src/core/filter.h b/Firestore/core/src/core/filter.h index 7082bc9771e..ec20120daf6 100644 --- a/Firestore/core/src/core/filter.h +++ b/Firestore/core/src/core/filter.h @@ -17,10 +17,8 @@ #ifndef FIRESTORE_CORE_SRC_CORE_FILTER_H_ #define FIRESTORE_CORE_SRC_CORE_FILTER_H_ -#include #include #include -#include // NOLINT(build/c++11) #include #include