Skip to content
This repository was archived by the owner on Sep 13, 2023. It is now read-only.

return error result only from Iterator.next() #14

Merged
merged 14 commits into from
Dec 19, 2018
Merged

return error result only from Iterator.next() #14

Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3fcf930
make Cursor::iter_*() methods return Result instead of panicking
mykmelez Aug 7, 2018
c3cb55c
demonstrate various uses of API
mykmelez Aug 8, 2018
99f24f7
fix test failure; clarify iterator collection type
mykmelez Oct 22, 2018
58d46ec
return Iter that produces error result from Cursor.iter*
mykmelez Oct 22, 2018
6414b83
alias Box<Iter> type to BoxedIter; make iter_dup_from return iterator…
mykmelez Oct 22, 2018
7f43959
empty commit to force CI rebuild
mykmelez Oct 23, 2018
04c7de8
empty commit to force CI rebuild
mykmelez Oct 23, 2018
6f06c1c
remove test that is no longer relevant
mykmelez Oct 23, 2018
09bd1de
remove unnecessary commented-out code
mykmelez Oct 23, 2018
a32a673
Merge branch 'return-error-result' into return-error-result-from-iter
mykmelez Oct 23, 2018
ea1386f
Merge branch 'master' into return-error-result-from-iter
mykmelez Nov 30, 2018
a558fab
convert Iter/IterDup to enums
mykmelez Dec 1, 2018
09d7656
Merge branch 'master' into return-error-result-from-iter
mykmelez Dec 1, 2018
af923dc
explicitly identify reference in match patterns
mykmelez Dec 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 68 additions & 30 deletions src/cursor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::marker::PhantomData;
use std::{fmt, mem, ptr, result, slice};
use std::{fmt, iter, mem, ptr, result, slice};

use libc::{EINVAL, c_void, size_t, c_uint};

Expand Down Expand Up @@ -57,12 +57,12 @@ pub trait Cursor<'txn> {
/// For databases with duplicate data items (`DatabaseFlags::DUP_SORT`), the
/// duplicate data items of each key will be returned before moving on to
/// the next key.
fn iter_from<K>(&mut self, key: K) -> Iter<'txn> where K: AsRef<[u8]> {
fn iter_from<K>(&mut self, key: K) -> Box<Iterator<Item=Result<(&'txn [u8], &'txn [u8])>>> where K: AsRef<[u8]> {
Copy link
Member

Choose a reason for hiding this comment

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

The way to avoid the Box is easy enough -- push the branch down into the underlying Iter struct. That is, make it:

/// An iterator over the values in an LMDB database.
pub enum Iter<'txn> {
    Err(Result::E),
    Ok(...),
}

where the bits that are currently in Iter are in the Ok branch as well.

Copy link
Author

Choose a reason for hiding this comment

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

The way to avoid the Box is easy enough -- push the branch down into the underlying Iter struct.

Ok, I've done this in a558fab.

match self.get(Some(key.as_ref()), None, ffi::MDB_SET_RANGE) {
Ok(_) | Err(Error::NotFound) => (),
Err(error) => panic!("mdb_cursor_get returned an unexpected error: {}", error),
Err(error) => return Box::new(iter::once(Err(error))),
Copy link
Member

Choose a reason for hiding this comment

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

Here, I see how you will only get an error once (and then the iterator ends). With the other approach (Item = Result<...>), how do you ensure you don't get Err forever? This feels similar to fuse in spirit.

Copy link
Author

Choose a reason for hiding this comment

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

Here, I see how you will only get an error once (and then the iterator ends). With the other approach (Item = Result<...>), how do you ensure you don't get Err forever? This feels similar to fuse in spirit.

I don't currently ensure that; rather, I leave it to the caller to decide what to do on error. I'm not entirely sure if it's possible for LMDB to return a value after returning an error. If so, then consumers should be able to continue iterating in that case.

Otherwise, consumers that call Iterator.next() directly can easily stop iterating at the first error, since they need to match on the return value anyway to distinguish between the Some(Ok), Some(Err) and None types; and collections can easily automatically stop collecting at the first error; so it seems like consumers wouldn't gain much by the Iter returning None after that.

};
Iter::new(self.cursor(), ffi::MDB_GET_CURRENT, ffi::MDB_NEXT)
Box::new(Iter::new(self.cursor(), ffi::MDB_GET_CURRENT, ffi::MDB_NEXT))
}

/// Iterate over duplicate database items. The iterator will begin with the
Expand All @@ -80,21 +80,21 @@ pub trait Cursor<'txn> {

