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

sui-field-count: const instead of fn and use it in sui-indexer-alt #20143

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gegaowp
Copy link
Contributor

@gegaowp gegaowp commented Nov 1, 2024

Description

  • change field_count trait from fn to const as we want to use it as part of const def
  • use it in sui-indexer-alt

Test plan

  1. existing tests for sui-field-count
  2. for replacements, I added temp tests to ensure the values before and after were the same, removed them afterwards to avoid unnecessary changes later on when we change struct definition

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:

Copy link

vercel bot commented Nov 1, 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 Nov 1, 2024 8:38pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Nov 1, 2024 8:38pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Nov 1, 2024 8:38pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Nov 1, 2024 8:38pm

@@ -22,13 +22,6 @@ use crate::{
schema::sum_coin_balances,
};

/// Each insert or update will include at most this many rows -- the size is chosen to maximize the
/// rows without hitting the limit on bind parameters.
const UPDATE_CHUNK_ROWS: usize = i16::MAX as usize / 5;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amnn I saw the *_CHUNK_ROWS only exist in these 2 files, while it should be applied to all tables?

Copy link
Member

Choose a reason for hiding this comment

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

So having seen how things are set-up, I think we'll want to make use of FIELD_COUNT slightly differently -- sorry for the churn:

  • We need to handle the integration of FIELD_COUNTs differently for concurrent and sequential pipelines. This means we can't do the integration once, in the Processor trait. We need to do it in the concurrent and sequential Handler traits respectively.
  • The concurrent::Handler is easiest to integrate with -- it already has a MAX_CHUNK_ROWS parameter whose definition we can replace as you've done here, and this parameter is used by the framework automatically.
  • You will also need to remove the overrides for MAX_CHUNK_ROWS in concurrent::Handler impls.
  • Sequential pipelines (like this one) don't automatically chunk their rows -- that's done by each implementation of sequential::Handler::commit, so for these ones, the best thing to do is to keep the constants per handler implementation. (we also can't say that e.g. all deletions in sequential pipelines need exactly one bind per row).

@@ -159,7 +152,7 @@ impl Handler for SumCoinBalances {
}
}

let update_chunks = updates.chunks(UPDATE_CHUNK_ROWS).map(|chunk| {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added temp tests to ensure values are the same before and after, removed them to avoid overheard later on.

Copy link
Member

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Thanks @gegaowp -- the change from a function to a constant looks good, but the integration needs some tweaking, PTAL.

@@ -22,13 +22,6 @@ use crate::{
schema::sum_coin_balances,
};

/// Each insert or update will include at most this many rows -- the size is chosen to maximize the
/// rows without hitting the limit on bind parameters.
const UPDATE_CHUNK_ROWS: usize = i16::MAX as usize / 5;
Copy link
Member

Choose a reason for hiding this comment

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

So having seen how things are set-up, I think we'll want to make use of FIELD_COUNT slightly differently -- sorry for the churn:

  • We need to handle the integration of FIELD_COUNTs differently for concurrent and sequential pipelines. This means we can't do the integration once, in the Processor trait. We need to do it in the concurrent and sequential Handler traits respectively.
  • The concurrent::Handler is easiest to integrate with -- it already has a MAX_CHUNK_ROWS parameter whose definition we can replace as you've done here, and this parameter is used by the framework automatically.
  • You will also need to remove the overrides for MAX_CHUNK_ROWS in concurrent::Handler impls.
  • Sequential pipelines (like this one) don't automatically chunk their rows -- that's done by each implementation of sequential::Handler::commit, so for these ones, the best thing to do is to keep the constants per handler implementation. (we also can't say that e.g. all deletions in sequential pipelines need exactly one bind per row).

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