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

query method for store API #311

Merged
merged 108 commits into from
Apr 24, 2023
Merged

query method for store API #311

merged 108 commits into from
Apr 24, 2023

Conversation

adzialocha
Copy link
Member

@adzialocha adzialocha commented Mar 23, 2023

This PR implements the "backend" for our new "Query" API to paginate, filter and sort collections based off the work in #302 and #308. Specifically this involves designing the SQL query which gets the data from the database, to then return it back to the GraphQL API consumer.

The SQL query is fairly complex as it a) is based on our "operation-graph-like" table structure and b) covers a lot of features. The goal is to only make one SQL query per API request. No index optimizations took place yet, this requires some heavy-load testing and benchmarking (just setting arbitrary indexes can worsen performance).

Todo

  • Revisit GraphQL API for pagination info and collection response (Fix paginated collection API #323)
  • Introduce endCursor in pagination info response
  • Encode pagination cursor as base64 string or similar to make it opaque
  • Handle (pinned) relation lists and relations in nested GraphQL queries. Revisit API here (especially pagination for collection queries)
    • Make cursor contain the id of the parent document, so we can identify which list inside that collection we're paginating
    • Experiment with JOINs to properly paginate over relation list values
  • Fix assembling document list after SQL query for relation lists, since documents can occur multiple times in a row there. A solution would be to group the rows by number of selected fields
  • Make query compatible with PostgreSQL
  • Separate the cursor type DocumentCursor from the scalar type CursorScalar in the GraphQL API, sticking to the separation of concerns we already have introduced in all other places
  • Set filter default to not query deleted documents, improve upsert method of Filter struct to overwrite previously set boolean filters to allow users to override that default

Next Steps

  • Add indices to SQL tables connected to heavy-load testing and benchmarking
  • Allow ordering over meta fields like document view id, owner, edited and deleted status
  • edited in GraphQL meta API
  • deleted in GraphQL meta API
  • Implement backwards pagination as well
  • Finish tests from previous PRs
  • Check if all arguments are correctly validated (it crashes when entering an unknown field in orderBy)
  • Define good pagination defaults and limits (for example max page sizes)

Closes: #310

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@adzialocha adzialocha changed the base branch from main to graphql-query-api March 23, 2023 16:42
@adzialocha adzialocha changed the base branch from graphql-query-api to query-api March 23, 2023 16:43
@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Patch coverage: 89.76% and project coverage change: -0.75 ⚠️

Comparison is base (66926fd) 89.29% compared to head (504df42) 88.55%.

❗ Current head 504df42 differs from pull request most recent head f871761. Consider uploading reports for the commit f871761 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
- Coverage   89.29%   88.55%   -0.75%     
==========================================
  Files          68       70       +2     
  Lines        5027     5661     +634     
==========================================
+ Hits         4489     5013     +524     
- Misses        538      648     +110     
Impacted Files Coverage Δ
aquadoggo/src/db/types/document.rs 42.85% <ø> (ø)
aquadoggo/src/graphql/queries/document.rs 100.00% <ø> (ø)
aquadoggo/src/graphql/tests.rs 100.00% <ø> (ø)
aquadoggo/src/graphql/types/ordering.rs 100.00% <ø> (ø)
aquadoggo/src/materializer/tasks/reduce.rs 92.70% <ø> (ø)
aquadoggo/src/graphql/types/document_fields.rs 47.36% <16.66%> (-48.90%) ⬇️
aquadoggo/src/graphql/scalars/cursor_scalar.rs 57.89% <37.50%> (+0.75%) ⬆️
aquadoggo/src/graphql/types/document_meta.rs 90.00% <66.66%> (-10.00%) ⬇️
aquadoggo/src/graphql/types/document.rs 96.96% <75.00%> (ø)
aquadoggo/src/db/query/pagination.rs 85.00% <86.66%> (-0.19%) ⬇️
... and 10 more

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adzialocha adzialocha linked an issue Mar 23, 2023 that may be closed by this pull request
@adzialocha adzialocha changed the title Database store API for find and findMany queries Database store API for query method Mar 23, 2023
@adzialocha adzialocha changed the title Database store API for query method query method for store API Mar 23, 2023
@adzialocha adzialocha changed the base branch from query-api to main March 28, 2023 10:55
@adzialocha adzialocha changed the base branch from main to query-api March 28, 2023 10:56
@adzialocha adzialocha changed the base branch from query-api to main March 28, 2023 17:41
Copy link
Member

@sandreae sandreae left a comment

Choose a reason for hiding this comment

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

Hey! Still boggles my mind how much functionality these recent PRs introduces, and this is really the heart of it 👍 it totally feels like AbstractQuery and all the parsing into SQL strings could be a whole crate with structs and traits for implementing similar search patterns with other data sets. In another life I suppose 😄

Mostly I had just clarification requests and small corrections. There's one thing about the count query I'd like clearing up though.

Great work!

aquadoggo/src/db/models/query.rs Show resolved Hide resolved
aquadoggo/src/db/query/pagination.rs Show resolved Hide resolved
}
}

fn where_filter_sql(filter: &Filter, schema: &Schema) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

Comments on these methods are nice, even though they "do what they say"

aquadoggo/src/db/stores/query.rs Outdated Show resolved Hide resolved
aquadoggo/src/db/stores/query.rs Outdated Show resolved Hide resolved
aquadoggo/src/db/stores/query.rs Show resolved Hide resolved
aquadoggo/src/db/stores/query.rs Show resolved Hide resolved
aquadoggo/src/db/stores/query.rs Outdated Show resolved Hide resolved

let rows_per_document = std::cmp::max(fields.len(), 1);

for (index, row) in rows.into_iter().enumerate() {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work because both the rows and fields are sorted by document_view_id?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly! I'll verify that again as I did some changes recently which might affect this

aquadoggo/src/db/stores/query.rs Show resolved Hide resolved
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.

Store methods for queries
2 participants