/// Iterate over duplicate items in the database starting from the given
/// key. Each item will be returned as an iterator of its duplicates.
fn iter_dup_from<K>(&mut self, key: &K) -> IterDup<'txn> where K: AsRef<[u8]> {
fn iter_dup_from<K>(&mut self, key: &K) -> Result<IterDup<'txn>> where K: AsRef<[u8]> {
match self.get(Some(key.as_ref()), None, ffi::MDB_SET_RANGE) {
Ok(_) | Err(Error::NotFound) => (),
Err(error) => panic!("mdb_cursor_get returned an unexpected error: {}", error),
Err(error) => return Err(error),
};
IterDup::new(self.cursor(), ffi::MDB_GET_CURRENT)
Ok(IterDup::new(self.cursor(), ffi::MDB_GET_CURRENT))
}

/// Iterate over the duplicates of the item in the database with the given key.
fn iter_dup_of<K>(&mut self, key: &K) -> Iter<'txn> where K: AsRef<[u8]> {
fn iter_dup_of<K>(&mut self, key: &K) -> Box<Iterator<Item=Result<(&'txn [u8], &'txn [u8])>>> where K: AsRef<[u8]> {
match self.get(Some(key.as_ref()), None, ffi::MDB_SET) {
Ok(_) | Err(Error::NotFound) => (),
Err(error) => panic!("mdb_cursor_get returned an unexpected error: {}", error),
Err(error) => return Box::new(iter::once(Err(error))),
};
Iter::new(self.cursor(), ffi::MDB_GET_CURRENT, ffi::MDB_NEXT_DUP)
Box::new(Iter::new(self.cursor(), ffi::MDB_GET_CURRENT, ffi::MDB_NEXT_DUP))
}
}

Expand Down Expand Up @@ -238,19 +238,19 @@ impl <'txn> fmt::Debug for Iter<'txn> {

impl <'txn> Iterator for Iter<'txn> {

type Item = (&'txn [u8], &'txn [u8]);
type Item = Result<(&'txn [u8], &'txn [u8])>;

fn next(&mut self) -> Option<(&'txn [u8], &'txn [u8])> {
fn next(&mut self) -> Option<Result<(&'txn [u8], &'txn [u8])>> {
let mut key = ffi::MDB_val { mv_size: 0, mv_data: ptr::null_mut() };
let mut data = ffi::MDB_val { mv_size: 0, mv_data: ptr::null_mut() };
let op = mem::replace(&mut self.op, self.next_op);
unsafe {
match ffi::mdb_cursor_get(self.cursor, &mut key, &mut data, op) {
ffi::MDB_SUCCESS => Some((val_to_slice(key), val_to_slice(data))),
ffi::MDB_SUCCESS => Some(Ok((val_to_slice(key), val_to_slice(data)))),
// EINVAL can occur when the cursor was previously seeked to a non-existent value,
// e.g. iter_from with a key greater than all values in the database.
ffi::MDB_NOTFOUND | EINVAL => None,
error => panic!("mdb_cursor_get returned an unexpected error: {}", error),
error => Some(Err(Error::from_err_code(error))),
}
}
}
Expand Down Expand Up @@ -433,22 +433,40 @@ mod test {

let txn = env.begin_ro_txn().unwrap();
let mut cursor = txn.open_ro_cursor(db).unwrap();
assert_eq!(items, cursor.iter().collect::<Vec<_>>());

// Because Result implements FromIterator, we can collect the iterator
// of items of type Result<_, E> into a Result<Vec<_, E>> by specifying
// the collection type via the turbofish syntax.
assert_eq!(items, cursor.iter().collect::<Result<Vec<_>>>().unwrap());

// Alternately, we can collect it into an appropriately typed variable.
let retr: Result<Vec<_>> = cursor.iter_start().collect();
assert_eq!(items, retr.unwrap());

cursor.get(Some(b"key2"), None, MDB_SET).unwrap();
assert_eq!(items.clone().into_iter().skip(2).collect::<Vec<_>>(),
cursor.iter().collect::<Vec<_>>());
cursor.iter().collect::<Result<Vec<_>>>().unwrap());

assert_eq!(items, cursor.iter_start().collect::<Vec<_>>());
assert_eq!(items, cursor.iter_start().collect::<Result<Vec<_>>>().unwrap());

assert_eq!(items.clone().into_iter().skip(1).collect::<Vec<_>>(),
cursor.iter_from(b"key2").collect::<Vec<_>>());
cursor.iter_from(b"key2").collect::<Result<Vec<_>>>().unwrap());

assert_eq!(items.clone().into_iter().skip(3).collect::<Vec<_>>(),
cursor.iter_from(b"key4").collect::<Vec<_>>());
cursor.iter_from(b"key4").collect::<Result<Vec<_>>>().unwrap());

assert_eq!(vec!().into_iter().collect::<Vec<(&[u8], &[u8])>>(),
cursor.iter_from(b"key6").collect::<Vec<_>>());
cursor.iter_from(b"key6").collect::<Result<Vec<_>>>().unwrap());

