Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add methods to PrefixIterator to support iterating from a specific key #9313

Merged
18 commits merged into from
Jul 27, 2021

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Jul 9, 2021

Fixes #9213.

This adds methods that aid in paginating storage iterators. I imagine that this will be used heavily in storage migrations that span multiple blocks, or other sorts of consensus mechanisms that span multiple blocks and require interaction with storage.

@KiChjang KiChjang added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 9, 2021
frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
frame/support/src/storage/mod.rs Show resolved Hide resolved
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I feel like the PR is correct, but incomplete. I don't see any change in public interfaces like IterableStorageMap etc. My assumption was that there'd be a new fn iter_from() added to these traits, which would create an iterating type from KeyPrefixIterator or similar types.

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

looks good but I would prefer having

  • some more tests
  • order specified
  • some naming nitpicks

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

looks good to me

fn drain_prefix(k1: impl EncodeLike<K1>) -> Self::PrefixIterator {
let mut iterator = Self::iter_prefix(k1);
iterator.drain = true;
iterator
}

fn iter() -> Self::Iterator {
Self::iter_from(G::prefix_hash())
Copy link
Contributor

Choose a reason for hiding this comment

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

Wee bit strange here. iter_from is already reading let prefix = G::prefix_hash();, but we pass in G::prefix_hash() to it again. I'd make the starting_raw_key an Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can just revert this and implement iter_from in terms of iter by calling set_previous_key on the resulting PrefixIterator.

@@ -144,10 +144,15 @@ impl<

/// Enumerate all elements in the map.
fn iter() -> Self::Iterator {
Self::iter_from(G::prefix_hash())
Copy link
Contributor

Choose a reason for hiding this comment

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

same grumble.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I've got one minor note, otherwise looks good now.

@KiChjang
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Jul 27, 2021

Waiting for commit status.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iterate one storage map over blocks
4 participants