Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: Clément Renault <renault.cle@gmail.com>
  • Loading branch information
irevoire and Kerollmops committed Aug 30, 2022
1 parent 5af1213 commit 00484b7
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 10 deletions.
19 changes: 10 additions & 9 deletions src/bitmap/multiops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::{MultiOps, RoaringBitmap};

use super::{container::Container, store::Store};

/// When collecting bitmaps for optimizing the computation.If we don't know how many
/// When collecting bitmaps for optimizing the computation. If we don't know how many
// elements are in the iterator we collect 10 elements.
const BASE_COLLECT: usize = 10;

Expand Down Expand Up @@ -120,7 +120,7 @@ fn try_multi_and_owned<E>(

// We're going to take a bunch of elements at the start of the iterator and sort
// them to reduce the size of our bitmap faster.
let mut start = collect_starting_elements::<_, Result<Vec<_>, _>>(iter.by_ref())?;
let mut start = collect_starting_elements::<_, _, Result<Vec<_>, _>>(iter.by_ref())?;
start.sort_unstable_by_key(|bitmap| bitmap.containers.len());
let mut start = start.into_iter();

Expand Down Expand Up @@ -152,7 +152,7 @@ fn try_multi_and_ref<'a, E>(

// We're going to take a bunch of elements at the start of the iterator and sort
// them to reduce the size of our bitmap faster.
let mut start = collect_starting_elements::<_, Result<Vec<_>, _>>(iter.by_ref())?;
let mut start = collect_starting_elements::<_, _, Result<Vec<_>, _>>(iter.by_ref())?;
start.sort_unstable_by_key(|bitmap| bitmap.containers.len());
let mut start = start.into_iter();

Expand Down Expand Up @@ -223,7 +223,7 @@ fn try_multi_or_owned<E>(

// We're going to take a bunch of elements at the start of the iterator and
// move the biggest one first to grow faster.
let mut start = collect_starting_elements::<_, Result<Vec<_>, _>>(iter.by_ref())?;
let mut start = collect_starting_elements::<_, _, Result<Vec<_>, _>>(iter.by_ref())?;
start.sort_unstable_by_key(|bitmap| Reverse(bitmap.containers.len()));
let start_size = start.len();
let mut start = start.into_iter();
Expand Down Expand Up @@ -312,7 +312,7 @@ fn try_multi_or_ref<'a, E: 'a>(

// Phase 1. Borrow all the containers from the first element.
let mut iter = bitmaps.into_iter();
let mut start = collect_starting_elements::<_, Result<Vec<_>, _>>(iter.by_ref())?;
let mut start = collect_starting_elements::<_, _, Result<Vec<_>, _>>(iter.by_ref())?;
let start_size = start.len();

start.sort_unstable_by_key(|bitmap| Reverse(bitmap.containers.len()));
Expand All @@ -337,7 +337,7 @@ fn try_multi_or_ref<'a, E: 'a>(
}

// Phase 3: Clean up
let containers: Vec<Container> = containers
let containers: Vec<_> = containers
.into_iter()
.map(|c| {
// Any borrowed bitmaps or arrays left over get cloned here
Expand Down Expand Up @@ -378,7 +378,7 @@ fn try_multi_xor_ref<'a, E: 'a>(
}

// Phase 3: Clean up
let containers: Vec<Container> = containers
let containers: Vec<_> = containers
.into_iter()
.map(|c| {
// Any borrowed bitmaps or arrays left over get cloned here
Expand Down Expand Up @@ -431,9 +431,10 @@ fn merge_container_ref<'a>(
}
}

fn collect_starting_elements<I, O>(iter: impl IntoIterator<Item = I>) -> O
fn collect_starting_elements<I, T, O>(iter: I) -> O
where
O: FromIterator<I>,
I: IntoIterator<Item = T>,
O: FromIterator<T>,
{
let mut iter = iter.into_iter();
let mut to_collect = iter.size_hint().1.unwrap_or(BASE_COLLECT);
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ impl Error for NonSortedIntegers {}

/// A [`collect`] blanket implementation that provides extra methods for [`RoaringBitmap`]
/// and [`RoaringTreemap`].
///
/// When merging multiple bitmap with the same operation it's usually faster to call the
/// method in this trait than to write your own for loop and merging the bitmaps yourself.
///
Expand Down
3 changes: 2 additions & 1 deletion src/treemap/multiops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ where
};
let mut treemaps = treemaps.collect::<Result<Vec<_>, _>>()?;

// for each keys in the first treemap we're going find and accumulate all the corresponding bitmaps
// for each key in the first treemap we're going to find and
// accumulate all the corresponding bitmaps
let keys: Vec<_> = treemap.map.keys().copied().collect();
for k in keys {
// the unwrap is safe since we're iterating on our keys
Expand Down

0 comments on commit 00484b7

Please sign in to comment.