diff --git a/.changeset/tricky-areas-drop.md b/.changeset/tricky-areas-drop.md new file mode 100644 index 000000000..f9fc6310c --- /dev/null +++ b/.changeset/tricky-areas-drop.md @@ -0,0 +1,5 @@ +--- +"@tanstack/db": patch +--- + +fix string comparison when sorting in descending order diff --git a/packages/db/src/query/order-by.ts b/packages/db/src/query/order-by.ts index 0ae74e10a..351e69108 100644 --- a/packages/db/src/query/order-by.ts +++ b/packages/db/src/query/order-by.ts @@ -13,6 +13,13 @@ import type { NamespacedRow, } from "../types" +type OrderByItem = { + operand: ConditionOperand + direction: `asc` | `desc` +} + +type OrderByItems = Array + export function processOrderBy( resultPipeline: NamespacedAndKeyedStream, query: Query, @@ -41,10 +48,7 @@ export function processOrderBy( } // Normalize orderBy to an array of objects - const orderByItems: Array<{ - operand: ConditionOperand - direction: `asc` | `desc` - }> = [] + const orderByItems: OrderByItems = [] if (typeof query.orderBy === `string`) { // Handle string format: '@column' @@ -84,22 +88,13 @@ export function processOrderBy( const valueExtractor = (namespacedRow: NamespacedRow) => { // For multiple orderBy columns, create a composite key if (orderByItems.length > 1) { - return orderByItems.map((item) => { - const val = evaluateOperandOnNamespacedRow( + return orderByItems.map((item) => + evaluateOperandOnNamespacedRow( namespacedRow, item.operand, mainTableAlias ) - - // Reverse the value for 'desc' ordering - return item.direction === `desc` && typeof val === `number` - ? -val - : item.direction === `desc` && typeof val === `string` - ? String.fromCharCode( - ...[...val].map((c) => 0xffff - c.charCodeAt(0)) - ) - : val - }) + ) } else if (orderByItems.length === 1) { // For a single orderBy column, use the value directly const item = orderByItems[0] @@ -108,22 +103,14 @@ export function processOrderBy( item!.operand, mainTableAlias ) - - // Reverse the value for 'desc' ordering - return item!.direction === `desc` && typeof val === `number` - ? -val - : item!.direction === `desc` && typeof val === `string` - ? String.fromCharCode( - ...[...val].map((c) => 0xffff - c.charCodeAt(0)) - ) - : val + return val } // Default case - no ordering return null } - const comparator = (a: unknown, b: unknown): number => { + const ascComparator = (a: unknown, b: unknown): number => { // if a and b are both numbers compare them directly if (typeof a === `number` && typeof b === `number`) { return a - b @@ -184,6 +171,44 @@ export function processOrderBy( return (a as any).toString().localeCompare((b as any).toString()) } + const descComparator = (a: unknown, b: unknown): number => { + return ascComparator(b, a) + } + + // Create a multi-property comparator that respects the order and direction of each property + const makeComparator = (orderByProps: OrderByItems) => { + return (a: unknown, b: unknown) => { + // If we're comparing arrays (multiple properties), compare each property in order + if (orderByProps.length > 1) { + // `a` and `b` must be arrays since `orderByItems.length > 1` + // hence the extracted values must be arrays + const arrayA = a as Array + const arrayB = b as Array + for (let i = 0; i < orderByProps.length; i++) { + const direction = orderByProps[i]!.direction + const compareFn = + direction === `desc` ? descComparator : ascComparator + const result = compareFn(arrayA[i], arrayB[i]) + if (result !== 0) { + return result + } + } + // should normally always be 0 because + // both values are extracted based on orderByItems + return arrayA.length - arrayB.length + } + + // Single property comparison + if (orderByProps.length === 1) { + const direction = orderByProps[0]!.direction + return direction === `desc` ? descComparator(a, b) : ascComparator(a, b) + } + + return ascComparator(a, b) + } + } + const comparator = makeComparator(orderByItems) + // Apply the appropriate orderBy operator based on whether an ORDER_INDEX column is requested if (hasOrderIndexColumn) { if (orderIndexType === `numeric`) { diff --git a/packages/db/tests/query/query-collection.test.ts b/packages/db/tests/query/query-collection.test.ts index 2a8ece301..22926a533 100644 --- a/packages/db/tests/query/query-collection.test.ts +++ b/packages/db/tests/query/query-collection.test.ts @@ -681,122 +681,147 @@ describe(`Query Collections`, () => { expect(result.state.get(`KEY::${result.id}/[3,1]`)).toBeUndefined() }) - it(`should order results by specified fields`, async () => { - const emitter = mitt() - - // Create collection with mutation capability - const collection = createCollection({ - id: `order-by-test`, - getId: (item) => item.id, - sync: { - sync: ({ begin, write, commit }) => { - emitter.on(`sync`, (changes) => { - begin() - ;(changes as Array).forEach((change) => { - write({ - type: change.type, - value: change.changes as Person, + it( + `should order results by specified fields`, + async () => { + const emitter = mitt() + + // Create collection with mutation capability + const collection = createCollection({ + id: `order-by-test`, + getId: (item) => item.id, + sync: { + sync: ({ begin, write, commit }) => { + emitter.on(`sync`, (changes) => { + begin() + ;(changes as Array).forEach((change) => { + write({ + type: change.type, + value: change.changes as Person, + }) }) + commit() }) - commit() - }) + }, }, - }, - }) - - // Sync from initial state - emitter.emit( - `sync`, - initialPersons.map((person) => ({ - type: `insert`, - changes: person, - })) - ) - - // Test ascending order by age - const ascendingQuery = queryBuilder() - .from({ collection }) - .orderBy(`@age`) - .select(`@id`, `@name`, `@age`) - - const compiledAscendingQuery = compileQuery(ascendingQuery) - compiledAscendingQuery.start() - - const ascendingResult = compiledAscendingQuery.results - - await waitForChanges() - - // Verify ascending order - const ascendingArray = Array.from(ascendingResult.toArray) - expect(ascendingArray).toEqual([ - { _key: `2`, id: `2`, name: `Jane Doe`, age: 25, _orderByIndex: 0 }, - { _key: `1`, id: `1`, name: `John Doe`, age: 30, _orderByIndex: 1 }, - { _key: `3`, id: `3`, name: `John Smith`, age: 35, _orderByIndex: 2 }, - ]) - - // Test descending order by age - const descendingQuery = queryBuilder() - .from({ collection }) - .orderBy({ "@age": `desc` }) - .select(`@id`, `@name`, `@age`) - - const compiledDescendingQuery = compileQuery(descendingQuery) - compiledDescendingQuery.start() - - const descendingResult = compiledDescendingQuery.results - - await waitForChanges() - - // Verify descending order - const descendingArray = Array.from(descendingResult.toArray) - expect(descendingArray).toEqual([ - { _key: `3`, id: `3`, name: `John Smith`, age: 35, _orderByIndex: 0 }, - { _key: `1`, id: `1`, name: `John Doe`, age: 30, _orderByIndex: 1 }, - { _key: `2`, id: `2`, name: `Jane Doe`, age: 25, _orderByIndex: 2 }, - ]) - - // Test multiple order by fields - const multiOrderQuery = queryBuilder() - .from({ collection }) - .orderBy([`@isActive`, { "@age": `asc` }]) - .select(`@id`, `@name`, `@age`, `@isActive`) - - const compiledMultiOrderQuery = compileQuery(multiOrderQuery) - compiledMultiOrderQuery.start() - - const multiOrderResult = compiledMultiOrderQuery.results - - await waitForChanges() + }) - // Verify multiple field ordering - const multiOrderArray = Array.from(multiOrderResult.toArray) - expect(multiOrderArray).toEqual([ - { - _key: `3`, - id: `3`, - name: `John Smith`, - age: 35, - isActive: false, - _orderByIndex: 0, - }, - { - _key: `2`, - id: `2`, - name: `Jane Doe`, - age: 25, - isActive: true, - _orderByIndex: 1, - }, - { - _key: `1`, - id: `1`, - name: `John Doe`, - age: 30, - isActive: true, - _orderByIndex: 2, - }, - ]) - }) + // Sync from initial state + emitter.emit( + `sync`, + initialPersons.map((person) => ({ + type: `insert`, + changes: person, + })) + ) + + // Test ascending order by age + const ascendingQuery = queryBuilder() + .from({ collection }) + .orderBy(`@age`) + .select(`@id`, `@name`, `@age`) + + const compiledAscendingQuery = compileQuery(ascendingQuery) + compiledAscendingQuery.start() + + const ascendingResult = compiledAscendingQuery.results + + await waitForChanges() + + // Verify ascending order + const ascendingArray = Array.from(ascendingResult.toArray) + expect(ascendingArray).toEqual([ + { _key: `2`, id: `2`, name: `Jane Doe`, age: 25, _orderByIndex: 0 }, + { _key: `1`, id: `1`, name: `John Doe`, age: 30, _orderByIndex: 1 }, + { _key: `3`, id: `3`, name: `John Smith`, age: 35, _orderByIndex: 2 }, + ]) + + // Test descending order by age + const descendingQuery = queryBuilder() + .from({ collection }) + .orderBy({ "@age": `desc` }) + .select(`@id`, `@name`, `@age`) + + const compiledDescendingQuery = compileQuery(descendingQuery) + compiledDescendingQuery.start() + + const descendingResult = compiledDescendingQuery.results + + await waitForChanges() + + // Verify descending order + const descendingArray = Array.from(descendingResult.toArray) + expect(descendingArray).toEqual([ + { _key: `3`, id: `3`, name: `John Smith`, age: 35, _orderByIndex: 0 }, + { _key: `1`, id: `1`, name: `John Doe`, age: 30, _orderByIndex: 1 }, + { _key: `2`, id: `2`, name: `Jane Doe`, age: 25, _orderByIndex: 2 }, + ]) + + // Test descending order by name + const descendingNameQuery = queryBuilder() + .from({ collection }) + .orderBy({ "@name": `desc` }) + .select(`@id`, `@name`, `@age`) + + const compiledDescendingNameQuery = compileQuery(descendingNameQuery) + compiledDescendingNameQuery.start() + + const descendingNameResult = compiledDescendingNameQuery.results + + await waitForChanges() + + // Verify descending order by name + const descendingNameArray = Array.from(descendingNameResult.toArray) + expect(descendingNameArray).toEqual([ + { _key: `3`, id: `3`, name: `John Smith`, age: 35, _orderByIndex: 0 }, + { _key: `1`, id: `1`, name: `John Doe`, age: 30, _orderByIndex: 1 }, + { _key: `2`, id: `2`, name: `Jane Doe`, age: 25, _orderByIndex: 2 }, + ]) + + // Test multiple order by fields + const multiOrderQuery = queryBuilder() + .from({ collection }) + .orderBy([`@isActive`, { "@name": `desc` }]) + .select(`@id`, `@name`, `@age`, `@isActive`) + + const compiledMultiOrderQuery = compileQuery(multiOrderQuery) + compiledMultiOrderQuery.start() + + const multiOrderResult = compiledMultiOrderQuery.results + + await waitForChanges() + + // Verify multiple field ordering + const multiOrderArray = Array.from(multiOrderResult.toArray) + expect(multiOrderArray).toEqual([ + { + _key: `3`, + id: `3`, + name: `John Smith`, + age: 35, + isActive: false, + _orderByIndex: 0, + }, + { + _key: `1`, + id: `1`, + name: `John Doe`, + age: 30, + isActive: true, + _orderByIndex: 1, + }, + { + _key: `2`, + id: `2`, + name: `Jane Doe`, + age: 25, + isActive: true, + _orderByIndex: 2, + }, + ]) + }, + { timeout: 250_000 } + ) it(`should maintain correct ordering when items are added, updated, or deleted`, async () => { const emitter = mitt()