// Demonstrate how a function that returns a result can use the "?"
// operator to propagate an error returned by Cursor::iter*() methods.
fn iterate<'a>(cursor: &mut RoCursor) -> Result<()> {
match cursor.iter_from("a").collect::<Result<Vec<_>>>() {
Ok(_) => Ok(()),
Err(error) => Err(error),
}
}
iterate(&mut cursor).unwrap();
}

#[test]
Expand Down Expand Up @@ -477,7 +495,7 @@ mod test {
assert_eq!(0, cursor.iter_from(b"foo").count());
assert_eq!(0, cursor.iter_dup().count());
assert_eq!(0, cursor.iter_dup_start().count());
assert_eq!(0, cursor.iter_dup_from(b"foo").count());
assert_eq!(0, cursor.iter_dup_from(b"foo").unwrap().count());
assert_eq!(0, cursor.iter_dup_of(b"foo").count());
}

Expand Down Expand Up @@ -510,29 +528,29 @@ mod test {

let txn = env.begin_ro_txn().unwrap();
let mut cursor = txn.open_ro_cursor(db).unwrap();
assert_eq!(items, cursor.iter_dup().flat_map(|x| x).collect::<Vec<_>>());
assert_eq!(items, cursor.iter_dup().flat_map(|x| x).collect::<Result<Vec<_>>>().unwrap());

cursor.get(Some(b"b"), None, MDB_SET).unwrap();
assert_eq!(items.clone().into_iter().skip(4).collect::<Vec<(&[u8], &[u8])>>(),
cursor.iter_dup().flat_map(|x| x).collect::<Vec<_>>());
cursor.iter_dup().flat_map(|x| x).collect::<Result<Vec<_>>>().unwrap());

assert_eq!(items,
cursor.iter_dup_start().flat_map(|x| x).collect::<Vec<(&[u8], &[u8])>>());
cursor.iter_dup_start().flat_map(|x| x).collect::<Result<Vec<(&[u8], &[u8])>>>().unwrap());

assert_eq!(items.clone().into_iter().skip(3).collect::<Vec<(&[u8], &[u8])>>(),
cursor.iter_dup_from(b"b").flat_map(|x| x).collect::<Vec<_>>());
cursor.iter_dup_from(b"b").unwrap().flat_map(|x| x).collect::<Result<Vec<_>>>().unwrap());

assert_eq!(items.clone().into_iter().skip(3).collect::<Vec<(&[u8], &[u8])>>(),
cursor.iter_dup_from(b"ab").flat_map(|x| x).collect::<Vec<_>>());
cursor.iter_dup_from(b"ab").unwrap().flat_map(|x| x).collect::<Result<Vec<_>>>().unwrap());

assert_eq!(items.clone().into_iter().skip(9).collect::<Vec<(&[u8], &[u8])>>(),
cursor.iter_dup_from(b"d").flat_map(|x| x).collect::<Vec<_>>());
cursor.iter_dup_from(b"d").unwrap().flat_map(|x| x).collect::<Result<Vec<_>>>().unwrap());

assert_eq!(vec!().into_iter().collect::<Vec<(&[u8], &[u8])>>(),
cursor.iter_dup_from(b"f").flat_map(|x| x).collect::<Vec<_>>());
cursor.iter_dup_from(b"f").unwrap().flat_map(|x| x).collect::<Result<Vec<_>>>().unwrap());

assert_eq!(items.clone().into_iter().skip(3).take(3).collect::<Vec<(&[u8], &[u8])>>(),
cursor.iter_dup_of(b"b").collect::<Vec<_>>());
cursor.iter_dup_of(b"b").collect::<Result<Vec<_>>>().unwrap());

assert_eq!(0, cursor.iter_dup_of(b"foo").count());
}
Expand Down Expand Up @@ -571,10 +589,30 @@ mod test {
let mut i = 0;
let mut count = 0u32;

for (key, data) in cursor.iter() {
for (key, data) in cursor.iter().map(Result::unwrap) {
i = i + key.len() + data.len();
count = count + 1;
}
for (key, data) in cursor.iter().filter_map(Result::ok) {
i = i + key.len() + data.len();
count = count + 1;
}

fn iterate<'a>(cursor: &mut RoCursor) -> Result<()> {
let mut i = 0;
let mut count = 0u32;
for result in cursor.iter() {
// let (key, data) = match result {
// Ok((key, data)) => (key, data),
// Err(error) => return Err(error),
// };
let (key, data) = result?;
i = i + key.len() + data.len();
count = count + 1;
}
Ok(())
}
iterate(&mut cursor).unwrap();

black_box(i);
assert_eq!(count, n);
Expand Down