Skip to content
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

SQLite implementation of query support for DB based index #1121

Merged

Conversation

habdelra
Copy link
Contributor

@habdelra habdelra commented Mar 26, 2024

This PR provides a SQLite implementation of queries for our DB based index. A handy test index setup helper is also included in this PR. This PR only focuses on the type filter and eq filter. And specifically no containsMany field support yet.

Copy link

github-actions bot commented Mar 26, 2024

Test Results

558 tests  +8   554 ✔️ +8   7m 56s ⏱️ -10s
    1 suites ±0       4 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit 5feaf04. ± Comparison against base commit 2f163c1.

This pull request removes 10 and adds 18 tests. Note that renamed tests count towards both.
Chrome 123.0 ‑ Unit | index-db: can create a new generation of index entries
Chrome 123.0 ‑ Unit | index-db: can get "production" index entry
Chrome 123.0 ‑ Unit | index-db: can get work in progress index entry
Chrome 123.0 ‑ Unit | index-db: can perform invalidations for an index entry
Chrome 123.0 ‑ Unit | index-db: can prevent concurrent batch invalidations from colliding
Chrome 123.0 ‑ Unit | index-db: can prevent concurrent batch invalidations from colliding when making new generation
Chrome 123.0 ‑ Unit | index-db: can remove an index entry
Chrome 123.0 ‑ Unit | index-db: can update an index entry
Chrome 123.0 ‑ Unit | index-db: does not create invalidation record for non-JSON invalidation
Chrome 123.0 ‑ Unit | index-db: returns undefined when getting a deleted entry
Chrome 123.0 ‑ Unit | indexer: can create a new generation of index entries
Chrome 123.0 ‑ Unit | indexer: can get "production" index entry
Chrome 123.0 ‑ Unit | indexer: can get work in progress index entry
Chrome 123.0 ‑ Unit | indexer: can perform invalidations for an index entry
Chrome 123.0 ‑ Unit | indexer: can prevent concurrent batch invalidations from colliding
Chrome 123.0 ‑ Unit | indexer: can prevent concurrent batch invalidations from colliding when making new generation
Chrome 123.0 ‑ Unit | indexer: can remove an index entry
Chrome 123.0 ‑ Unit | indexer: can update an index entry
Chrome 123.0 ‑ Unit | indexer: does not create invalidation record for non-JSON invalidation
Chrome 123.0 ‑ Unit | indexer: returns undefined when getting a deleted entry
…

♻️ This comment has been updated with latest results.

@habdelra habdelra marked this pull request as ready for review March 28, 2024 16:07
Copy link
Contributor

@lukemelia lukemelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

Copy link
Contributor

@jurgenwerk jurgenwerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate all the comments, looks well thought out.

@@ -41,9 +63,31 @@ interface GetEntryOptions {
useWorkInProgressIndex?: boolean;
}

interface QueryResultsMeta {
// TODO SQLite doesn't let us use cursors in the classic sense so we need to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by cursors in the classic sense? I am familiar with implementing cursor based pagination where the last value of the column from the previous query is selected as the cursor for the next set. This sounds like it would be achievable with SQLite also. Just wondering what kind of pagination you had in mind here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQLite does have a cursor available, but it’s only available from the synchronous API.

Normally you get it like this:

import sqlite3;
con = sqlite3.connect("tutorial.db");
con.cursor()cur = con.cursor();
result = cur.execute("SELECT name FROM sqlite_master");
result.fetchone()

however from the async interface you no longer have a direct handle on the connection. Rather the connection is only available in the worker thread since it’s not possible for the connection to be serialized over the postMessage boundary.

The worker thread only has a couple API calls:
close(), config-get(), execute(), open(). https://sqlite.org/wasm/doc/trunk/api-worker1.md
the idea being only POJO’s are available to cross the postMessage boundary between a worker thread and the main thread

Copy link
Contributor Author

@habdelra habdelra Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead we’ll have to manually control the result set as I described in this comment. We’ll need to use the realm version so that we can prevent the index mutations made between pages from effecting the results (so the realm version will be part of the query), as well as only fetching one page at a time using the ROW_NUMBER() and/or LIMIT sql commands by having the caller keep track of what page number of results they are requesting and including that in our Query interface.

I made a pagination linear ticket for this work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I see now what you mean by the cursor and the boundary.

@habdelra habdelra merged commit 9533728 into main Mar 29, 2024
22 checks passed
@delete-merged-branch delete-merged-branch bot deleted the cs-6457-implement-search-query-support-in-sqlite-adapter branch March 29, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants