-
Notifications
You must be signed in to change notification settings - Fork 48
query optimiser that pushes where clauses down to subqueries closer to the source #256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 71eb5ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 @tanstack/db
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/vue-db
commit: |
Size Change: +2.98 kB (+8.58%) 🔍 Total Size: 37.7 kB
ℹ️ View Unchanged
|
Size Change: 0 B Total Size: 1.05 kB ℹ️ View Unchanged
|
3d339cf
to
cbe3ad8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, i left a few comments.
for (const clause of whereClauses) { | ||
if (clause.type === `func` && clause.name === `and`) { | ||
// Recursively split nested AND clauses to handle complex expressions | ||
const splitArgs = splitAndClauses( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring on this function says:
Step 1: Split all AND clauses at the root level into separate WHERE clauses.
But since we call splitAndClauses
recursively here this is not only at the root level but also nested. So it turns any nested conjunctions (i.e. AND clauses) into a flatened array of all conjuncts.
|
||
// Create mapping from optimized query to original for caching | ||
queryMapping.set(query, rawQuery) | ||
mapNestedQueries(query, rawQuery, queryMapping) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like that we're passing around mutable state and relying on mapNestedQueries
to mutate that state. It feels like this should be a class since it keeps some state and provides methods over that state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it's getting quite complex. I would like to refactor the compiler into a class, but let's do it in a follow-up PR or this will blow up.
This leads on to pushing those where clauses down to the collection subscription which can use the index... see #257
There is a detailed comment explaining the optimiser at the top of
packages/db/src/query/optimizer.ts
, but in essence it rewrites the query IR moving things around.