From 1293a168d98fa9721a527513dd9ed6c90216ceb5 Mon Sep 17 00:00:00 2001 From: Kevin De Porre Date: Wed, 27 Aug 2025 11:31:31 +0200 Subject: [PATCH 1/7] Added unit tests to reproduce bug --- packages/db/tests/query/order-by.test.ts | 44 ++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/packages/db/tests/query/order-by.test.ts b/packages/db/tests/query/order-by.test.ts index d48efd3a5..28ff389ae 100644 --- a/packages/db/tests/query/order-by.test.ts +++ b/packages/db/tests/query/order-by.test.ts @@ -680,6 +680,50 @@ function createOrderByTests(autoIndex: `off` | `eager`): void { expect(results).toHaveLength(3) // Alice (50000) and Eve (52000) filtered out expect(results.map((r) => r.salary)).toEqual([55000, 60000, 65000]) }) + + it(`orders correctly with a limit`, async () => { + const collection = createLiveQueryCollection((q) => + q + .from({ employees: employeesCollection }) + .where(({ employees }) => gt(employees.salary, 50000)) + .orderBy(({ employees }) => employees.salary, `asc`) + .limit(2) + .select(({ employees }) => ({ + id: employees.id, + name: employees.name, + salary: employees.salary, + })) + ) + await collection.preload() + + const results = Array.from(collection.values()) + + expect(results).toHaveLength(2) + expect(results.map((r) => r.salary)).toEqual([52000, 55000]) + expect(results.map((r) => r.id)).toEqual([5, 3]) + }) + + it(`returns a single row with limit 1`, async () => { + const collection = createLiveQueryCollection((q) => + q + .from({ employees: employeesCollection }) + .where(({ employees }) => gt(employees.salary, 50000)) + .orderBy(({ employees }) => employees.salary, `asc`) + .limit(1) + .select(({ employees }) => ({ + id: employees.id, + name: employees.name, + salary: employees.salary, + })) + ) + await collection.preload() + + const results = Array.from(collection.values()) + + expect(results).toHaveLength(1) + expect(results[0]!.id).toEqual(5) + expect(results[0]!.salary).toEqual(52000) + }) }) describe(`Fractional Index Behavior`, () => { From 0b183722059de1c58810739927e34cfd8f424ea7 Mon Sep 17 00:00:00 2001 From: Kevin De Porre Date: Wed, 27 Aug 2025 15:05:17 +0200 Subject: [PATCH 2/7] Also load more data if needed on initial load --- .../query/live/collection-config-builder.ts | 44 ++++++++++------ .../src/query/live/collection-subscriber.ts | 50 ++++++++++++------- 2 files changed, 61 insertions(+), 33 deletions(-) diff --git a/packages/db/src/query/live/collection-config-builder.ts b/packages/db/src/query/live/collection-config-builder.ts index 06592773f..d2fde0dbe 100644 --- a/packages/db/src/query/live/collection-config-builder.ts +++ b/packages/db/src/query/live/collection-config-builder.ts @@ -132,7 +132,6 @@ export class CollectionConfigBuilder< // Mark the collection as ready after the first successful run if (ready && this.allCollectionsReady()) { markReady() - // Remember that we marked the collection as ready this.collectionReady = true } } @@ -158,10 +157,13 @@ export class CollectionConfigBuilder< syncState ) - this.subscribeToAllCollections(config, fullSyncState) + const loadMoreDataCallbacks = this.subscribeToAllCollections( + config, + fullSyncState + ) - // Initial run - this.maybeRunGraph(config, fullSyncState) + // Initial run with callback to load more data if needed + this.maybeRunGraph(config, fullSyncState, loadMoreDataCallbacks) // Return the unsubscribe function return () => { @@ -315,19 +317,33 @@ export class CollectionConfigBuilder< config: Parameters[`sync`]>[0], syncState: FullSyncState ) { - Object.entries(this.collections).forEach(([collectionId, collection]) => { - const collectionSubscriber = new CollectionSubscriber( - collectionId, - collection, - config, - syncState, - this - ) - collectionSubscriber.subscribe() - }) + const loaders = Object.entries(this.collections).map( + ([collectionId, collection]) => { + const collectionSubscriber = new CollectionSubscriber( + collectionId, + collection, + config, + syncState, + this + ) + collectionSubscriber.subscribe() + + const loadMore = + collectionSubscriber.loadMoreIfNeeded.bind(collectionSubscriber) + + return loadMore + } + ) + + const loadMoreDataCallback = () => { + loaders.map((loader) => loader()) // .every((doneLoading) => doneLoading) + return true + } // Mark the collections as subscribed in the sync state syncState.subscribedToAllCollections = true + + return loadMoreDataCallback } } diff --git a/packages/db/src/query/live/collection-subscriber.ts b/packages/db/src/query/live/collection-subscriber.ts index 50e454ce5..8079dfb8a 100644 --- a/packages/db/src/query/live/collection-subscriber.ts +++ b/packages/db/src/query/live/collection-subscriber.ts @@ -85,20 +85,20 @@ export class CollectionSubscriber< changes, this.collection.config.getKey ) - if (sentChanges > 0 || !this.collectionConfigBuilder.isCollectionReady()) { - // Only run the graph if we sent any changes - // otherwise we may get into an infinite loop - // trying to load more data for the orderBy query - // when there's no more data in the collection - // EXCEPTION: if the collection is not yet ready - // we need to run it even if there are no changes - // in order for the collection to be marked as ready - this.collectionConfigBuilder.maybeRunGraph( - this.config, - this.syncState, - callback - ) - } + + // Do not provide the callback that loads more data + // if there's no more data to load + // otherwise we end up in an infinite loop trying to load more data + const dataLoader = sentChanges > 0 ? callback : undefined + + // We need to call `maybeRunGraph` even if there's no data to load + // because we need to mark the collection as ready if it's not already + // and that's only done in `maybeRunGraph` + this.collectionConfigBuilder.maybeRunGraph( + this.config, + this.syncState, + dataLoader + ) } // Wraps the sendChangesToPipeline function @@ -268,11 +268,19 @@ export class CollectionSubscriber< // This function is called by maybeRunGraph // after each iteration of the query pipeline // to ensure that the orderBy operator has enough data to work with - private loadMoreIfNeeded() { - const { dataNeeded } = + loadMoreIfNeeded() { + const orderByInfo = this.collectionConfigBuilder.optimizableOrderByCollections[ this.collectionId - ]! + ] + + if (!orderByInfo) { + // This query has no orderBy operator + // so there's no data to load, just return true + return true + } + + const { dataNeeded } = orderByInfo if (!dataNeeded) { // This should never happen because the topK operator should always set the size callback @@ -285,12 +293,15 @@ export class CollectionSubscriber< // `dataNeeded` probes the orderBy operator to see if it needs more data // if it needs more data, it returns the number of items it needs const n = dataNeeded() + let noMoreNextItems = false if (n > 0) { - this.loadNextItems(n) + const loadedItems = this.loadNextItems(n) + noMoreNextItems = loadedItems === 0 } // Indicate that we're done loading data if we didn't need to load more data - return n === 0 + // or there's no more data to load + return n === 0 || noMoreNextItems } private sendChangesToPipelineWithTracking( @@ -322,6 +333,7 @@ export class CollectionSubscriber< return { type: `insert`, key, value: this.collection.get(key) } }) this.sendChangesToPipelineWithTracking(nextInserts) + return nextInserts.length } private getWhereClauseFromAlias( From 9e452cec18b1d038606a3b682486698afcd9e879 Mon Sep 17 00:00:00 2001 From: Kevin De Porre Date: Wed, 27 Aug 2025 15:17:18 +0200 Subject: [PATCH 3/7] Remove unused method --- packages/db/src/query/live/collection-config-builder.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/db/src/query/live/collection-config-builder.ts b/packages/db/src/query/live/collection-config-builder.ts index d2fde0dbe..ab78b2332 100644 --- a/packages/db/src/query/live/collection-config-builder.ts +++ b/packages/db/src/query/live/collection-config-builder.ts @@ -80,10 +80,6 @@ export class CollectionConfigBuilder< this.compileBasePipeline() } - isCollectionReady() { - return this.collectionReady - } - getConfig(): CollectionConfig { return { id: this.id, @@ -132,6 +128,7 @@ export class CollectionConfigBuilder< // Mark the collection as ready after the first successful run if (ready && this.allCollectionsReady()) { markReady() + // Remember that we marked the collection as ready this.collectionReady = true } } From f3e35889e385c2b81850abdd552b748ac2d3bf91 Mon Sep 17 00:00:00 2001 From: Kevin De Porre Date: Wed, 27 Aug 2025 15:19:06 +0200 Subject: [PATCH 4/7] Changeset --- .changeset/evil-bats-remain.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/evil-bats-remain.md diff --git a/.changeset/evil-bats-remain.md b/.changeset/evil-bats-remain.md new file mode 100644 index 000000000..e8a9afb0f --- /dev/null +++ b/.changeset/evil-bats-remain.md @@ -0,0 +1,5 @@ +--- +"@tanstack/db": patch +--- + +Fix bug that caused initial query results to have too few rows when query has orderBy, limit, and where clauses. From a5c25f6e8f7dcb9b7d59bb2168aafc368a828b44 Mon Sep 17 00:00:00 2001 From: Kevin De Porre Date: Wed, 27 Aug 2025 15:19:48 +0200 Subject: [PATCH 5/7] Removed unused class field --- packages/db/src/query/live/collection-config-builder.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/db/src/query/live/collection-config-builder.ts b/packages/db/src/query/live/collection-config-builder.ts index ab78b2332..515db3007 100644 --- a/packages/db/src/query/live/collection-config-builder.ts +++ b/packages/db/src/query/live/collection-config-builder.ts @@ -59,8 +59,6 @@ export class CollectionConfigBuilder< OrderByOptimizationInfo > = {} - private collectionReady = false - constructor( private readonly config: LiveQueryCollectionConfig ) { @@ -128,8 +126,6 @@ export class CollectionConfigBuilder< // Mark the collection as ready after the first successful run if (ready && this.allCollectionsReady()) { markReady() - // Remember that we marked the collection as ready - this.collectionReady = true } } } From d3b1d66169c11291c4344514e3fc2d3b0b40d41f Mon Sep 17 00:00:00 2001 From: Kevin De Porre Date: Wed, 27 Aug 2025 15:55:50 +0200 Subject: [PATCH 6/7] Unit test to check that the topK is properly populated if it isn't filled and a live update inserts a row that is bigger than max sent value --- packages/db/tests/query/order-by.test.ts | 51 ++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/packages/db/tests/query/order-by.test.ts b/packages/db/tests/query/order-by.test.ts index 28ff389ae..07ad6a0c8 100644 --- a/packages/db/tests/query/order-by.test.ts +++ b/packages/db/tests/query/order-by.test.ts @@ -493,6 +493,57 @@ function createOrderByTests(autoIndex: `off` | `eager`): void { ]) }) + it(`applies incremental insert of a new row inside the topK but after max sent value correctly`, async () => { + const collection = createLiveQueryCollection((q) => + q + .from({ employees: employeesCollection }) + .orderBy(({ employees }) => employees.salary, `asc`) + .offset(1) + .limit(10) + .select(({ employees }) => ({ + id: employees.id, + name: employees.name, + salary: employees.salary, + })) + ) + await collection.preload() + + const results = Array.from(collection.values()) + + expect(results.map((r) => r.salary)).toEqual([ + 52_000, 55_000, 60_000, 65_000, + ]) + + // Now insert a new employee with highest salary + // this should now become part of the topK because + // the topK isn't full yet, so even though it's after the max sent value + // it should still be part of the topK + const newEmployee = { + id: 6, + name: `George`, + department_id: 1, + salary: 72_000, + hire_date: `2023-01-01`, + } + + employeesCollection.utils.begin() + employeesCollection.utils.write({ + type: `insert`, + value: newEmployee, + }) + employeesCollection.utils.commit() + + const newResults = Array.from(collection.values()) + + expect(newResults.map((r) => [r.id, r.salary])).toEqual([ + [5, 52_000], + [3, 55_000], + [2, 60_000], + [4, 65_000], + [6, 72_000], + ]) + }) + it(`applies incremental insert of a new row after the topK correctly`, async () => { const collection = createLiveQueryCollection((q) => q From a4b84019c549cbeb2fec08b7aefef5eb0bb4da08 Mon Sep 17 00:00:00 2001 From: Kevin De Porre Date: Wed, 27 Aug 2025 15:56:12 +0200 Subject: [PATCH 7/7] Only filter out changes > max sent value if the top K is full --- .../src/query/live/collection-subscriber.ts | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/db/src/query/live/collection-subscriber.ts b/packages/db/src/query/live/collection-subscriber.ts index 8079dfb8a..38e5ceee0 100644 --- a/packages/db/src/query/live/collection-subscriber.ts +++ b/packages/db/src/query/live/collection-subscriber.ts @@ -229,7 +229,7 @@ export class CollectionSubscriber< private subscribeToOrderedChanges( whereExpression: BasicExpression | undefined ) { - const { offset, limit, comparator } = + const { offset, limit, comparator, dataNeeded } = this.collectionConfigBuilder.optimizableOrderByCollections[ this.collectionId ]! @@ -245,11 +245,18 @@ export class CollectionSubscriber< // and filter out changes that are bigger than the biggest value we've sent so far // because they can't affect the topK const splittedChanges = splitUpdates(changes) - const filteredChanges = filterChangesSmallerOrEqualToMax( - splittedChanges, - comparator, - this.biggest - ) + let filteredChanges = splittedChanges + if (dataNeeded!() === 0) { + // If the topK is full [..., maxSentValue] then we do not need to send changes > maxSentValue + // because they can never make it into the topK. + // However, if the topK isn't full yet, we need to also send changes > maxSentValue + // because they will make it into the topK + filteredChanges = filterChangesSmallerOrEqualToMax( + splittedChanges, + comparator, + this.biggest + ) + } this.sendChangesToPipeline( filteredChanges, this.loadMoreIfNeeded.bind(this)