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: optimized board queries #6931

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

Conversation

psychedelicious
Copy link
Collaborator

Summary

In #6603 I made some changes to reduce SQL queries and network requests when listing boards. That PR got crusty, this PR updates it to work on main.

Two main changes.

feat(app): add method & route to get uncategorized image counts

Previously we were abusing the list boards query (twice!) to get the counts for uncategorized images/assets. There is now a dedicated endpoint for this.

feat(app): optimize boards queries

Use SQL instead of python to retrieve image count, asset count and board cover image.

This reduces the number of SQL queries needed to list all boards. Previously, we did 1 + 2 * board_count queries::

  • 1 query to get the list of board records
  • 1 query per board to get its total count
  • 1 query per board to get its cover image

Then, on the frontend, we made two additional network requests to get each board's counts:

  • 1 request (== 1 SQL query) for image count
  • 1 request (== 1 SQL query) for asset count

All of this information is now retrieved in a single SQL query, and provided via single network request.

As part of this change, BoardRecord now includes image_count, asset_count and cover_image_name. This makes BoardDTO redundant, but removing it is a deeper change...

Related Issues / Discussions

QA Instructions

Needs re-review from @maryhipp.

Board counts should be correct and update as images are moved between boards and generated/saved to gallery.

Merge Plan

This should only be merged after v5 is released.

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)

@github-actions github-actions bot added api python PRs that change python files services PRs that change app services frontend PRs that change frontend files python-tests PRs that change python tests labels Sep 24, 2024
Use SQL instead of python to retrieve image count, asset count and board cover image.

This reduces the number of SQL queries needed to list all boards. Previously, we did `1 + 2 * board_count` queries::
- 1 query to get the list of board records
- 1 query per board to get its total count
- 1 query per board to get its cover image

Then, on the frontend, we made two additional network requests to get each board's counts:
- 1 request (== 1 SQL query) for image count
- 1 request (== 1 SQL query) for asset count

All of this information is now retrieved in a single SQL query, and provided via single network request.

As part of this change, `BoardRecord` now includes `image_count`, `asset_count` and `cover_image_name`. This makes `BoardDTO` redundant, but removing it is a deeper change...
- Update tooltips to use counts in the DTO
- Remove unused `getBoardImagesTotal` and `getBoardAssetsTotal` queries, which were just abusing the list endpoint to get totals...
- Remove extraneous optimistic update in invocation complete listener
@psychedelicious psychedelicious force-pushed the psyche/optimized-board-count-queries branch from 5ddcf82 to a091942 Compare September 25, 2024 01:19
Now that the counts are already available and the tooltip does not make a network request, we can remove the delay (which was added to prevent network thrashing as you moved the mouse over the boards list).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api frontend PRs that change frontend files python PRs that change python files python-tests PRs that change python tests services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant