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

MessageStore interfaces for sorting and pagination #478

Merged
merged 36 commits into from
Sep 18, 2023

Conversation

LiranCohen
Copy link
Member

Starting point for #274

Modified the DateSort enum into the following types:

export enum RecordsDateSort {
  CreatedAscending = 'createdAscending',
  CreatedDescending = 'createdDescending',
  PublishedAscending = 'publishedAscending',
  PublishedDescending = 'publishedDescending',
}

export enum TimestampDateSort {
  TimestampAscending = 'timestampAscending',
  TimestampDescending = 'timestampDescending'
}

export type DateSort = RecordsDateSort | TimestampDateSort;

This allows explicit types for Records sorting(by created or published) vs generic message sorting (by timestamp).

Currently RecordsQuery specify the default sort to be RecordsDateSort.CreatedDescending
Everything else defaults to TimestampDateSort.TimestampDescending

Either as a part of this effort or as a different PR there can be some enhancements to certain parts of the code that either look for only 1 message, or attempt to sort the message they're looking for via Message.getNewestMessage or getOldestMessage.

@LiranCohen LiranCohen force-pushed the lirancohen/message-store-sorting-pagiation branch from 7782db6 to b2d9aac Compare August 22, 2023 00:06
@LiranCohen LiranCohen force-pushed the lirancohen/message-store-sorting-pagiation branch 3 times, most recently from 454767c to 2e1ca9c Compare August 24, 2023 20:49
@LiranCohen
Copy link
Member Author

I did some more thinking about what this will look like when fully implemented with level, this is what I came away with and would love your thoughts @thehenrytsai & @diehuxx.

I generalized the MessageSort type as per @thehenrytsai's suggestion, however as I was thinking about truly generalizing the implementation to be inspired by MongoDB (which accepts multiple fields, and generalized sorting of fields) I wasn't sure if that kind of overhead was really necessary.

For every type of property we are sorting by, the number of indexes grows. So for now we should really only sort by dateCreated and datePublished where they are available (currently RecordsWrite), and sort by messageTimestamp as a default for everything else.

I think leaving the interface generalized is a good idea so that we can add other higher level abstractions with different types of dateSort fields to different message types and only need to add indexes.

Also thinking about pagination, cursor based pagination started making a lot more sense within the DWN context. I think we should start there and move to offset based pagination if there is a strong demand for it, as it will also add overhead to indexing and performance that might not be necessary.

Right now I am thinking that the cursor pointer for pagination be the messageCid and I went ahead and added that to the response in RecordsQuery since authorization is stripped. This cursor applies generically to all types of data that are stored within the message store and gives us an easy way to look up index starting points given the existing indexing scheme.

Would be happy to discuss this more in depth over a call. But for now figured looking at the interface and naive implementation will be a good place to start.

@LiranCohen LiranCohen force-pushed the lirancohen/message-store-sorting-pagiation branch from 2e1ca9c to 78714cb Compare September 6, 2023 12:39
@LiranCohen
Copy link
Member Author

@thehenrytsai I think this is ready for a review at this point after yesterday's conversation.

I was going to add some types/properties to describe the sortable index fields somewhere, but it felt like premature optimization.

@LiranCohen LiranCohen marked this pull request as ready for review September 6, 2023 15:50
@LiranCohen LiranCohen force-pushed the lirancohen/message-store-sorting-pagiation branch from bdc2caf to 665686a Compare September 7, 2023 18:52
@LiranCohen LiranCohen force-pushed the lirancohen/message-store-sorting-pagiation branch from 665686a to 8602426 Compare September 8, 2023 01:53
@LiranCohen LiranCohen force-pushed the lirancohen/message-store-sorting-pagiation branch 2 times, most recently from 8e94251 to 97d3930 Compare September 13, 2023 17:35
@LiranCohen LiranCohen force-pushed the lirancohen/message-store-sorting-pagiation branch from 1125755 to 6588d01 Compare September 15, 2023 15:40
@LiranCohen LiranCohen force-pushed the lirancohen/message-store-sorting-pagiation branch from 74c9f19 to 4cc42be Compare September 18, 2023 16:31
Copy link
Contributor

@thehenrytsai thehenrytsai left a comment

Choose a reason for hiding this comment

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

🥇 🎸 🚀 Long time coming!

@LiranCohen LiranCohen merged commit c4b13dc into main Sep 18, 2023
@LiranCohen LiranCohen deleted the lirancohen/message-store-sorting-pagiation branch September 18, 2023 18:08
LiranCohen added a commit to decentralized-identity/dwn-sql-store that referenced this pull request Sep 26, 2023
Updating the DWN SDK to the latest version.

This Includes:
 - `encodedData` stored within the `MessageStore` below a certain threshold
 - `MessageStore` interface now accepts multiple Filters to create OR conditions between filters.
 - 'MessageStore' interface accepts sorting and pagination inputs
 - If sorting by `datePublished`, add the property `published` set to true within the filter.
 
 Relevent `dwn-sdk-js` PRs:
 decentralized-identity/dwn-sdk-js#478
 decentralized-identity/dwn-sdk-js#456

* upgrade kysley for tuple support

* upgrade sdk and queries, first pass -- tests fail

* sorting/pagination

* store encodedData when available, sort and paginate according to messageCid cusror

* added more comments

* added comment about removing encodedData from message before encoding for storage

* bump version

* new release version

* npm i after version bump
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