Skip to content

Fix: sort document reference by long type id #8673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion packages/firestore/src/local/memory_remote_document_cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ interface MemoryRemoteDocumentCacheEntry {
size: number;
}

/**
* The smallest value representable by a 64-bit signed integer (long).
*/
const MIN_LONG_VALUE = '-9223372036854775808';

type DocumentEntryMap = SortedMap<DocumentKey, MemoryRemoteDocumentCacheEntry>;
function documentEntryMap(): DocumentEntryMap {
return new SortedMap<DocumentKey, MemoryRemoteDocumentCacheEntry>(
Expand Down Expand Up @@ -171,7 +176,12 @@ class MemoryRemoteDocumentCacheImpl implements MemoryRemoteDocumentCache {
// Documents are ordered by key, so we can use a prefix scan to narrow down
// the documents we need to match the query against.
const collectionPath = query.path;
const prefix = new DocumentKey(collectionPath.child(''));
// Document keys are ordered first by numeric value ("__id<Long>__"),
// then lexicographically by string value. Start the iterator at the minimum
// possible Document key value.
const prefix = new DocumentKey(
collectionPath.child('__id' + MIN_LONG_VALUE + '__')
);
const iterator = this.docs.getIteratorFrom(prefix);
while (iterator.hasNext()) {
const {
Expand Down
55 changes: 44 additions & 11 deletions packages/firestore/src/model/path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
* limitations under the License.
*/

import { Integer } from '@firebase/webchannel-wrapper/bloom-blob';

import { debugAssert, fail } from '../util/assert';
import { Code, FirestoreError } from '../util/error';

Expand Down Expand Up @@ -163,28 +165,59 @@ abstract class BasePath<B extends BasePath<B>> {
return this.segments.slice(this.offset, this.limit());
}

/**
* Compare 2 paths segment by segment, prioritizing numeric IDs
* (e.g., "__id123__") in numeric ascending order, followed by string
* segments in lexicographical order.
*/
static comparator<T extends BasePath<T>>(
p1: BasePath<T>,
p2: BasePath<T>
): number {
const len = Math.min(p1.length, p2.length);
for (let i = 0; i < len; i++) {
const left = p1.get(i);
const right = p2.get(i);
if (left < right) {
return -1;
}
if (left > right) {
return 1;
const comparison = BasePath.compareSegments(p1.get(i), p2.get(i));
if (comparison !== 0) {
return comparison;
}
}
if (p1.length < p2.length) {
return Math.sign(p1.length - p2.length);
}

private static compareSegments(lhs: string, rhs: string): number {
const isLhsNumeric = BasePath.isNumericId(lhs);
const isRhsNumeric = BasePath.isNumericId(rhs);

if (isLhsNumeric && !isRhsNumeric) {
// Only lhs is numeric
return -1;
}
if (p1.length > p2.length) {
} else if (!isLhsNumeric && isRhsNumeric) {
// Only rhs is numeric
return 1;
} else if (isLhsNumeric && isRhsNumeric) {
// both numeric
return BasePath.extractNumericId(lhs).compare(
BasePath.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 static isNumericId(segment: string): boolean {
return segment.startsWith('__id') && segment.endsWith('__');
}

private static extractNumericId(segment: string): Integer {
return Integer.fromString(segment.substring(4, segment.length - 2));
}
}

Expand Down
176 changes: 175 additions & 1 deletion packages/firestore/test/integration/api/database.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ import {
withTestDocAndInitialData,
withNamedTestDbsOrSkipUnlessUsingEmulator,
toDataArray,
checkOnlineAndOfflineResultsMatch
checkOnlineAndOfflineResultsMatch,
toIds
} from '../util/helpers';
import { DEFAULT_SETTINGS, DEFAULT_PROJECT_ID } from '../util/settings';

Expand Down Expand Up @@ -2245,4 +2246,177 @@ apiDescribe('Database', persistence => {
});
});
});

describe('sort documents by DocumentId', () => {
it('snapshot listener sorts query by DocumentId same way as get query', async () => {
const testDocs = {
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 },
// largest long numbers
'__id9223372036854775807__': { a: 1 },
'__id9223372036854775806__': { a: 1 },
// smallest long numbers
'__id-9223372036854775808__': { a: 1 },
'__id-9223372036854775807__': { a: 1 }
};

return withTestCollection(persistence, testDocs, async collectionRef => {
const orderedQuery = query(collectionRef, orderBy(documentId()));
const expectedDocs = [
'__id-9223372036854775808__',
'__id-9223372036854775807__',
'__id-2__',
'__id7__',
'__id12__',
'__id9223372036854775806__',
'__id9223372036854775807__',
'12',
'7',
'A',
'Aa',
'__id1_',
'_id1__',
'a'
];

const getSnapshot = await getDocsFromServer(orderedQuery);
expect(toIds(getSnapshot)).to.deep.equal(expectedDocs);

const storeEvent = new EventsAccumulator<QuerySnapshot>();
const unsubscribe = onSnapshot(orderedQuery, storeEvent.storeEvent);
const watchSnapshot = await storeEvent.awaitEvent();
expect(toIds(watchSnapshot)).to.deep.equal(expectedDocs);

unsubscribe();
});
});

it('snapshot listener sorts filtered query by DocumentId same way as get query', async () => {
const testDocs = {
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 },
// largest long numbers
'__id9223372036854775807__': { a: 1 },
'__id9223372036854775806__': { a: 1 },
// smallest long numbers
'__id-9223372036854775808__': { a: 1 },
'__id-9223372036854775807__': { a: 1 }
};

return withTestCollection(persistence, testDocs, async collectionRef => {
const filteredQuery = query(
collectionRef,
orderBy(documentId()),
where(documentId(), '>', '__id7__'),
where(documentId(), '<=', 'Aa')
);
const expectedDocs = [
'__id12__',
'__id9223372036854775806__',
'__id9223372036854775807__',
'12',
'7',
'A',
'Aa'
];

const getSnapshot = await getDocsFromServer(filteredQuery);
expect(toIds(getSnapshot)).to.deep.equal(expectedDocs);

const storeEvent = new EventsAccumulator<QuerySnapshot>();
const unsubscribe = onSnapshot(filteredQuery, storeEvent.storeEvent);
const watchSnapshot = await storeEvent.awaitEvent();
expect(toIds(watchSnapshot)).to.deep.equal(expectedDocs);
unsubscribe();
});
});

// eslint-disable-next-line no-restricted-properties
(persistence.gc === 'lru' ? describe : describe.skip)('offline', () => {
it('SDK orders query the same way online and offline', async () => {
const testDocs = {
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 },
// largest long numbers
'__id9223372036854775807__': { a: 1 },
'__id9223372036854775806__': { a: 1 },
// smallest long numbers
'__id-9223372036854775808__': { a: 1 },
'__id-9223372036854775807__': { a: 1 }
};

return withTestCollection(
persistence,
testDocs,
async collectionRef => {
const orderedQuery = query(collectionRef, orderBy(documentId()));
let expectedDocs = [
'__id-9223372036854775808__',
'__id-9223372036854775807__',
'__id-2__',
'__id7__',
'__id12__',
'__id9223372036854775806__',
'__id9223372036854775807__',
'12',
'7',
'A',
'Aa',
'__id1_',
'_id1__',
'a'
];
await checkOnlineAndOfflineResultsMatch(
orderedQuery,
...expectedDocs
);

const filteredQuery = query(
collectionRef,
orderBy(documentId()),
where(documentId(), '>', '__id7__'),
where(documentId(), '<=', 'Aa')
);
expectedDocs = [
'__id12__',
'__id9223372036854775806__',
'__id9223372036854775807__',
'12',
'7',
'A',
'Aa'
];
await checkOnlineAndOfflineResultsMatch(
filteredQuery,
...expectedDocs
);
}
);
});
});
});
});
Loading