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

[GraphQL/Events] Disable filtering on both event_type and emitting_module #18740

Merged
merged 16 commits into from
Aug 28, 2024

Conversation

wlmyng
Copy link
Contributor

@wlmyng wlmyng commented Jul 19, 2024

Description

From GraphQL performance benchmarks, we observed that events queries filtered on both event_type and emitting_module frequently time out. Some approaches were explored to support this, but ultimately we will do away with this combination for the time being.

The new paginated events query consists of a few roundtrips, first to collect a list of tx_sequence_number and event_sequence_number, and then a subsequent query composed of a series of UNION ALLs to leverage the index on the events table.

If filters are not specified for event_type, emitting_module, or sender, then we can query the events table directly.

Test plan

  1. event_connection/combo_filter_error.move - tests that combining event_type and emitting_module will error
  2. event_connection/no_filter.move - tests cursor pagination on an events query without any filters works as expected
  3. event_connection/tx_digest.move - tests that when cursors and a filter on digest are used in tandem, that results are empty if they do not point to the same tx_sequence_number, and default to the cursor's event_sequence_number exclusive as the starting or ending point
  4. event_connection/type_filter.move - tests event_type filter works
  5. event_connection/type_param_filter.move - tests event_type with type param instantiation works

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL: Events queries no longer support filtering on both eventType and emittingModule
  • CLI:
  • Rust SDK:
  • REST API:

Copy link

vercel bot commented Jul 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 28, 2024 4:35pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Aug 28, 2024 4:35pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Aug 28, 2024 4:35pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Aug 28, 2024 4:35pm

@wlmyng wlmyng changed the title Wlmyng/gql/new events query [gql] new events query implementation based on benchmark findings Jul 19, 2024
@wlmyng wlmyng marked this pull request as ready for review July 21, 2024 08:15
@wlmyng wlmyng changed the base branch from wlmyng/gql/scan-limit-tx-approach to head-for-new-events July 26, 2024 14:18
@emmazzz
Copy link
Contributor

emmazzz commented Aug 11, 2024

By the way we'll need to apply the schema changes (removal of some columns in event tables that won't be used anymore in graphql) in this commit as well before merging this PR into main, as the backfills are done with these event columns removed.

@wlmyng wlmyng force-pushed the wlmyng/gql/new-events-query branch from f939ba2 to beadcc6 Compare August 19, 2024 18:05
@wlmyng wlmyng requested review from pchrysochoidis and mystieanwaya and removed request for a team August 19, 2024 18:05
@wlmyng wlmyng changed the base branch from head-for-new-events to wlmyng/gql/scan-limit-tx-approach August 19, 2024 18:05
@amnn
Copy link
Member

amnn commented Aug 28, 2024

Heads up @wlmyng and @emmazzz -- I took the liberty of simplifying add_bounds. Seeing as we aren't supporting compound queries, we don't need to use CTEs, which significantly simplifies the query. This also means we can get rid of the support for RawQuery::with.

I also took the opportunity to clean up some other things that were maybe a bit problematic:

  • tx_hi - 1 -- there's a small underflow risk which we can avoid there.
  • Using u64::MAX as the default event sequence number upperbound also carries an overflow risk, as Postgres uses signed integers, so I've avoided that too.

wlmyng and others added 15 commits August 28, 2024 08:43
… respective types

rename ev_cursor to just cursor

use bytea_literal in pg module instead of individually in digest and sui_address

logic for constructing the raw query, which is a single roundtrip consisting of multiple ctes to determine the tx and ev range

i think its working? time for some tests and cleanup

introduce new cte field to RawQuery

remove unused code

for now, fetch checkpoint_viewed_at's tx upper

split things out so hopefully its easier to review and makes more sense compartmentalized like this?

tx digest - test digest no events, and then digest with events

test base case of no filter also works
Seeing as we don't support compound filters for events, we don't need to
go to the trouble of using CTEs just yet.
@wlmyng wlmyng force-pushed the wlmyng/gql/new-events-query branch from 9cf7d94 to 5150155 Compare August 28, 2024 15:45
@wlmyng wlmyng changed the title [gql] new events query implementation based on benchmark findings [GraphQL/Events] Disable filtering on both event_type and emitting_module Aug 28, 2024
@wlmyng wlmyng merged commit 3f03d1c into main Aug 28, 2024
48 checks passed
@wlmyng wlmyng deleted the wlmyng/gql/new-events-query branch August 28, 2024 17:22
emmazzz added a commit that referenced this pull request Aug 30, 2024
…itting… (#19135)

…_module` (#18740)

## Description 

Cherry pick event indexer + graphql changes.
## Test plan 

How did you test the new or updated feature?

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:

---------

Co-authored-by: wlmyng <127570466+wlmyng@users.noreply.github.com>
Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com>
jangid pushed a commit to jangid/sui that referenced this pull request Aug 30, 2024
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.

4 participants