Skip to content

Fix limit prefix delete case #368

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.

Sign up for GitHub

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

Merged
ordian merged 8 commits into from
Apr 23, 2020
Merged

Fix limit prefix delete case #368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4b0aa96
Fix limit prefix delete case. Warning this uses iter by prefix for
cheme Apr 4, 2020
336522d
Comment
cheme Apr 4, 2020
c1e543e
Merge branch 'master' into del_range_bound
cheme Apr 15, 2020
7b68b22
Changes needed for merging, end_prefix returning option in new code and
cheme Apr 15, 2020
f79431c
reduce cornercase iteration windows size.
cheme Apr 23, 2020
c824d3c
extract common iter prefix delete corner case code.
cheme Apr 23, 2020
f3d8dbd
Update kvdb-rocksdb/src/lib.rs
cheme Apr 23, 2020
d3b6bf3
Remove unused code, there can be call to 'delete_range_cf' on empty r…
cheme Apr 23, 2020
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
7 changes: 5 additions & 2 deletions kvdb-memorydb/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,11 @@ impl KeyValueDB for InMemory {
col.clear();
} else {
let start_range = Bound::Included(prefix.to_vec());
let end_range = Bound::Excluded(kvdb::end_prefix(&prefix[..]));
let keys: Vec<_> = col.range((start_range, end_range)).map(|(k, _)| k.clone()).collect();
let keys: Vec<_> = if let Some(end_range) = kvdb::end_prefix(&prefix[..]) {
col.range((start_range, Bound::Excluded(end_range))).map(|(k, _)| k.clone()).collect()
} else {
col.range((start_range, Bound::Unbounded)).map(|(k, _)| k.clone()).collect()
};
for key in keys.into_iter() {
col.remove(&key[..]);
}
Expand Down
15 changes: 11 additions & 4 deletions kvdb-rocksdb/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,15 +443,22 @@ impl Database {
stats_total_bytes += key.len();
batch.delete_cf(cf, &key).map_err(other_io_err)?
}
DBOp::DeletePrefix { col: _, prefix } => {
DBOp::DeletePrefix { col, prefix } => {
if prefix.len() > 0 {
let end_range = kvdb::end_prefix(&prefix[..]);
batch.delete_range_cf(cf, &prefix[..], &end_range[..]).map_err(other_io_err)?;
if let Some(end_range) = kvdb::end_prefix(&prefix[..]) {
batch.delete_range_cf(cf, &prefix[..], &end_range[..]).map_err(other_io_err)?;
} else {
for (key, _) in self.iter_from_prefix(col, &prefix[..]) {
batch.delete_cf(cf, &key[..]).map_err(other_io_err)?;
}
Comment thread
ordian marked this conversation as resolved.
Outdated
}
} else {
// Deletes all values in the column.
let end_range = &[u8::max_value()];
Comment thread
ordian marked this conversation as resolved.
Outdated
batch.delete_range_cf(cf, &prefix[..], &end_range[..]).map_err(other_io_err)?;
batch.delete_cf(cf, &end_range[..]).map_err(other_io_err)?;
for (key, _) in self.iter_from_prefix(col, &end_range[..]) {
batch.delete_cf(cf, &key[..]).map_err(other_io_err)?;
}
}
}
};
Expand Down
21 changes: 13 additions & 8 deletions kvdb-shared-tests/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ pub fn test_io_stats(db: &dyn KeyValueDB) -> io::Result<()> {
}

/// The number of columns required to run `test_delete_prefix`.
pub const DELETE_PREFIX_NUM_COLUMNS: u32 = 5;
pub const DELETE_PREFIX_NUM_COLUMNS: u32 = 7;

/// A test for `KeyValueDB::delete_prefix`.
pub fn test_delete_prefix(db: &dyn KeyValueDB) -> io::Result<()> {
Expand All @@ -190,6 +190,7 @@ pub fn test_delete_prefix(db: &dyn KeyValueDB) -> io::Result<()> {
&[2][..],
&[2, 0][..],
&[2, 255][..],
&[255, 255][..],
];
let init_db = |ix: u32| -> io::Result<()> {
let mut batch = db.transaction();
Expand All @@ -199,8 +200,8 @@ pub fn test_delete_prefix(db: &dyn KeyValueDB) -> io::Result<()> {
db.write(batch)?;
Ok(())
};
let check_db = |ix: u32, content: [bool; 10]| -> io::Result<()> {
let mut state = [true; 10];
let check_db = |ix: u32, content: [bool; 11]| -> io::Result<()> {
let mut state = [true; 11];
for (c, key) in keys.iter().enumerate() {
state[c] = db.get(ix, key)?.is_some();
}
Expand All @@ -209,15 +210,19 @@ pub fn test_delete_prefix(db: &dyn KeyValueDB) -> io::Result<()> {
};
let tests: [_; DELETE_PREFIX_NUM_COLUMNS as usize] = [
// standard
(&[1u8][..], [true, true, true, false, false, false, false, true, true, true]),
(&[1u8][..], [true, true, true, false, false, false, false, true, true, true, true]),
// edge
(&[1u8, 255, 255][..], [true, true, true, true, true, true, false, true, true, true]),
(&[1u8, 255, 255][..], [true, true, true, true, true, true, false, true, true, true, true]),
// none 1
(&[1, 2][..], [true, true, true, true, true, true, true, true, true, true]),
(&[1, 2][..], [true, true, true, true, true, true, true, true, true, true, true]),
// none 2
(&[8][..], [true, true, true, true, true, true, true, true, true, true]),
(&[8][..], [true, true, true, true, true, true, true, true, true, true, true]),
// last value
(&[255, 255][..], [true, true, true, true, true, true, true, true, true, true, false]),
// last value, limit prefix
(&[255][..], [true, true, true, true, true, true, true, true, true, true, false]),
// all
(&[][..], [false, false, false, false, false, false, false, false, false, false]),
(&[][..], [false, false, false, false, false, false, false, false, false, false, false]),
];
for (ix, test) in tests.iter().enumerate() {
let ix = ix as u32;
Expand Down
22 changes: 12 additions & 10 deletions kvdb/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,17 @@ pub trait KeyValueDB: Sync + Send + parity_util_mem::MallocSizeOf {

/// For a given start prefix (inclusive), returns the correct end prefix (non-inclusive).
/// This assumes the key bytes are ordered in lexicographical order.
pub fn end_prefix(prefix: &[u8]) -> Vec<u8> {
pub fn end_prefix(prefix: &[u8]) -> Option<Vec<u8>> {
Comment thread
ordian marked this conversation as resolved.
let mut end_range = prefix.to_vec();
while let Some(0xff) = end_range.last() {
end_range.pop();
}
if let Some(byte) = end_range.last_mut() {
*byte += 1;
Some(end_range)
} else {
None
}
end_range
}

#[cfg(test)]
Expand All @@ -159,17 +161,17 @@ mod test {

#[test]
fn end_prefix_test() {
assert_eq!(end_prefix(&[5, 6, 7]), vec![5, 6, 8]);
assert_eq!(end_prefix(&[5, 6, 255]), vec![5, 7]);
assert_eq!(end_prefix(&[5, 6, 7]), Some(vec![5, 6, 8]));
assert_eq!(end_prefix(&[5, 6, 255]), Some(vec![5, 7]));
// This is not equal as the result is before start.
assert_ne!(end_prefix(&[5, 255, 255]), vec![5, 255]);
assert_ne!(end_prefix(&[5, 255, 255]), Some(vec![5, 255]));
// This is equal ([5, 255] will not be deleted because
// it is before start).
assert_eq!(end_prefix(&[5, 255, 255]), vec![6]);
assert_eq!(end_prefix(&[255, 255, 255]), vec![]);
assert_eq!(end_prefix(&[5, 255, 255]), Some(vec![6]));
assert_eq!(end_prefix(&[255, 255, 255]), None);

assert_eq!(end_prefix(&[0x00, 0xff]), vec![0x01]);
assert_eq!(end_prefix(&[0xff]), vec![]);
assert_eq!(end_prefix(&[]), vec![]);
assert_eq!(end_prefix(&[0x00, 0xff]), Some(vec![0x01]));
assert_eq!(end_prefix(&[0xff]), None);
assert_eq!(end_prefix(&[]), None);
}
}