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

EventsQuery interface and LevelIndex enhancements. #625

Merged
merged 110 commits into from
Nov 30, 2023

Conversation

LiranCohen
Copy link
Member

This PR contains a new interface method, EventsQuery, which is primarily used to query for filtered events in order to enable SelectiveSync.

EventLogLevel:

EventLogLevel now takes an additional property indexes to it's append() function. These indexes are key-value indexes in the same format that we use withe the MessageStore index.

IndexLevel:

To enable this functionality IndexLevel has been enhanced to become a more general purpose LevelDB Index that supports sorting as well as a cursor pointer property.

Copy link

codesandbox bot commented Nov 22, 2023

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

src/utils/records.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (066e74b) 98.57% compared to head (d760a0a) 98.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #625      +/-   ##
==========================================
+ Coverage   98.57%   98.80%   +0.22%     
==========================================
  Files          68       71       +3     
  Lines        8417     9011     +594     
  Branches     1227     1364     +137     
==========================================
+ Hits         8297     8903     +606     
+ Misses        114      100      -14     
- Partials        6        8       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LiranCohen LiranCohen force-pushed the lirancohen/selective-sync branch from bd9f3ec to a5b19bd Compare November 24, 2023 21:56
@LiranCohen LiranCohen force-pushed the lirancohen/selective-sync branch from 7458c12 to bff7098 Compare November 28, 2023 18:21
@LiranCohen
Copy link
Member Author

@thehenrytsai

I've updated this PR based on our discussions and the comments left.

I've separated out the logic for choosing a paging strategy, we now have shouldQueryWithInMemoryPaging to handle that decision.

And for in-memory paging I've introduced the FilterSelector.reduceFilters method to reduce the incoming filters into a more efficient set of filters.

I'd like to have some discussion regarding typing for EventsQueryFilter to get some input on improving that, I have a couple of ideas there.

For throwing an error for an invalid sortProperty rather than returning empty results, I'd like to do that in a separate PR as it has some edge cases I'd like to properly address that will be easier to review in that context. I'll open an issue and do that one and cursor together.

…ray (#632)

* Modified code such that in-memory-paging supports empty filter and array

* Removed the need for FilterSelector
@LiranCohen
Copy link
Member Author

@thehenrytsai Great improvement for legibility I'll work on getting the tests up to par. Let's catch up today on the EventsQuery filter type, I have some ideas but want to get your input there.

@LiranCohen
Copy link
Member Author

@thehenrytsai
I didn't notice this when originally bringing in this filter, but it has some inconsistencies that i noticed when writing tests.
I updated it to a simplified version but I wanted to compare them here in case there was some intent missed.

things to note:
I updated the comment, the remaining conditions will NOT have a cursor.

I wasn't sure about the intent wrt having filter.protocol !== undefined within multiple OR statements as it would only be necessary once. I also wasn't sure what the significance of filter.protocol !== undefined || filter.parentId === undefined was.

For now I just made all of these properties return true if they are defined. Let me know if there are more nuanced intents you would like to see here.

Original:

  private static isFilterConcise(filter: Filter, queryOptions: QueryOptions): boolean {
    // if there is a specific recordId in the filter, return true immediately.
    if (filter.recordId !== undefined) {
      return true;
    }

    // unless a recordId is present, if there is a cursor we never use in memory paging
    if (queryOptions.cursor !== undefined) {
      return false;
    }

    // NOTE: remaining conditions will have cursor

    if (filter.contextId !== undefined) {
      return true;
    }

    if (filter.protocol !== undefined || filter.protocolPath !== undefined) {
      return true;
    }

    if (filter.protocol !== undefined || filter.parentId === undefined) {
      return true;
    }

    if (filter.protocol === undefined && filter.schema !== undefined) {
      return true;
    }

    // all else
    return false;
  }

Updated:

  private static isFilterConcise(filter: Filter, queryOptions: QueryOptions): boolean {
    // if there is a specific recordId in the filter, return true immediately.
    if (filter.recordId !== undefined) {
      return true;
    }

    // unless a recordId is present, if there is a cursor we never use in memory paging
    if (queryOptions.cursor !== undefined) {
      return false;
    }
    // NOTE: remaining conditions will not have cursor
    if (
      filter.protocol !== undefined ||
      filter.protocolPath !== undefined ||
      filter.contextId !== undefined ||
      filter.parentId !== undefined ||
      filter.schema !== undefined
    ) {
      return true;
    }

    // all else
    return false;
  }

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.

Do it.

@LiranCohen LiranCohen merged commit 641888e into main Nov 30, 2023
4 checks passed
@LiranCohen LiranCohen deleted the lirancohen/selective-sync branch January 10, 2024 19:41
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.

3 participants