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

Flatten index to remove expensive joins during invalidation #2003

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

habdelra
Copy link
Contributor

@habdelra habdelra commented Jan 6, 2025

This PR flattens our index so that remove the expensive join that was required to determine the index records that pertain to the current realm version. Instead of using a single table to control both the "production" index and the "working" index (the index that is being built), we use 2 tables. One table, boxel_index_working, is just for building the index for each mutation, and the other table, boxel_index, is for querying the final version of the index.

TODO:

  • there is still another potentially expensive join used for invalidation. Specifically we are using a less than optimal query in order to preserve SQLite syntax compatibility in our invalidation query. Also consider breaking this compatibility and using a more optimal postgres query for this specific scenario.
  • merge in boxel_index.icon_html column from Show card type icon of CardsGird's filter list #1971

Comment on lines -487 to -495
test('can get paginated results that are stable during index mutations', async function (assert) {
await runSharedTest(indexQueryEngineTests, assert, {
indexQueryEngine,
dbAdapter,
loader,
testCards,
});
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when ed ad I reviewed this he felt that this was actually a weird feature in that this means that as you paginate the search results you are not necessarily seeing the latest query results since we are trying to maintain the results from when you originally made the request as you paginate thru the results as opposed to showing the latest values.

@@ -48,14 +48,6 @@ module('Unit | index-writer', function (hooks) {
});
});

test('only invalidates latest version of content', async function (assert) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this becomes moot since we are now breaking out the working version and the production version into separate tables

Comment on lines 467 to 473
// TODO: If this query is still slow we'll need to consider dropping
// this join and writing a pg specific query here--as this query is
// currently written in a less performant manner in order to be
// compatible with SQLite. make sure to measure performance in hosted
// env first before sending for team review. Should strive to be less
// than 50ms (currently about 600-700ms) when this is run against a
// reasonably sized index.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should really just do this

@habdelra habdelra marked this pull request as draft January 6, 2025 20:36
Copy link

github-actions bot commented Jan 6, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   21m 7s ⏱️ -39s
724 tests  - 2  722 ✔️  - 2  2 💤 ±0  0 ±0 
729 runs   - 2  727 ✔️  - 2  2 💤 ±0  0 ±0 

Results for commit 87ef92c. ± Comparison against base commit 1353a82.

This pull request removes 2 tests.
Chrome 131.0 ‑ Unit | index-writer: only invalidates latest version of content
Chrome 131.0 ‑ Unit | query: can get paginated results that are stable during index mutations

♻️ This comment has been updated with latest results.

@habdelra habdelra marked this pull request as ready for review January 7, 2025 18:07
@habdelra habdelra requested a review from a team January 7, 2025 18: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.

LGTM 👍

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.

2 participants