Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Journaling bloom filter crate in util #2395

Merged
merged 4 commits into from
Sep 30, 2016
Merged

Journaling bloom filter crate in util #2395

merged 4 commits into from
Sep 30, 2016

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Sep 29, 2016

No description provided.

@NikVolf NikVolf added the A0-pleasereview 🤓 Pull request needs code review. label Sep 29, 2016
pub hash_functions: u32,
pub entries: Vec<(usize, u64)>,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

tests could be in a separate tests module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, though it's not much of a convention

@rphmeier
Copy link
Contributor

rphmeier commented Sep 29, 2016

Needs the license from the original work: https://github.com/jedisct1/rust-bloom-filter/blob/master/LICENSE

Actually, I think we might need this license in our distribution if we use this project in any form if statically linking it in counts as a binary distribution.

(is this compatible with our GPL? @tjsaw)

@NikVolf
Copy link
Contributor Author

NikVolf commented Sep 29, 2016

@rphmeier
don't think any license talks valid
it is 3 or 4 lines left from the original crate
and it uses completely different logic and data structures

@NikVolf NikVolf closed this Sep 29, 2016
@NikVolf NikVolf reopened this Sep 29, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 59c0551 on bloom-crate into * on master*.

@arkpar
Copy link
Collaborator

arkpar commented Sep 30, 2016

This is BSD-2-Clause. Pretty sure it is compatible

}

pub fn drain(&mut self) -> Vec<(usize, u64)> {
let journal = self.journal.drain().collect::<Vec<usize>>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for a temporary vector here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not a simple change since drain holds &mut ref to self
but i will try something

Copy link
Collaborator

Choose a reason for hiding this comment

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

let journal = mem::replace(&self.journal, HashMap::new()).into_iter()

Copy link
Contributor

Choose a reason for hiding this comment

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

or explicitly borrow the other field of self outside of the closure so the closure doesn't capture self.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

journal.iter().map(|idx| (*idx, self.elems[*idx])).collect::<Vec<(usize, u64)>>()
}

pub fn how_full(&self) -> f64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be called "saturation" or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


/// Initializes bloom filter from saved state
pub fn from_parts(parts: &[u64], k_num: u32) -> Bloom {
let bitmap_size = parts.len()*8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

spacing around '*'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@arkpar arkpar added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 30, 2016
@rphmeier rphmeier added the M4-core ⛓ Core client code / Rust. label Sep 30, 2016
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 30, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1863049 on bloom-crate into * on master*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1863049 on bloom-crate into * on master*.

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 30, 2016
@NikVolf NikVolf merged commit 1d3e242 into master Sep 30, 2016
@arkpar arkpar deleted the bloom-crate branch October 3, 2016 15:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants