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

Implements DoubleEndedIterator for trie iterators #208

Merged
merged 16 commits into from
Jan 5, 2024

Conversation

snowmead
Copy link
Contributor

@snowmead snowmead commented Dec 22, 2023

Closes #164

Enables bidirectional iteration by implementing the DoubleEndedIterator for all iterator types.

New double ended iterator structs:

  • TrieDBDoubleEndedIterator
  • TrieDBNodeDoubleEndedIterator
  • TrieDBKeyDoubleEndedIterator
  • FatDBDoubleEndedIterator

Internal iteration methods now accept a fwd bool flag to conditionally traverse a trie backwards or forwards (non exhaustive list).

  • TrieDBRawIterator::next_raw_item
  • TrieDBRawIterator::seek
  • Crumb::increment (renamed to Crumb::step to better convey its behaviour)

Modifies next_raw_item to return middle node (i.e. a node containing child nodes) in reverse order based on iteration direction. Middle node is returned last when iterating backwards, mirroring forward iteration behaviour.

Adds new AftExiting status to pop crumb after returning it in Exiting status for backward iteration.

Library users can create one of these iterators from a TrieDB instance by calling the public TrieDB::into_* methods.

let trie = TrieDBBuilder::<T>::new(&memdb, &root).build();
// instantiates `TrieDBDoubleEndedIterator`
let mut iter = trie.into_double_ended_iter().unwrap();

// forward iteration
iter.next().unwrap().unwrap());
// backward iteration
iter.next_back()

@snowmead snowmead marked this pull request as ready for review December 27, 2023 19:10
Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

trie-db/src/iterator.rs Outdated Show resolved Hide resolved
trie-db/src/iterator.rs Outdated Show resolved Hide resolved
trie-db/src/iterator.rs Outdated Show resolved Hide resolved
trie-db/src/iterator.rs Outdated Show resolved Hide resolved
snowmead and others added 3 commits December 28, 2023 08:33
…n methods, remove back_trail & back_key_nibbles, remove clone from OwnedNode
@snowmead
Copy link
Contributor Author

snowmead commented Dec 28, 2023

@cheme

The modification for iterating backward seems straightforward to me.

However, I believe adding this behaviour in a standard iterator is unintuitive for library users.

Additionally, iterating backward with a single cursor might prematurely end the iteration when it reaches the back of the trie.

That's a valid concern. It could potentially be addressed by always including the root node or prefix node in the crumb list. Yet, I am uncertain whether the change justifies the effort.

IMHO I don't think it does. I believe having separate structs for explicit iteration in both directions enhances code clarity. It also provides a better understanding of the behavior of standard iterator structs compared to double ended iterator structs.

I've implemented modifications based on our discussion (the unit test demonstrates this):

  • Renamed increment to step.
  • Double ended iterator structs can be created from TrieDB (added functions). I'm unsure if a more elegant or generic solution is desired.
  • Deduplicated all internal iteration functions and added the fwd bool parameter.
  • Created double ended iterator structs for each type of iterator (e.g., node, key, etc.).

Still need to add a bunch of tests.

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, into_double_ended_iter api looks good to me.

trie-db/src/fatdb.rs Show resolved Hide resolved
trie-db/src/iterator.rs Outdated Show resolved Hide resolved
@cheme cheme requested a review from arkpar December 29, 2023 09:24
@cheme
Copy link
Contributor

cheme commented Jan 3, 2024

Small note (not something that needs change right now), I think that the raw node iter can return on a status At, then if iterating in different direction we may not have the right next node : eg entering a branch backward then state is At then calling iterator forward and then state become AtChild(0) when we should have exit.
This is not an issue right now as we don't expose different iteration direction, but at some point I will probably switch to state At(direction: bool) to be able to support this case.

@cheme cheme merged commit 2edd0a1 into paritytech:master Jan 5, 2024
4 checks passed
@snowmead snowmead deleted the snowmead-double-ended-iterator branch January 5, 2024 22:50
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.

Implement DoubleEndedIterator for TrieDBNodeIterator and TrieDBIterator
3 participants