enable live queries to use indexes on collections #258
Conversation
🦋 Changeset detectedLatest commit: 9ee908c The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@tanstack/db-example-react-todo commit: |
|
Size Change: +8.47 kB (+26.92%) 🚨 Total Size: 39.9 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 1.05 kB ℹ️ View Unchanged
|
|
We need to add tests that validate what happens when a row that was filtered out/in based on an index value during inital run, then changes to be filtered in/out. I have a feeling there may be a bug where we need to do a little book keeping on the subscribe to know when rows were sent, and then know if it's an update/insert or deleted/update on the pipeline. |
| @@ -2459,7 +2478,7 @@ export class CollectionImpl< | |||
| const operation = expression.name as `eq` | `gt` | `gte` | `lt` | `lte` | |||
|
|
|||
| // Find an index that matches this field | |||
| for (const index of this.indexes.values()) { | |||
| for (const [, index] of this.indexes) { | |||
There was a problem hiding this comment.
i much preferred how it was before
| } | ||
| } | ||
|
|
||
| return checkExpression(whereClause) |
There was a problem hiding this comment.
Why would we define an inner function for this?
I'm not sure i'm a find of the switch statement, this is equivalent:
const tpe = expr.type
if (tpe === `func`) {
// Check if this function is supported
if (!SUPPORTED_COLLECTION_FUNCS.has(expr.name)) {
return false
}
// Recursively check all arguments
return expr.args.every((arg) =>
checkExpression(arg as BasicExpression<boolean>)
)
}
return [`val`, `ref`].includes(tpe)| * @param collectionAlias - The alias of the collection being filtered | ||
| * @returns The converted BasicExpression or null if conversion fails | ||
| */ | ||
| export function convertToBasicExpression( |
There was a problem hiding this comment.
Same comments on this function. No need for an inner function + i'd prefer an if statement instead of a switch.
| query, | ||
| inputsCache as Record<string, KeyedStream> | ||
| ) | ||
|
|
||
| pipelineCache = compilationResult.pipeline |
There was a problem hiding this comment.
Why not destructuring it? :-)
| const aIsObject = typeof a === `object` && a !== null | ||
| const bIsObject = typeof b === `object` && b !== null | ||
|
|
||
| if (aIsObject || bIsObject) { |
There was a problem hiding this comment.
Checking for null is actually not needed because at the very beginning of this function we already check for null and if a or b is null we immediately return.
| return 0 | ||
| } catch { | ||
| // If comparison fails, fall back to string comparison | ||
| return safeStringify(a).localeCompare(safeStringify(b)) |
There was a problem hiding this comment.
I don't think comparison will ever throw on primitive values?
samwillis
force-pushed
the
index-collections
branch
from
95b29d8 to
e526c1e
Compare
stacked on #257 and #256
This connects the optimised queries with the collections indexes, pushing where clauses that only touch a single collection to the subscription. The subscription then handles using the indexes if they are available.
Note that the design of this means that the query optimiser and compiler does not need to have knowledge of indexes, any where clause that only touches a single collection is pushed to the subscription rather than handled as a
filterint he D2mini pipeline.