fix: string comparison for sorting in descending order

.changeset/tricky-areas-drop.md

@@ -0,0 +1,5 @@
+---
+"@tanstack/db": patch
+---
+
+fix string comparison when sorting in descending order

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<OrderByItem>
+
 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<unknown>
+        const arrayB = b as Array<unknown>
+        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`) {

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<Person>({
-      id: `order-by-test`,
-      getId: (item) => item.id,
-      sync: {
-        sync: ({ begin, write, commit }) => {
-          emitter.on(`sync`, (changes) => {
-            begin()
-            ;(changes as Array<PendingMutation>).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<Person>({
+        id: `order-by-test`,
+        getId: (item) => item.id,
+        sync: {
+          sync: ({ begin, write, commit }) => {
+            emitter.on(`sync`, (changes) => {
+              begin()
+              ;(changes as Array<PendingMutation>).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()