From 19300ef559617186bec7d4551dac5860ed52aa68 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 6 Dec 2024 16:41:05 -0500 Subject: [PATCH 1/7] change the logic for documentId comparison --- dev/src/path.ts | 52 +++++++++++++++++++---- dev/system-test/firestore.ts | 81 ++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 9 deletions(-) diff --git a/dev/src/path.ts b/dev/src/path.ts index 92a20c14d..6834918ed 100644 --- a/dev/src/path.ts +++ b/dev/src/path.ts @@ -150,6 +150,10 @@ abstract class Path { /** * Compare the current path against another Path object. * + * Compare 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. + * * @private * @internal * @param other The path to compare to. @@ -158,20 +162,50 @@ abstract class Path { compareTo(other: Path): number { const len = Math.min(this.segments.length, other.segments.length); for (let i = 0; i < len; i++) { - if (this.segments[i] < other.segments[i]) { - return -1; - } - if (this.segments[i] > other.segments[i]) { - return 1; + const comparison = this.compareSegments( + this.segments[i], + other.segments[i] + ); + if (comparison !== 0) { + return comparison; } } - if (this.segments.length < other.segments.length) { + return this.segments.length - other.segments.length; + } + + private compareSegments(lhs: string, rhs: string): number { + const isLhsNumeric = this.isNumericId(lhs); + const isRhsNumeric = this.isNumericId(rhs); + + if (isLhsNumeric && !isRhsNumeric) { + // Only lhs is numeric return -1; - } - if (this.segments.length > other.segments.length) { + } else if (!isLhsNumeric && isRhsNumeric) { + // Only rhs is numeric return 1; + } else if (isLhsNumeric && isRhsNumeric) { + // both numeric + return this.extractNumericId(lhs) - this.extractNumericId(rhs); + } else { + // both non-numeric + if (lhs < rhs) { + return -1; + } + if (lhs > rhs) { + return 1; + } + return 0; } - return 0; + } + + // Checks if a segment is a numeric ID (starts with "__id" and ends with "__"). + private isNumericId(segment: string): boolean { + return segment.startsWith('__id') && segment.endsWith('__'); + } + + // Extracts the numeric value from a numeric ID segment. + private extractNumericId(segment: string): number { + return parseInt(segment.substring(4, segment.length - 2), 10); } /** diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index 2afb8b688..ff6bc5549 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -3913,6 +3913,87 @@ describe('Query class', () => { unsubscribe(); }); + it('snapshot listener sorts query by DocumentId same way as server', async () => { + const batch = firestore.batch(); + + batch.set(randomCol.doc('A'), {a: 1}); + batch.set(randomCol.doc('a'), {a: 1}); + batch.set(randomCol.doc('Aa'), {a: 1}); + batch.set(randomCol.doc('7'), {a: 1}); + batch.set(randomCol.doc('12'), {a: 1}); + batch.set(randomCol.doc('__id7__'), {a: 1}); + batch.set(randomCol.doc('__id12__'), {a: 1}); + batch.set(randomCol.doc('__id1_'), {a: 1}); + batch.set(randomCol.doc('_id1__'), {a: 1}); + await batch.commit(); + + const query = randomCol.orderBy(FieldPath.documentId()); + const expectedDocs = [ + '__id7__', + '__id12__', + '12', + '7', + 'A', + 'Aa', + '__id1_', + '_id1__', + 'a', + ]; + + const getSnapshot = await query.get(); + expect(getSnapshot.docs.map(d => d.id)).to.deep.equal(expectedDocs); + + const unsubscribe = query.onSnapshot(snapshot => + currentDeferred.resolve(snapshot) + ); + + const watchSnapshot = await waitForSnapshot(); + // Compare the snapshot (including sort order) of a snapshot + // from Query.onSnapshot() to an actual snapshot from Query.get() + snapshotsEqual(watchSnapshot, { + docs: getSnapshot.docs, + docChanges: getSnapshot.docChanges(), + }); + unsubscribe(); + }); + + it('snapshot listener sorts filtered query by DocumentId same way as server', async () => { + const batch = firestore.batch(); + + batch.set(randomCol.doc('A'), {a: 1}); + batch.set(randomCol.doc('a'), {a: 1}); + batch.set(randomCol.doc('Aa'), {a: 1}); + batch.set(randomCol.doc('7'), {a: 1}); + batch.set(randomCol.doc('12'), {a: 1}); + batch.set(randomCol.doc('__id7__'), {a: 1}); + batch.set(randomCol.doc('__id12__'), {a: 1}); + batch.set(randomCol.doc('__id1_'), {a: 1}); + batch.set(randomCol.doc('_id1__'), {a: 1}); + await batch.commit(); + + const query = randomCol + .where(FieldPath.documentId(), '>', '__id7__') + .where(FieldPath.documentId(), '<=', 'A') + .orderBy(FieldPath.documentId()); + const expectedDocs = ['__id12__', '12', '7', 'A']; + + const getSnapshot = await query.get(); + expect(getSnapshot.docs.map(d => d.id)).to.deep.equal(expectedDocs); + + const unsubscribe = query.onSnapshot(snapshot => + currentDeferred.resolve(snapshot) + ); + + const watchSnapshot = await waitForSnapshot(); + // Compare the snapshot (including sort order) of a snapshot + // from Query.onSnapshot() to an actual snapshot from Query.get() + snapshotsEqual(watchSnapshot, { + docs: getSnapshot.docs, + docChanges: getSnapshot.docChanges(), + }); + unsubscribe(); + }); + it('SDK orders vector field same way as backend', async () => { // We validate that the SDK orders the vector field the same way as the backend // by comparing the sort order of vector fields from a Query.get() and From 679cba83a0738ea5092469d2cf0fdaf950f4b455 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 6 Dec 2024 16:54:18 -0500 Subject: [PATCH 2/7] use Math.sign to compare numeric values --- dev/src/path.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/src/path.ts b/dev/src/path.ts index 6834918ed..0322c5ea1 100644 --- a/dev/src/path.ts +++ b/dev/src/path.ts @@ -170,7 +170,7 @@ abstract class Path { return comparison; } } - return this.segments.length - other.segments.length; + return Math.sign(this.segments.length - other.segments.length); } private compareSegments(lhs: string, rhs: string): number { @@ -185,7 +185,7 @@ abstract class Path { return 1; } else if (isLhsNumeric && isRhsNumeric) { // both numeric - return this.extractNumericId(lhs) - this.extractNumericId(rhs); + return Math.sign(this.extractNumericId(lhs) - this.extractNumericId(rhs)); } else { // both non-numeric if (lhs < rhs) { From 4b6c02c764d26c75dbbc2042d8bf055aa4b56fcd Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 10 Dec 2024 13:35:01 -0500 Subject: [PATCH 3/7] add largest and smallest long number to the test --- dev/system-test/firestore.ts | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index ff6bc5549..c34598e99 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -3915,7 +3915,6 @@ describe('Query class', () => { it('snapshot listener sorts query by DocumentId same way as server', async () => { const batch = firestore.batch(); - batch.set(randomCol.doc('A'), {a: 1}); batch.set(randomCol.doc('a'), {a: 1}); batch.set(randomCol.doc('Aa'), {a: 1}); @@ -3923,14 +3922,22 @@ describe('Query class', () => { batch.set(randomCol.doc('12'), {a: 1}); batch.set(randomCol.doc('__id7__'), {a: 1}); batch.set(randomCol.doc('__id12__'), {a: 1}); + batch.set(randomCol.doc('__id-2__'), {a: 1}); batch.set(randomCol.doc('__id1_'), {a: 1}); batch.set(randomCol.doc('_id1__'), {a: 1}); + // largest long number + batch.set(randomCol.doc('__id9223372036854775807__'), {a: 1}); + // smallest long number + batch.set(randomCol.doc('__id-9223372036854775808__'), {a: 1}); await batch.commit(); const query = randomCol.orderBy(FieldPath.documentId()); const expectedDocs = [ + '__id-9223372036854775808__', + '__id-2__', '__id7__', '__id12__', + '__id9223372036854775807__', '12', '7', 'A', @@ -3959,7 +3966,6 @@ describe('Query class', () => { it('snapshot listener sorts filtered query by DocumentId same way as server', async () => { const batch = firestore.batch(); - batch.set(randomCol.doc('A'), {a: 1}); batch.set(randomCol.doc('a'), {a: 1}); batch.set(randomCol.doc('Aa'), {a: 1}); @@ -3967,15 +3973,26 @@ describe('Query class', () => { batch.set(randomCol.doc('12'), {a: 1}); batch.set(randomCol.doc('__id7__'), {a: 1}); batch.set(randomCol.doc('__id12__'), {a: 1}); + batch.set(randomCol.doc('__id-2__'), {a: 1}); batch.set(randomCol.doc('__id1_'), {a: 1}); batch.set(randomCol.doc('_id1__'), {a: 1}); + // largest long number + batch.set(randomCol.doc('__id9223372036854775807__'), {a: 1}); + // smallest long number + batch.set(randomCol.doc('__id-9223372036854775808__'), {a: 1}); await batch.commit(); const query = randomCol .where(FieldPath.documentId(), '>', '__id7__') .where(FieldPath.documentId(), '<=', 'A') .orderBy(FieldPath.documentId()); - const expectedDocs = ['__id12__', '12', '7', 'A']; + const expectedDocs = [ + '__id12__', + '__id9223372036854775807__', + '12', + '7', + 'A', + ]; const getSnapshot = await query.get(); expect(getSnapshot.docs.map(d => d.id)).to.deep.equal(expectedDocs); From afa527c0328a433fa8d18adca365f22a9270bd55 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 12 Dec 2024 14:55:06 -0500 Subject: [PATCH 4/7] remove math.sign comparision on numbers --- dev/src/path.ts | 39 ++++++++++++++++++++++++++---------- dev/system-test/firestore.ts | 9 +++++++-- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/dev/src/path.ts b/dev/src/path.ts index 0322c5ea1..73a57d0b7 100644 --- a/dev/src/path.ts +++ b/dev/src/path.ts @@ -185,16 +185,13 @@ abstract class Path { return 1; } else if (isLhsNumeric && isRhsNumeric) { // both numeric - return Math.sign(this.extractNumericId(lhs) - this.extractNumericId(rhs)); + return this.compareNumbers( + this.extractNumericId(lhs), + this.extractNumericId(rhs) + ); } else { // both non-numeric - if (lhs < rhs) { - return -1; - } - if (lhs > rhs) { - return 1; - } - return 0; + return this.compareStrings(lhs, rhs); } } @@ -203,9 +200,29 @@ abstract class Path { return segment.startsWith('__id') && segment.endsWith('__'); } - // Extracts the numeric value from a numeric ID segment. - private extractNumericId(segment: string): number { - return parseInt(segment.substring(4, segment.length - 2), 10); + // Extracts the long number from a numeric ID segment. + private extractNumericId(segment: string): bigint { + return BigInt(segment.substring(4, segment.length - 2)); + } + + private compareNumbers(lhs: bigint, rhs: bigint): number { + if (lhs < rhs) { + return -1; + } else if (lhs > rhs) { + return 1; + } else { + return 0; + } + } + + private compareStrings(lhs: string, rhs: string): number { + if (lhs < rhs) { + return -1; + } else if (lhs > rhs) { + return 1; + } else { + return 0; + } } /** diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index c34598e99..07cdcb5b3 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -3927,16 +3927,20 @@ describe('Query class', () => { batch.set(randomCol.doc('_id1__'), {a: 1}); // largest long number batch.set(randomCol.doc('__id9223372036854775807__'), {a: 1}); + batch.set(randomCol.doc('__id9223372036854775806__'), {a: 1}); // smallest long number batch.set(randomCol.doc('__id-9223372036854775808__'), {a: 1}); + batch.set(randomCol.doc('__id-9223372036854775807__'), {a: 1}); await batch.commit(); const query = randomCol.orderBy(FieldPath.documentId()); const expectedDocs = [ '__id-9223372036854775808__', + '__id-9223372036854775807__', '__id-2__', '__id7__', '__id12__', + '__id9223372036854775806__', '__id9223372036854775807__', '12', '7', @@ -3956,7 +3960,6 @@ describe('Query class', () => { const watchSnapshot = await waitForSnapshot(); // Compare the snapshot (including sort order) of a snapshot - // from Query.onSnapshot() to an actual snapshot from Query.get() snapshotsEqual(watchSnapshot, { docs: getSnapshot.docs, docChanges: getSnapshot.docChanges(), @@ -3978,8 +3981,10 @@ describe('Query class', () => { batch.set(randomCol.doc('_id1__'), {a: 1}); // largest long number batch.set(randomCol.doc('__id9223372036854775807__'), {a: 1}); + batch.set(randomCol.doc('__id9223372036854775806__'), {a: 1}); // smallest long number batch.set(randomCol.doc('__id-9223372036854775808__'), {a: 1}); + batch.set(randomCol.doc('__id-9223372036854775807__'), {a: 1}); await batch.commit(); const query = randomCol @@ -3988,6 +3993,7 @@ describe('Query class', () => { .orderBy(FieldPath.documentId()); const expectedDocs = [ '__id12__', + '__id9223372036854775806__', '__id9223372036854775807__', '12', '7', @@ -4003,7 +4009,6 @@ describe('Query class', () => { const watchSnapshot = await waitForSnapshot(); // Compare the snapshot (including sort order) of a snapshot - // from Query.onSnapshot() to an actual snapshot from Query.get() snapshotsEqual(watchSnapshot, { docs: getSnapshot.docs, docChanges: getSnapshot.docChanges(), From 81e70a48625c2c1515d20dd6fe778066b23551d5 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 12 Dec 2024 15:01:47 -0500 Subject: [PATCH 5/7] Update path.ts --- dev/src/path.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dev/src/path.ts b/dev/src/path.ts index 73a57d0b7..05e456cd8 100644 --- a/dev/src/path.ts +++ b/dev/src/path.ts @@ -170,7 +170,10 @@ abstract class Path { return comparison; } } - return Math.sign(this.segments.length - other.segments.length); + return this.compareNumbers( + BigInt(this.segments.length), + BigInt(other.segments.length) + ); } private compareSegments(lhs: string, rhs: string): number { From e49959d86398cdef6a0312dfa54fa9b5ab378f45 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 13 Dec 2024 11:13:26 -0500 Subject: [PATCH 6/7] resolve comment --- dev/src/path.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/dev/src/path.ts b/dev/src/path.ts index 05e456cd8..73a57d0b7 100644 --- a/dev/src/path.ts +++ b/dev/src/path.ts @@ -170,10 +170,7 @@ abstract class Path { return comparison; } } - return this.compareNumbers( - BigInt(this.segments.length), - BigInt(other.segments.length) - ); + return Math.sign(this.segments.length - other.segments.length); } private compareSegments(lhs: string, rhs: string): number { From a338b77df3c3eff07493a2631e0453cd6b2f93a0 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 16 Dec 2024 15:25:06 -0500 Subject: [PATCH 7/7] add "__id" to the tests --- dev/system-test/firestore.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index 07cdcb5b3..f57c2767c 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -3925,6 +3925,7 @@ describe('Query class', () => { batch.set(randomCol.doc('__id-2__'), {a: 1}); batch.set(randomCol.doc('__id1_'), {a: 1}); batch.set(randomCol.doc('_id1__'), {a: 1}); + batch.set(randomCol.doc('__id'), {a: 1}); // largest long number batch.set(randomCol.doc('__id9223372036854775807__'), {a: 1}); batch.set(randomCol.doc('__id9223372036854775806__'), {a: 1}); @@ -3946,6 +3947,7 @@ describe('Query class', () => { '7', 'A', 'Aa', + '__id', '__id1_', '_id1__', 'a', @@ -3979,6 +3981,7 @@ describe('Query class', () => { batch.set(randomCol.doc('__id-2__'), {a: 1}); batch.set(randomCol.doc('__id1_'), {a: 1}); batch.set(randomCol.doc('_id1__'), {a: 1}); + batch.set(randomCol.doc('__id'), {a: 1}); // largest long number batch.set(randomCol.doc('__id9223372036854775807__'), {a: 1}); batch.set(randomCol.doc('__id9223372036854775806__'), {a: 1});