-
Notifications
You must be signed in to change notification settings - Fork 42
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
STORAGE: KVRead::iterate #919
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty good stuff!
K: 'static, | ||
V: 'static, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be static so K/V is guaranteed to outlive the iterator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I can explain, but the compiler insisted that the iterator can outlive these types 🤷
@@ -183,6 +203,48 @@ where | |||
} | |||
} | |||
|
|||
struct KVIter<'a, S: KVStore, K, V> { | |||
items: Vec<(&'a S::Repr, &'a S::Repr)>, | |||
next: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe curr_size
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I wanted to express here is the next index that will be returned from the Vec
. The vec isn't being consumed, its size stays the same.
fn next(&mut self) -> Option<Self::Item> { | ||
if let Some((k, v)) = self.items.get(self.next) { | ||
self.next += 1; | ||
let kv = S::from_repr(k).and_then(|k| S::from_repr(v).map(|v| (k, v))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
S: Decode<K>, | ||
K: 'static, | ||
V: 'static, | ||
'a: 'b, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall seeing this before, but its one of these lifetimes I never remember how is written
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also from the compiler. I didn't think adding this method on KVCollection
would involve more constraints. It complained that the impl Interator
captures the lifetimes of both reference inputs, so it needs to outlive them, and this was the only way to express that they are the same, or compatible.
Adds an
iterate
method to theKVRead
abstraction if theKVStore::Repr
type implementsOrd
.It could be extended to take a parameter like the RocksDB implementation to say whether we want to iterate from the start, the end, from a specific key, or in between two keys; perhaps
iterate
could take parameters and aKVCollection::iter
could just do the default in-order one. Here I just wanted to see how it can be implemented and where to add it.This is a support PR for #914 to implement initialisation from the storage.