diff --git a/.changeset/quick-tigers-talk.md b/.changeset/quick-tigers-talk.md new file mode 100644 index 000000000..361c51e4f --- /dev/null +++ b/.changeset/quick-tigers-talk.md @@ -0,0 +1,5 @@ +--- +"@tanstack/db": patch +--- + +Fix bug where optimized queries would use the wrong index because the index is on the right column but was built using different comparison options (e.g. different direction, string sort, or null ordering). diff --git a/packages/db/src/collection/index.ts b/packages/db/src/collection/index.ts index 63e818799..ac944d6eb 100644 --- a/packages/db/src/collection/index.ts +++ b/packages/db/src/collection/index.ts @@ -428,7 +428,10 @@ export class CollectionImpl< * // Create a ordered index with custom options * const ageIndex = collection.createIndex((row) => row.age, { * indexType: BTreeIndex, - * options: { compareFn: customComparator }, + * options: { + * compareFn: customComparator, + * compareOptions: { direction: 'asc', nulls: 'first', stringSort: 'lexical' } + * }, * name: 'age_btree' * }) * diff --git a/packages/db/src/indexes/auto-index.ts b/packages/db/src/indexes/auto-index.ts index b7caef579..84553d94e 100644 --- a/packages/db/src/indexes/auto-index.ts +++ b/packages/db/src/indexes/auto-index.ts @@ -1,4 +1,6 @@ +import { DEFAULT_COMPARE_OPTIONS } from "../utils" import { BTreeIndex } from "./btree-index" +import type { CompareOptions } from "../query/builder/types" import type { BasicExpression } from "../query/ir" import type { CollectionImpl } from "../collection/index.js" @@ -30,6 +32,7 @@ export function ensureIndexForField< fieldName: string, fieldPath: Array, collection: CollectionImpl, + compareOptions: CompareOptions = DEFAULT_COMPARE_OPTIONS, compareFn?: (a: any, b: any) => number ) { if (!shouldAutoIndex(collection)) { @@ -37,8 +40,10 @@ export function ensureIndexForField< } // Check if we already have an index for this field - const existingIndex = Array.from(collection.indexes.values()).find((index) => - index.matchesField(fieldPath) + const existingIndex = Array.from(collection.indexes.values()).find( + (index) => + index.matchesField(fieldPath) && + index.matchesCompareOptions(compareOptions) ) if (existingIndex) { @@ -50,7 +55,7 @@ export function ensureIndexForField< collection.createIndex((row) => (row as any)[fieldName], { name: `auto_${fieldName}`, indexType: BTreeIndex, - options: compareFn ? { compareFn } : {}, + options: compareFn ? { compareFn, compareOptions } : {}, }) } catch (error) { console.warn(`Failed to create auto-index for field "${fieldName}":`, error) diff --git a/packages/db/src/indexes/base-index.ts b/packages/db/src/indexes/base-index.ts index 80544fc90..66859e920 100644 --- a/packages/db/src/indexes/base-index.ts +++ b/packages/db/src/indexes/base-index.ts @@ -1,5 +1,7 @@ import { compileSingleRowExpression } from "../query/compiler/evaluators.js" import { comparisonFunctions } from "../query/builder/functions.js" +import { DEFAULT_COMPARE_OPTIONS, deepEquals } from "../utils.js" +import type { CompareOptions } from "../query/builder/types.js" import type { BasicExpression } from "../query/ir.js" /** @@ -36,6 +38,7 @@ export abstract class BaseIndex< protected lookupCount = 0 protected totalLookupTime = 0 protected lastUpdated = new Date() + protected compareOptions: CompareOptions constructor( id: number, @@ -45,6 +48,7 @@ export abstract class BaseIndex< ) { this.id = id this.expression = expression + this.compareOptions = DEFAULT_COMPARE_OPTIONS this.name = name this.initialize(options) } @@ -76,6 +80,10 @@ export abstract class BaseIndex< ) } + matchesCompareOptions(compareOptions: CompareOptions): boolean { + return deepEquals(this.compareOptions, compareOptions) + } + getStats(): IndexStats { return { entryCount: this.keyCount, diff --git a/packages/db/src/indexes/btree-index.ts b/packages/db/src/indexes/btree-index.ts index 76b7dab78..64ad70567 100644 --- a/packages/db/src/indexes/btree-index.ts +++ b/packages/db/src/indexes/btree-index.ts @@ -1,6 +1,7 @@ import { BTree } from "../utils/btree.js" import { defaultComparator, normalizeValue } from "../utils/comparison.js" import { BaseIndex } from "./base-index.js" +import type { CompareOptions } from "../query/builder/types.js" import type { BasicExpression } from "../query/ir.js" import type { IndexOperation } from "./base-index.js" @@ -9,6 +10,7 @@ import type { IndexOperation } from "./base-index.js" */ export interface BTreeIndexOptions { compareFn?: (a: any, b: any) => number + compareOptions?: CompareOptions } /** @@ -53,6 +55,9 @@ export class BTreeIndex< ) { super(id, expression, name, options) this.compareFn = options?.compareFn ?? defaultComparator + if (options?.compareOptions) { + this.compareOptions = options!.compareOptions + } this.orderedEntries = new BTree(this.compareFn) } @@ -248,10 +253,12 @@ export class BTreeIndex< take(n: number, from?: any, filterFn?: (key: TKey) => boolean): Array { const keysInResult: Set = new Set() const result: Array = [] - const nextKey = (k?: any) => this.orderedEntries.nextHigherKey(k) + const nextPair = (k?: any) => this.orderedEntries.nextHigherPair(k) + let pair: [any, any] | undefined let key = normalizeValue(from) - while ((key = nextKey(key)) && result.length < n) { + while ((pair = nextPair(key)) !== undefined && result.length < n) { + key = pair[0] const keys = this.valueMap.get(key) if (keys) { const it = keys.values() diff --git a/packages/db/src/query/compiler/order-by.ts b/packages/db/src/query/compiler/order-by.ts index 240b7e856..42b3c04b8 100644 --- a/packages/db/src/query/compiler/order-by.ts +++ b/packages/db/src/query/compiler/order-by.ts @@ -132,6 +132,7 @@ export function processOrderBy( fieldName, followRefResult.path, followRefCollection, + clause.compareOptions, compare ) } @@ -152,7 +153,8 @@ export function processOrderBy( const index: BaseIndex | undefined = findIndexForField( followRefCollection.indexes, - followRefResult.path + followRefResult.path, + clause.compareOptions ) if (index && index.supports(`gt`)) { diff --git a/packages/db/src/utils.ts b/packages/db/src/utils.ts index 5ad117792..5f40934e1 100644 --- a/packages/db/src/utils.ts +++ b/packages/db/src/utils.ts @@ -2,6 +2,8 @@ * Generic utility functions */ +import type { CompareOptions } from "./query/builder/types" + interface TypedArray { length: number [index: number]: number @@ -209,3 +211,9 @@ export function isTemporal(a: any): boolean { const tag = getStringTag(a) return typeof tag === `string` && temporalTypes.includes(tag) } + +export const DEFAULT_COMPARE_OPTIONS: CompareOptions = { + direction: `asc`, + nulls: `first`, + stringSort: `locale`, +} diff --git a/packages/db/src/utils/index-optimization.ts b/packages/db/src/utils/index-optimization.ts index 4648d88cc..be51dbcd6 100644 --- a/packages/db/src/utils/index-optimization.ts +++ b/packages/db/src/utils/index-optimization.ts @@ -15,6 +15,8 @@ * - Optimizes IN array expressions */ +import { DEFAULT_COMPARE_OPTIONS } from "../utils.js" +import type { CompareOptions } from "../query/builder/types.js" import type { BaseIndex, IndexOperation } from "../indexes/base-index.js" import type { BasicExpression } from "../query/ir.js" @@ -31,10 +33,14 @@ export interface OptimizationResult { */ export function findIndexForField( indexes: Map>, - fieldPath: Array + fieldPath: Array, + compareOptions: CompareOptions = DEFAULT_COMPARE_OPTIONS ): BaseIndex | undefined { for (const index of indexes.values()) { - if (index.matchesField(fieldPath)) { + if ( + index.matchesField(fieldPath) && + index.matchesCompareOptions(compareOptions) + ) { return index } } diff --git a/packages/db/tests/query/order-by.test.ts b/packages/db/tests/query/order-by.test.ts index b26603c6a..f5ea8263a 100644 --- a/packages/db/tests/query/order-by.test.ts +++ b/packages/db/tests/query/order-by.test.ts @@ -6,6 +6,7 @@ import { eq, gt, isUndefined, + lt, max, not, } from "../../src/query/builder/functions.js" @@ -1186,6 +1187,327 @@ function createOrderByTests(autoIndex: `off` | `eager`): void { const results = Array.from(collection.values()) expect(results).toHaveLength(0) }) + + it(`can use orderBy on different columns of the same collection`, async () => { + type DateItem = { + id: string + date: Date + value: number + } + + const dateCollection = createCollection( + mockSyncCollectionOptions({ + id: `test-dates`, + getKey: (item) => item.id, + initialData: [ + { + id: `1`, + date: new Date(`2025-09-15`), + value: 5, + }, + { + id: `2`, + date: new Date(`2025-09-10`), + value: 42, + }, + ], + autoIndex, + }) + ) + + // When autoIndex is `eager` this creates an index on the date field + const firstQuery = createLiveQueryCollection((q) => + q + .from({ numbers: dateCollection }) + .orderBy(({ numbers }) => numbers.date, `asc`) + .limit(1) + ) + await firstQuery.preload() + + // This then tries to use an index on the date field but in the opposite direction + const orderByQuery = createLiveQueryCollection((q) => + q + .from({ numbers: dateCollection }) + .orderBy(({ numbers }) => numbers.value, `asc`) + .limit(1) + ) + await orderByQuery.preload() + + const orderedDatesResult = Array.from(firstQuery.values()) + expect(orderedDatesResult).toHaveLength(1) + + expect(orderedDatesResult[0]!.id).toBe(`2`) + expect(orderedDatesResult[0]!.date).toEqual(new Date(`2025-09-10`)) + expect(orderedDatesResult[0]!.value).toBe(42) + + const orderedNumbersResult = Array.from(orderByQuery.values()) + expect(orderedNumbersResult).toHaveLength(1) + + expect(orderedNumbersResult[0]!.id).toBe(`1`) + expect(orderedNumbersResult[0]!.value).toBe(5) + expect(orderedNumbersResult[0]!.date).toEqual(new Date(`2025-09-15`)) + }) + + it(`can use orderBy in both ascending and descending order on the same column`, async () => { + type DateItem = { + id: string + date: Date + } + + const dateCollection = createCollection( + mockSyncCollectionOptions({ + id: `test-dates`, + getKey: (item) => item.id, + initialData: [ + { + id: `1`, + date: new Date(`2025-09-15`), + }, + { + id: `2`, + date: new Date(`2025-09-10`), + }, + ], + autoIndex, + }) + ) + + // When autoIndex is `eager` this creates an index on the date field + const firstQuery = createLiveQueryCollection((q) => + q + .from({ numbers: dateCollection }) + .orderBy(({ numbers }) => numbers.date, `asc`) + .limit(1) + .select(({ numbers }) => ({ + id: numbers.id, + date: numbers.date, + })) + ) + await firstQuery.preload() + + // This then tries to use an index on the date field but in the opposite direction + const orderByQuery = createLiveQueryCollection((q) => + q + .from({ numbers: dateCollection }) + .orderBy(({ numbers }) => numbers.date, `desc`) + .limit(1) + .select(({ numbers }) => ({ + id: numbers.id, + date: numbers.date, + })) + ) + await orderByQuery.preload() + + const results = Array.from(orderByQuery.values()) + expect(results).toHaveLength(1) + + expect(results[0]!.id).toBe(`1`) + expect(results[0]!.date).toEqual(new Date(`2025-09-15`)) + }) + + it(`optimizes where clause correctly after orderBy on same column`, async () => { + type PersonItem = { + id: string + age: number | null + } + + const personsCollection = createCollection( + mockSyncCollectionOptions({ + id: `test-dates`, + getKey: (item) => item.id, + initialData: [ + { + id: `1`, + age: 14, + }, + { + id: `2`, + age: 25, + }, + { + id: `3`, + age: null, + }, + ], + autoIndex, + }) + ) + + // When autoIndex is `eager` this creates an index on the date field + const query1 = createLiveQueryCollection((q) => + q + .from({ persons: personsCollection }) + .orderBy(({ persons }) => persons.age, { + direction: `asc`, + nulls: `last`, + }) + .limit(3) + ) + await query1.preload() + + const result1 = Array.from(query1.values()) + expect(result1).toHaveLength(3) + expect(result1.map((r) => r.age)).toEqual([14, 25, null]) + + // The default compare options defaults to nulls first + const query2 = createLiveQueryCollection((q) => + q + .from({ persons: personsCollection }) + .where(({ persons }) => lt(persons.age, 18)) + ) + await query2.preload() + + const result2 = Array.from(query2.values()) + const ages = result2.map((r) => r.age) + expect(ages).toHaveLength(2) + expect(ages).toContain(null) + expect(ages).toContain(14) + + // The default compare options defaults to nulls first + // So the null value is not part of the result + const query3 = createLiveQueryCollection((q) => + q + .from({ persons: personsCollection }) + .where(({ persons }) => gt(persons.age, 18)) + ) + await query3.preload() + + const result3 = Array.from(query3.values()) + const ages2 = result3.map((r) => r.age) + expect(ages2).toHaveLength(1) + expect(ages2).toContain(25) + }) + + it(`can use orderBy when two different comparators are used on the same column`, async () => { + type DateItem = { + id: string + value: string + } + + const dateCollection = createCollection( + mockSyncCollectionOptions({ + id: `test-dates`, + getKey: (item) => item.id, + initialData: [ + { + id: `1`, + value: `a`, + }, + { + id: `2`, + value: `b`, + }, + { + id: `3`, + value: `C`, + }, + ], + autoIndex, + }) + ) + + // When autoIndex is `eager` this creates an index on the date field + const query1 = createLiveQueryCollection((q) => + q + .from({ numbers: dateCollection }) + .orderBy(({ numbers }) => numbers.value, { + direction: `asc`, + stringSort: `lexical`, + }) + .limit(2) + ) + await query1.preload() + + const results1 = Array.from(query1.values()).map((r) => r.value) + expect(results1).toEqual([`C`, `a`]) + + // This then tries to use an index on the date field but in the opposite direction + const query2 = createLiveQueryCollection((q) => + q + .from({ numbers: dateCollection }) + .orderBy(({ numbers }) => numbers.value, { + direction: `asc`, + stringSort: `locale`, + locale: `en-US`, + }) + .limit(2) + ) + await query2.preload() + + const results2 = Array.from(query2.values()).map((r) => r.value) + expect(results2).toEqual([`a`, `b`]) + }) + + it(`can use orderBy when nulls first vs nulls last are used on the same column`, async () => { + type NullableItem = { + id: string + value: number | null + } + + const nullableCollection = createCollection( + mockSyncCollectionOptions({ + id: `test-nullable`, + getKey: (item) => item.id, + initialData: [ + { + id: `1`, + value: 10, + }, + { + id: `2`, + value: null, + }, + { + id: `3`, + value: 5, + }, + { + id: `4`, + value: null, + }, + ], + autoIndex, + }) + ) + + // When autoIndex is `eager` this creates an index on the value field with nulls first + const query1 = createLiveQueryCollection((q) => + q + .from({ items: nullableCollection }) + .orderBy(({ items }) => items.value, { + direction: `asc`, + nulls: `first`, + }) + .limit(3) + .select(({ items }) => ({ + id: items.id, + value: items.value, + })) + ) + await query1.preload() + + const results1 = Array.from(query1.values()) + expect(results1.map((r) => r.value)).toEqual([null, null, 5]) + + // This then tries to use an index on the value field but with nulls last + const query2 = createLiveQueryCollection((q) => + q + .from({ items: nullableCollection }) + .orderBy(({ items }) => items.value, { + direction: `asc`, + nulls: `last`, + }) + .limit(3) + .select(({ items }) => ({ + id: items.id, + value: items.value, + })) + ) + await query2.preload() + + const results2 = Array.from(query2.values()) + expect(results2.map((r) => r.value)).toEqual([5, 10, null]) + }) }) describe(`Nullable Column OrderBy`, () => {