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

feat(server, web): smart search filtering and pagination #6525

Merged
merged 38 commits into from Feb 13, 2024

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Jan 20, 2024

Description

This PR uses the new pgvecto.rs VBASE feature to implement paginated smart search. It updates the web app to take advantage of this for infinite scroll. Additionally, it refactors the custom search builder to be more reusable, integrating it into smart search. This paves the way for filtered hybrid search in the future.

Summary of changes:

  • Rename smart info repository to search repository
  • Move asset search method to search repository
  • Make smart search repository method paginated and share the same builder (with some exclusions)
  • Update web to query for the next page after reaching the bottom of the page (specifically when the second-to-last row is in view)
  • Modularize typing for search builder
  • Add new smart search DTO that includes most of the filters for the search builder
  • Split current /search endpoint into /search/metadata and /search/smart
    • There's enough difference in what the two endpoints can accept that separating them makes validation etc. simpler
  • Mark current search endpoint as deprecated
    • This is because the new search filtering logic isn't ready yet, and because making all of the necessary changes to actually switch everything to the new endpoints increases the scope of what is already a very large PR

How Has This Been Tested?

Tested with both pgvecto.rs and pgvector. The latter can still somewhat benefit from this change, but with a maximum of 1000 assets that will effectively be lower in the case of filters or multiple users.

infinite_scroll_480_av1.mp4

@mertalev mertalev force-pushed the feat/server-web-search-pagination branch from cb0d60c to b2828a9 Compare February 10, 2024 02:32
Copy link

cloudflare-pages bot commented Feb 10, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: bf879db
Status: ✅  Deploy successful!
Preview URL: https://4f23e17d.immich.pages.dev
Branch Preview URL: https://feat-server-web-search-pagin.immich.pages.dev

View logs

@mertalev mertalev marked this pull request as ready for review February 11, 2024 01:35
@mertalev mertalev changed the title feat(server, web): smart search pagination feat(server, web): smart search filtering and pagination Feb 11, 2024
Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

Really good!

server/src/domain/repositories/search.repository.ts Outdated Show resolved Hide resolved
server/src/domain/search/dto/search.dto.ts Show resolved Hide resolved
server/src/domain/search/search.service.ts Show resolved Hide resolved
server/src/domain/search/search.service.ts Show resolved Hide resolved
server/src/infra/infra.utils.ts Show resolved Hide resolved
web/src/routes/(user)/search/+page.svelte Outdated Show resolved Hide resolved
@alextran1502
Copy link
Contributor

Thank you for the high quality PR.

Can you explain briefly how you achieve the infinite scrolling, especially on the performance aspect? Will it cause a performance issue if you keep scrolling since the DOM will continue to add more div as it is scrolled?

@mertalev
Copy link
Contributor Author

Thank you for the high quality PR.

Can you explain briefly how you achieve the infinite scrolling, especially on the performance aspect? Will it cause a performance issue if you keep scrolling since the DOM will continue to add more div as it is scrolled?

Existing assets aren't re-rendered when a new page of assets are added, so I don't think this slows things down much besides likely increasing RAM usage. We also only append to the list of assets - this avoids overhead of copying or traversing this list as it gets longer.

There's definitely a point where it will get slower, not just on the frontend but also because the vector search query will have to skip past more and more rows for each page. But in an earlier version, I scrolled to about 50-60 pages deep (i.e. 5-6k assets) and didn't notice a degradation, so I'm not sure what the threshold is where it would start getting slower. It might be different for a client with less RAM, on a different browser (I tested on Chrome), etc.

@mertalev
Copy link
Contributor Author

mertalev commented Feb 13, 2024

I can confirm it starts degrading for me at 7k+ assets. The scrolling starts getting jittery, the next page takes longer to load and the thumbnails don't show up as quickly. Continuously scrolling without ever stopping, RAM usage climbed to 1.5gb at 10k assets. But after leaving it alone to type this, it dropped down to around 650mb. Lastly, changing the width of the page changes the size of the thumbnails, meaning they have to get re-rendered. This absolutely chokes the page and results in over 3gb RAM usage with 10k assets.

@mertalev
Copy link
Contributor Author

I think we can set a 50 page limit to prevent the number of assets from getting out of hand.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Code looks fine AFAICT. We just can't seem to be able to decide if it'll live in search/ or asset/ lol

web/src/routes/(user)/search/+page.svelte Outdated Show resolved Hide resolved
@mertalev mertalev merged commit e334443 into main Feb 13, 2024
24 checks passed
@mertalev mertalev deleted the feat/server-web-search-pagination branch February 13, 2024 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants