From d79801682b3f93d2f2aeb5c1349ac70b75ab0e62 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 12 Dec 2024 16:00:50 -0500 Subject: [PATCH 1/3] sort document keys by numeric ids --- .../Tests/Integration/API/FIRQueryTests.mm | 89 +++++++++++++++++++ Firestore/core/src/model/base_path.h | 37 +++++++- 2 files changed, 125 insertions(+), 1 deletion(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 9afd1b469f1..2e18fed01ec 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -804,6 +804,95 @@ - (void)testCollectionGroupQueriesWithStartAtEndAtWithArbitraryDocumentIDs { XCTAssertEqualObjects(ids, (@[ @"cg-doc2" ])); } +- (void)testSnapshotListenerSortsQueryByDocumentIdInTheSameOrderAsServer { + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"A" : @{@"a" : @1}, + @"a" : @{@"a" : @1}, + @"Aa" : @{@"a" : @1}, + @"7" : @{@"a" : @1}, + @"12" : @{@"a" : @1}, + @"__id7__" : @{@"a" : @1}, + @"__id12__" : @{@"a" : @1}, + @"__id-2__" : @{@"a" : @1}, + @"__id1_" : @{@"a" : @1}, + @"_id1__" : @{@"a" : @1}, + @"__id9223372036854775807__" : @{@"a" : @1}, + @"__id-9223372036854775808__" : @{@"a" : @1}, + }]; + + FIRQuery *query = [collRef queryOrderedByFieldPath:[FIRFieldPath documentID]]; + NSArray *expectedDocs = @[ + @"__id-9223372036854775808__", @"__id-2__", @"__id7__", @"__id12__", + @"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id1_", @"_id1__", @"a" + ]; + FIRQuerySnapshot *getSnapshot = [self readDocumentSetForRef:query]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(getSnapshot), expectedDocs); + + id registration = + [query addSnapshotListener:self.eventAccumulator.valueEventHandler]; + FIRQuerySnapshot *watchSnapshot = [self.eventAccumulator awaitEventWithName:@"Snapshot"]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(watchSnapshot), expectedDocs); + + [registration remove]; +} + +- (void)testSnapshotListenerSortsFilteredQueryByDocumentIdInTheSameOrderAsServer { + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"A" : @{@"a" : @1}, + @"a" : @{@"a" : @1}, + @"Aa" : @{@"a" : @1}, + @"7" : @{@"a" : @1}, + @"12" : @{@"a" : @1}, + @"__id7__" : @{@"a" : @1}, + @"__id12__" : @{@"a" : @1}, + @"__id-2__" : @{@"a" : @1}, + @"__id1_" : @{@"a" : @1}, + @"_id1__" : @{@"a" : @1}, + @"__id9223372036854775807__" : @{@"a" : @1}, + @"__id-9223372036854775808__" : @{@"a" : @1}, + }]; + + FIRQuery *query = [[[collRef queryWhereFieldPath:[FIRFieldPath documentID] + isGreaterThan:@"__id7__"] + queryWhereFieldPath:[FIRFieldPath documentID] + isLessThanOrEqualTo:@"A"] queryOrderedByFieldPath:[FIRFieldPath documentID]]; + NSArray *expectedDocs = + @[ @"__id12__", @"__id9223372036854775807__", @"12", @"7", @"A" ]; + FIRQuerySnapshot *getSnapshot = [self readDocumentSetForRef:query]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(getSnapshot), expectedDocs); + + id registration = + [query addSnapshotListener:self.eventAccumulator.valueEventHandler]; + FIRQuerySnapshot *watchSnapshot = [self.eventAccumulator awaitEventWithName:@"Snapshot"]; + XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(watchSnapshot), expectedDocs); + + [registration remove]; +} + +- (void)testSdkOrdersQueryByDocumentIdTheSameWayOnlineAndOffline { + FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{ + @"A" : @{@"a" : @1}, + @"a" : @{@"a" : @1}, + @"Aa" : @{@"a" : @1}, + @"7" : @{@"a" : @1}, + @"12" : @{@"a" : @1}, + @"__id7__" : @{@"a" : @1}, + @"__id12__" : @{@"a" : @1}, + @"__id-2__" : @{@"a" : @1}, + @"__id1_" : @{@"a" : @1}, + @"_id1__" : @{@"a" : @1}, + @"__id9223372036854775807__" : @{@"a" : @1}, + @"__id-9223372036854775808__" : @{@"a" : @1}, + }]; + + [self checkOnlineAndOfflineQuery:[collRef queryOrderedByFieldPath:[FIRFieldPath documentID]] + matchesResult:@[ + @"__id-9223372036854775808__", @"__id-2__", @"__id7__", @"__id12__", + @"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id1_", @"_id1__", + @"a" + ]]; +} + - (void)testCollectionGroupQueriesWithWhereFiltersOnArbitraryDocumentIDs { // Use .document() to get a random collection group name to use but ensure it starts with 'b' // for predictable ordering. diff --git a/Firestore/core/src/model/base_path.h b/Firestore/core/src/model/base_path.h index 3d604f6bf4e..22f2e14f929 100644 --- a/Firestore/core/src/model/base_path.h +++ b/Firestore/core/src/model/base_path.h @@ -150,8 +150,18 @@ class BasePath { std::equal(begin(), end(), potential_child.begin()); } + /** + * Compares the current path against another Path object. Paths are compared + * segment by segment, prioritizing numeric IDs (e.g., "__id123__") in numeric + * ascending order, followed by string segments in lexicographical order. + */ util::ComparisonResult CompareTo(const T& rhs) const { - return util::CompareContainer(segments_, rhs.segments_); + size_t min_size = std::min(size(), rhs.size()); + for (size_t i = 0; i < min_size; ++i) { + auto cmp = compareSegments(segments_[i], rhs.segments_[i]); + if (!util::Same(cmp)) return cmp; + } + return util::Compare(size(), rhs.size()); } friend bool operator==(const BasePath& lhs, const BasePath& rhs) { @@ -174,6 +184,31 @@ class BasePath { private: SegmentsT segments_; + + static util::ComparisonResult compareSegments(const std::string& lhs, + const std::string& rhs) { + bool isLhsNumeric = isNumericId(lhs); + bool isRhsNumeric = isNumericId(rhs); + + if (isLhsNumeric && !isRhsNumeric) { + return util::ComparisonResult::Ascending; + } else if (!isLhsNumeric && isRhsNumeric) { + return util::ComparisonResult::Descending; + } else if (isLhsNumeric && isRhsNumeric) { + return util::Compare(extractNumericId(lhs), extractNumericId(rhs)); + } else { + return util::Compare(lhs, rhs); + } + } + + static bool isNumericId(const std::string& segment) { + return segment.substr(0, 4) == "__id" && + segment.substr(segment.size() - 2) == "__"; + } + + static long extractNumericId(const std::string& segment) { + return std::stol(segment.substr(4, segment.size() - 2)); + } }; } // namespace impl From cb9128314c670ec17ae534ff754846062887a9a0 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 12 Dec 2024 16:10:57 -0500 Subject: [PATCH 2/3] Update base_path.h --- Firestore/core/src/model/base_path.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/core/src/model/base_path.h b/Firestore/core/src/model/base_path.h index 22f2e14f929..e3063e0be2b 100644 --- a/Firestore/core/src/model/base_path.h +++ b/Firestore/core/src/model/base_path.h @@ -206,7 +206,7 @@ class BasePath { segment.substr(segment.size() - 2) == "__"; } - static long extractNumericId(const std::string& segment) { + static int64_t extractNumericId(const std::string& segment) { return std::stol(segment.substr(4, segment.size() - 2)); } }; From 2850aec3a01ea951277e83efbc93534a3b93ae46 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 16 Dec 2024 15:13:01 -0500 Subject: [PATCH 3/3] resolve comments --- .../Tests/Integration/API/FIRQueryTests.mm | 9 ++++--- Firestore/core/src/model/base_path.h | 27 ++++++++++++------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 2e18fed01ec..b1cd36a5c49 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -816,6 +816,7 @@ - (void)testSnapshotListenerSortsQueryByDocumentIdInTheSameOrderAsServer { @"__id-2__" : @{@"a" : @1}, @"__id1_" : @{@"a" : @1}, @"_id1__" : @{@"a" : @1}, + @"__id" : @{@"a" : @1}, @"__id9223372036854775807__" : @{@"a" : @1}, @"__id-9223372036854775808__" : @{@"a" : @1}, }]; @@ -823,7 +824,7 @@ - (void)testSnapshotListenerSortsQueryByDocumentIdInTheSameOrderAsServer { FIRQuery *query = [collRef queryOrderedByFieldPath:[FIRFieldPath documentID]]; NSArray *expectedDocs = @[ @"__id-9223372036854775808__", @"__id-2__", @"__id7__", @"__id12__", - @"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id1_", @"_id1__", @"a" + @"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id", @"__id1_", @"_id1__", @"a" ]; FIRQuerySnapshot *getSnapshot = [self readDocumentSetForRef:query]; XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(getSnapshot), expectedDocs); @@ -848,6 +849,7 @@ - (void)testSnapshotListenerSortsFilteredQueryByDocumentIdInTheSameOrderAsServer @"__id-2__" : @{@"a" : @1}, @"__id1_" : @{@"a" : @1}, @"_id1__" : @{@"a" : @1}, + @"__id" : @{@"a" : @1}, @"__id9223372036854775807__" : @{@"a" : @1}, @"__id-9223372036854775808__" : @{@"a" : @1}, }]; @@ -881,6 +883,7 @@ - (void)testSdkOrdersQueryByDocumentIdTheSameWayOnlineAndOffline { @"__id-2__" : @{@"a" : @1}, @"__id1_" : @{@"a" : @1}, @"_id1__" : @{@"a" : @1}, + @"__id" : @{@"a" : @1}, @"__id9223372036854775807__" : @{@"a" : @1}, @"__id-9223372036854775808__" : @{@"a" : @1}, }]; @@ -888,8 +891,8 @@ - (void)testSdkOrdersQueryByDocumentIdTheSameWayOnlineAndOffline { [self checkOnlineAndOfflineQuery:[collRef queryOrderedByFieldPath:[FIRFieldPath documentID]] matchesResult:@[ @"__id-9223372036854775808__", @"__id-2__", @"__id7__", @"__id12__", - @"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id1_", @"_id1__", - @"a" + @"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id", @"__id1_", + @"_id1__", @"a" ]]; } diff --git a/Firestore/core/src/model/base_path.h b/Firestore/core/src/model/base_path.h index e3063e0be2b..e8efa395e8d 100644 --- a/Firestore/core/src/model/base_path.h +++ b/Firestore/core/src/model/base_path.h @@ -158,7 +158,7 @@ class BasePath { util::ComparisonResult CompareTo(const T& rhs) const { size_t min_size = std::min(size(), rhs.size()); for (size_t i = 0; i < min_size; ++i) { - auto cmp = compareSegments(segments_[i], rhs.segments_[i]); + auto cmp = CompareSegments(segments_[i], rhs.segments_[i]); if (!util::Same(cmp)) return cmp; } return util::Compare(size(), rhs.size()); @@ -185,29 +185,36 @@ class BasePath { private: SegmentsT segments_; - static util::ComparisonResult compareSegments(const std::string& lhs, + static const size_t kNumericIdPrefixLength = 4; + static const size_t kNumericIdSuffixLength = 2; + static const size_t kNumericIdTotalOverhead = + kNumericIdPrefixLength + kNumericIdSuffixLength; + + static util::ComparisonResult CompareSegments(const std::string& lhs, const std::string& rhs) { - bool isLhsNumeric = isNumericId(lhs); - bool isRhsNumeric = isNumericId(rhs); + bool isLhsNumeric = IsNumericId(lhs); + bool isRhsNumeric = IsNumericId(rhs); if (isLhsNumeric && !isRhsNumeric) { return util::ComparisonResult::Ascending; } else if (!isLhsNumeric && isRhsNumeric) { return util::ComparisonResult::Descending; } else if (isLhsNumeric && isRhsNumeric) { - return util::Compare(extractNumericId(lhs), extractNumericId(rhs)); + return util::Compare(ExtractNumericId(lhs), ExtractNumericId(rhs)); } else { return util::Compare(lhs, rhs); } } - static bool isNumericId(const std::string& segment) { - return segment.substr(0, 4) == "__id" && - segment.substr(segment.size() - 2) == "__"; + static bool IsNumericId(const std::string& segment) { + return segment.size() > kNumericIdTotalOverhead && + segment.substr(0, kNumericIdPrefixLength) == "__id" && + segment.substr(segment.size() - kNumericIdSuffixLength) == "__"; } - static int64_t extractNumericId(const std::string& segment) { - return std::stol(segment.substr(4, segment.size() - 2)); + static int64_t ExtractNumericId(const std::string& segment) { + return std::stol(segment.substr(kNumericIdPrefixLength, + segment.size() - kNumericIdSuffixLength)); } };