I am refactoring some of the code to make the Builder and Hash classes enclosed in the ProtoBloomFilter class. I have also add the proxy serializer as noted in the effective java talk. This simplifies the classes significantly. I will have unit tests and fix the javadocs before the next push.
Claude On Mon, Sep 16, 2019 at 2:01 AM Gary Gregory <garydgreg...@gmail.com> wrote: > On Sun, Sep 15, 2019 at 8:17 PM sebb <seb...@gmail.com> wrote: > > > On Mon, 16 Sep 2019 at 00:17, Gilles Sadowski <gillese...@gmail.com> > > wrote: > > > > > > Hi. > > > > > > Le sam. 14 sept. 2019 à 08:15, Claude Warren <cla...@xenei.com> a > écrit > > : > > > > > > > > @Gilles > > > > > > > > I am happy to rename the package without the plural if that is the > > > > standard, I will also fix the indent issue. Is there a definition > > that can > > > > be quickly imported into Eclipse to do the proper formatting? > > > > > > Hopefully, someone else can answer that one. > > > > > > > I am adding/updating all comments in the code. > > > > > > > > FilterConfig contains a main method to make it easy to assess how the > > > > various settings influence the length and number of functions in the > > > > filter. This could go into another class or a Utility class but > > neither of > > > > those seem like a better solution. > > > > > > I don't think there should be a "main" method in a library. > > > Also, just printing to stdout does not seem very flexible. > > > Perhaps define a "toString()" method? > > > > > > > I agree with your assessment of final and immutable for methods, > > variables > > > > and classes. > > > > > > > > I am removing the FilterConfig constructor for BloomFilter and > > ensuring the > > > > BloomFilter makes a defensive copy. > > > > > > > > The difference between match() and equals() is that equals() is a bit > > > > vector equality and match is the standard bloom filter comparison T&C > > = T. > > > > Documentation is being added to the match() and inverseMatch() to > > explain > > > > this. > > > > > > > > STORED_SIZE is similar to Long.BYTES (perhaps it should be renamed). > > It > > > > identifies how many bytes the FilterConfig will use when written to a > > > > DataOuptut or read from a DataInput. > > > > > > Why is it necessary to know such internal details? > > > Anyways, I think that persistence is best left to higher-level code. > > > > > > > > > > > I think the Utils class can be removed once the other code is moved > to > > > > Java8 streams and not extended iterators. > > > > > > > > Hash function is changed to Objects.hash() > > > > > > > > Constructors are/will be private where factories make sense. For > some > > the > > > > simple constructor makes more sense. (e.g. BloomFilter(BitSet) ) > > > > > > See > > > > https://www.baeldung.com/java-constructors-vs-static-factory-methods > > > and > > > https://blog.joda.org/2014/03/valjos-value-java-objects.html > > > > > > > I have no problem removing the Serializable on FilterConfig, but can > > you > > > > explain (or point to a post) why it should be avoided? > > > > > > Same as above (not prejudging how application want to handle > > serialization). > > > > It's difficult to code Serializable classes properly, and just as hard > > to maintain them. > > Also the serialised format becomes part of the API making changes harder. > > > > We should not promise what we likely cannot continue to deliver, > > especially since it may not be needed. > > > > Serializable classes are also a target for code exploits. > > > > +1 to removing Serializable support in its current implementation. If > serialization is a requirement, then it should be implemented using a > Serialization Proxy a la J. Block: > > https://www.youtube.com/watch?v=V1vQf4qyMXg&feature=youtu.be&t=56m12s > > Gary > > > > > > The update/build methods are being documented and an explanation of > the > > > > difference included. > > > > > > Am I wrong that the "build(...)" methods only spare the typing of 2 > > > characters? > > > If so, it's not worth the increase of the API size. > > > > > > > > > > > getLog() is also being documented > > > > > > > > The asBitSet() method was to extract the bitset from the bloom filter > > so > > > > that it could be stored otherwise used with BitSet based processes. > > The > > > > name of the method is being chagned to getBitSet() and it makes a > > defensive > > > > copy. > > > > > > > > The ProtoBloomFilter is not a factory. It is really a bean and there > > are > > > > times when it makes sense to use the ProtoBloomFilter. The name is > > > > important as it identifies what it is. It contains the necessary > > > > information to build any configuration of BloomFilter. There is a > > proposal > > > > to use ProtoBloomFilters in service calls. It could be defined as an > > > > interface with a concrete implementation in the > > ProtoBloomFilterBuilder. > > > > > > > > As an example of ProtoBloomFilter usage. Say you have a Person > object > > that > > > > has the user's first name, last name, eye color, hair color. > > > > > > > > The ProtoBloomFilterBuilder would accept the 4 values as 4 calls to > > > > update() followed by a build() to build the protoBloomFilter. > > > > ProtoBloomFilter proto = ne wProtoBloomFilterBuilder().update( > "Claude" > > > > ).update( "Warren" ).update( "brown" ).update( "silver" ).build(); > > > > > > I'm still missing why it cannot be a factory, but maybe it is a > > > question of design habits. What about the following: > > > > > > public class BloomFilter { > > > private final BitSet bitSet; > > > > > > private BloomFilter(BitSet bs) { > > > bitSet = (BitSet) (bs.clone()); > > > } > > > > > > /** Factory method. (cf. "ValJO") */ > > > public BloomFilter of(BitSet bs) { > > > return new BloomFilter(bs); > > > } > > > > > > // Combined "ProtoBloomFilter" and "ProtoBloomFilterBuilder" logic. > > > public static class Builder { > > > private final Set<Hash> hashes = new HashSet<>(); > > > > > > private Builder() {} > > > > > > /** Factory method(s). */ > > > public Builder with(Builder other) { > > > hashes.addAll(other.hashes); > > > } > > > public Builder with(ByteBuffer b) { /* ... */ } > > > // etc. > > > > > > /** Factory (converter). */ > > > public BloomFilter create(FilterConfig cfg) { > > > final BitSet set = new BitSet(cfg.getNumberOfBits()); > > > for (Hash hash : hashes) { > > > hash.populate(set, cfg); > > > } > > > return BloomFilter.of(set); > > > } > > > } > > > } > > > > > > Perhaps I'm taking too many shortcuts... But, as pointed out by > > > Gary, more coverage is required; and unit tests will help figure out > > > the leanest design. > > > > > > > > > > > I can create a bloom filter configuration that has 1x10^7 items and > > > > accepts 1 / 1x10^6 collisions. > > > > FilterConfig config1 = new FilterConfig( 10000000, 1000000) > > > > > > > > and build the BloomFilter > > > > BloomFilter filter1 = proto.create( config1 ) > > > > > > > > I can use that filter to determine if the person is in a collection > of > > > > 1x10^7 items. The implementation of that collection could be > multiple > > > > buckets of 1x10^4 items with a collision rate of 5x10^3 items. To > > check > > > > that I create an appropriate FilterConfig > > > > FilterConfig config2 = new FilterConfig( 10000, 5000 ) > > > > > > > > and then create the proper bloom filter > > > > BloomFilter filter2 = proto.create( config2 ) > > > > > > > > > > With my above attempt at an alternative API, we'd have (untested): > > > > > > BloomFilter.Builder proto = BloomFilter > > > .with("Claude") > > > .with("Warren") > > > .with("brown"). > > > .with("silver"); > > > > > > BloomFilter filter1 = proto.create(config1); > > > BloomFilter filter2 = proto.create(config2); > > > > > > > > > IIUC, in your current implementation of "ProtoBloomFilter", there > > > is a side-effect of "build()" (i.e. the call to "clear()"); thus > calling > > > "build()" twice will not yield the same result. I don't think that's > > > a desirable feature. > > > > > > Regards, > > > Gilles > > > > > > > Claude > > > > > > > > > > > > On Fri, Sep 13, 2019 at 2:22 PM Gilles Sadowski < > gillese...@gmail.com> > > > > wrote: > > > > > > > > > > > [...] > > > > > > > > > > > > Gilles, > > > > > > > > > > > > Take a look at the PR, I added comments to allow the PR to have 0 > > deps. > > > > > > > > > > > > Gary > > > > > > > > > > > > > > > > > > > > > > IMO, the package should be named "bloomfilter" (without "s"). > > > > > Naming seems inconsistent in [Collections]: Some (package) > > > > > names are singular, other plural. > > > > > > > > > > * Indent must be 4 spaces. > > > > > * All fields and methods must be commented. > > > > > * "FilterConfig" contains a "main" method. > > > > > * Local variables and fields must be declared "final" whenever > > constant. > > > > > * Some methods are declared "final", others not. > > > > > * "BloomFilter" should be made immutable. > > > > > * "BloomFilter" could do without a second constructor: > "FilterConfig" > > > > > should have a method that returns a "BitSet". > > > > > * "BloomFilter" must make a defensive copy of its "bitSet" > argument. > > > > > * What's the difference between "match" and "equals"? > > > > > * Why is "STORED_SIZE" public? > > > > > * "Utils" classes should be avoided. > > > > > * Hash function should be imported/shaded from "Commons Codec" > > > > > (and defined there if missing). > > > > > * Constructor(s) should be private (instantiation through factory > > > > > methods (cf. ValJO). > > > > > * "Serializable" should be avoided. > > > > > * The "update" methods are missing explanations (or reference) > about > > > > > their purpose. Also, it seems that "update" and "build" are > > redundant. > > > > > * Does "getLog" return a standard value? If so, the reference > should > > > > > appear in the Javadoc (not just as code comment). > > > > > * What is the purpose of method "asBitSet()"? > > > > > * (IIUC) "ProtoBloomFilter" should be renamed "BloomFilterFactory". > > > > > "ProtoBloomFilterBuilder" should be a static inner class of that > > > > > factory. > > > > > > > > > > Regards, > > > > > Gilles > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > -- I like: Like Like - The likeliest place on the web <http://like-like.xenei.com> LinkedIn: http://www.linkedin.com/in/claudewarren
