aherbert commented on a change in pull request #160:
URL:
https://github.com/apache/commons-collections/pull/160#discussion_r425402326
##########
File path:
src/test/java/org/apache/commons/collections4/bloomfilter/hasher/HasherBuilderTest.java
##########
@@ -76,16 +79,15 @@ public void withCharSequenceTest() {
public void withUnecodedCharSequenceTest() {
final String ascii = "plain";
final String extended = getExtendedString();
- for (final String s : new String[] {ascii, extended}) {
+ for (final String s : new String[] { ascii, extended }) {
final TestBuilder builder = new TestBuilder();
builder.withUnencoded(s);
final byte[] encoded = builder.items.get(0);
final char[] original = s.toCharArray();
// Should be twice the length
Assert.assertEquals(original.length * 2, encoded.length);
// Should be little endian (lower bits first)
- final CharBuffer buffer = ByteBuffer.wrap(encoded)
-
.order(ByteOrder.LITTLE_ENDIAN).asCharBuffer();
+ final CharBuffer buffer =
ByteBuffer.wrap(encoded).order(ByteOrder.LITTLE_ENDIAN).asCharBuffer();
Review comment:
Introduced new formatting
##########
File path:
src/test/java/org/apache/commons/collections4/bloomfilter/hasher/HasherBuilderTest.java
##########
@@ -29,7 +32,8 @@
/**
* Tests the
- * {@link org.apache.commons.collections4.bloomfilter.hasher.Hasher.Builder
Hasher.Builder}.
+ * {@link org.apache.commons.collections4.bloomfilter.hasher.Hasher.Builder
+ * Hasher.Builder}.
Review comment:
Introduced new formatting
##########
File path:
src/main/java/org/apache/commons/collections4/bloomfilter/BitSetBloomFilter.java
##########
@@ -25,7 +25,7 @@
import org.apache.commons.collections4.bloomfilter.hasher.StaticHasher;
/**
- * A bloom filter using a Java BitSet to track enabled bits. This is a standard
+ * A bloom filter uses a Java BitSet to track enabled bits. This is a standard
Review comment:
I do not agree this is the correct grammar:
`"A bloom filter using a Java BitSet"`: _This_ implementation of a Bloom
filter is using a Java BitSet ...
`"A bloom filter uses a Java BitSet"`: _Any_ implementation of a Bloom
filter uses a Java BitSet ...
##########
File path:
src/main/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilter.java
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.collections4.bloomfilter;
+
+import java.util.function.Function;
+
+import org.apache.commons.collections4.bloomfilter.hasher.DynamicHasher;
+import org.apache.commons.collections4.bloomfilter.hasher.Hasher;
+import org.apache.commons.collections4.bloomfilter.hasher.Shape;
+import
org.apache.commons.collections4.bloomfilter.hasher.function.Murmur128x64Cyclic;
+
+/**
+ * A bloom filter that uses a BitSetBloomFilter to create a Bloom filter that
+ * can merge instances of a specific class.
+ *
+ * @param <T> The Class to merge.
+ * @since 4.5
+ */
+public class SimpleBloomFilter<T> extends BitSetBloomFilter implements
BloomFilter {
+
+ /**
+ * The function that converts the instance of T to the SimpleBuilder.
+ * <p>
+ * If the object T is to be considered as a single item in the filter then
+ * function must create the {@code SimpleBuilder} and only call a single
{@code with()}
+ * method.</p>
+ * <p>
+ * If the object T is to be considered as several items then the function
must
+ * create the {@code SimpleBuilder} and call the {@code with()} method
once for each item.</p>
+ */
+ private Function<T, SimpleBuilder> func;
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape and function. This
constructor
+ * creates an empty Bloom filter.
+ * @param hasher the Hasher to use.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @see #func
+ */
+ public SimpleBloomFilter(Hasher hasher, Shape shape, Function<T,
SimpleBuilder> func) {
+ super(hasher, shape);
+ this.func = func;
+ }
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape and function. This
constructor
+ * creates an empty Bloom filter.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @see #func
+ */
+ public SimpleBloomFilter(Shape shape, Function<T, SimpleBuilder> func) {
+ super(shape);
+ this.func = func;
+ }
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape, function and a data
object.
+ * This constructor creates an Bloom filter populated with the data from
the
+ * {@code data} parameter.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @param data the data object to populate the filter with.
+ * @see #func
Review comment:
Just ran this locally on Java 8: `mvn javadoc:javadoc`
When browsing the output the link for `func` does nothing. So it appears
even though you can build the javadoc this will not generate documentation from
the private field.
Doing the same on Java 14 and the `func` listed under `See Also:` is not a
link. It may be that this should be a javadoc error but the maven plugin is not
set up to flag it, or it may just not be reported as an error.
Currently the intention to bring the documentation from the private field is
not being realised. So this class should be rewritten to move this javadoc
somewhere public.
##########
File path:
src/main/java/org/apache/commons/collections4/bloomfilter/hasher/Hasher.java
##########
@@ -103,6 +108,95 @@ default Builder withUnencoded(CharSequence item) {
}
return with(bytes);
}
+
+ /**
+ * Adds an integer into the hasher. The integer is converted into 4
bytes
+ * through the use of ByteBuffer.putInt.
+ * @param data the integer to add.
+ * @return a reference to this object
+ * @see ByteBuffer#putInt
+ */
+ default B with(int data) {
+ return with( ByteBuffer.allocate(Integer.BYTES).putInt( data
).array());
+ }
+
+ /**
+ * Adds a long into the hasher. The long is converted into 8 bytes
+ * through the use of ByteBuffer.putLong.
+ * @param data the long to add.
+ * @return a reference to this object
+ * @see ByteBuffer#putLong
+ */
+ default B with(long data) {
+ return with( ByteBuffer.allocate(Long.BYTES).putLong( data
).array());
+ }
+
+ /**
+ * Adds a double into the hasher. The double is converted into 8 bytes
+ * through the use of ByteBuffer.putDouble.
+ * @param data the double to add.
+ * @return a reference to this object
+ * @see ByteBuffer#putDouble
+ */
+ default B with(double data) {
+ return with( ByteBuffer.allocate(Double.BYTES).putDouble( data
).array());
Review comment:
Here I would use:
```java
return with(Double.doubleToRawLongBits(data));
```
same for float.
##########
File path:
src/main/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilter.java
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.collections4.bloomfilter;
+
+import java.util.function.Function;
+
+import org.apache.commons.collections4.bloomfilter.hasher.DynamicHasher;
+import org.apache.commons.collections4.bloomfilter.hasher.Hasher;
+import org.apache.commons.collections4.bloomfilter.hasher.Shape;
+import
org.apache.commons.collections4.bloomfilter.hasher.function.Murmur128x64Cyclic;
+
+/**
+ * A bloom filter that uses a BitSetBloomFilter to create a Bloom filter that
+ * can merge instances of a specific class.
+ *
+ * @param <T> The Class to merge.
+ * @since 4.5
+ */
+public class SimpleBloomFilter<T> extends BitSetBloomFilter implements
BloomFilter {
+
+ /**
+ * The function that converts the instance of T to the SimpleBuilder.
+ * <p>
+ * If the object T is to be considered as a single item in the filter then
+ * function must create the {@code SimpleBuilder} and only call a single
{@code with()}
+ * method.</p>
+ * <p>
+ * If the object T is to be considered as several items then the function
must
+ * create the {@code SimpleBuilder} and call the {@code with()} method
once for each item.</p>
+ */
+ private Function<T, SimpleBuilder> func;
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape and function. This
constructor
+ * creates an empty Bloom filter.
+ * @param hasher the Hasher to use.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @see #func
+ */
+ public SimpleBloomFilter(Hasher hasher, Shape shape, Function<T,
SimpleBuilder> func) {
+ super(hasher, shape);
+ this.func = func;
+ }
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape and function. This
constructor
+ * creates an empty Bloom filter.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @see #func
+ */
+ public SimpleBloomFilter(Shape shape, Function<T, SimpleBuilder> func) {
+ super(shape);
+ this.func = func;
+ }
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape, function and a data
object.
+ * This constructor creates an Bloom filter populated with the data from
the
+ * {@code data} parameter.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @param data the data object to populate the filter with.
+ * @see #func
+ */
+ public SimpleBloomFilter(Shape shape, Function<T, SimpleBuilder> func, T
data) {
+ this(shape, func);
+ this.merge( data );
Review comment:
As before, creating a filter and adding data are two separate functions.
E.g. there is no constructor for an ArrayList that accepts a single item as
well as the capacity.
This concept is better removed or changed to create a filter with another
Bloom filter (i.e. a collection). Here this highlights the dual functionality
of the Shape which specifies not only the number of hash functions per item but
also contains the number of expected items which is not strictly necessary for
the filter to function. Creating from another Bloom filter can copy the other
filter shape (indeed it mush match hash function identity and number of hash
functions) but what would be the expected number of items?
##########
File path:
src/main/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilter.java
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.collections4.bloomfilter;
+
+import java.util.function.Function;
+
+import org.apache.commons.collections4.bloomfilter.hasher.DynamicHasher;
+import org.apache.commons.collections4.bloomfilter.hasher.Hasher;
+import org.apache.commons.collections4.bloomfilter.hasher.Shape;
+import
org.apache.commons.collections4.bloomfilter.hasher.function.Murmur128x64Cyclic;
+
+/**
+ * A bloom filter that uses a BitSetBloomFilter to create a Bloom filter that
+ * can merge instances of a specific class.
+ *
+ * @param <T> The Class to merge.
+ * @since 4.5
+ */
+public class SimpleBloomFilter<T> extends BitSetBloomFilter implements
BloomFilter {
+
+ /**
+ * The function that converts the instance of T to the SimpleBuilder.
+ * <p>
+ * If the object T is to be considered as a single item in the filter then
+ * function must create the {@code SimpleBuilder} and only call a single
{@code with()}
+ * method.</p>
+ * <p>
+ * If the object T is to be considered as several items then the function
must
+ * create the {@code SimpleBuilder} and call the {@code with()} method
once for each item.</p>
+ */
+ private Function<T, SimpleBuilder> func;
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape and function. This
constructor
+ * creates an empty Bloom filter.
Review comment:
This is not an empty filter: it has a hasher argument.
I would drop this constructor. Creating a Bloom filter and adding data are
two separate functions.
##########
File path:
src/main/java/org/apache/commons/collections4/bloomfilter/hasher/Hasher.java
##########
@@ -103,6 +108,95 @@ default Builder withUnencoded(CharSequence item) {
}
return with(bytes);
}
+
+ /**
+ * Adds an integer into the hasher. The integer is converted into 4
bytes
+ * through the use of ByteBuffer.putInt.
+ * @param data the integer to add.
+ * @return a reference to this object
+ * @see ByteBuffer#putInt
+ */
+ default B with(int data) {
+ return with( ByteBuffer.allocate(Integer.BYTES).putInt( data
).array());
Review comment:
If you read the code behind `ByteBuffer.allocate` there is a lot of
overhead to these calls. If this Builder is going to be efficient then I would
suggest writing the methods explicitly:
```java
// little-endian
return with(new byte[]{(byte) (data ),
(byte) (data >> 8),
(byte) (data >> 16),
(byte) (data >> 24)});
```
##########
File path:
src/main/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilter.java
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.collections4.bloomfilter;
+
+import java.util.function.Function;
+
+import org.apache.commons.collections4.bloomfilter.hasher.DynamicHasher;
+import org.apache.commons.collections4.bloomfilter.hasher.Hasher;
+import org.apache.commons.collections4.bloomfilter.hasher.Shape;
+import
org.apache.commons.collections4.bloomfilter.hasher.function.Murmur128x64Cyclic;
+
+/**
+ * A bloom filter that uses a BitSetBloomFilter to create a Bloom filter that
+ * can merge instances of a specific class.
+ *
+ * @param <T> The Class to merge.
+ * @since 4.5
+ */
+public class SimpleBloomFilter<T> extends BitSetBloomFilter implements
BloomFilter {
+
+ /**
+ * The function that converts the instance of T to the SimpleBuilder.
+ * <p>
+ * If the object T is to be considered as a single item in the filter then
+ * function must create the {@code SimpleBuilder} and only call a single
{@code with()}
+ * method.</p>
+ * <p>
+ * If the object T is to be considered as several items then the function
must
+ * create the {@code SimpleBuilder} and call the {@code with()} method
once for each item.</p>
+ */
+ private Function<T, SimpleBuilder> func;
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape and function. This
constructor
+ * creates an empty Bloom filter.
+ * @param hasher the Hasher to use.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @see #func
+ */
+ public SimpleBloomFilter(Hasher hasher, Shape shape, Function<T,
SimpleBuilder> func) {
+ super(hasher, shape);
+ this.func = func;
+ }
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape and function. This
constructor
+ * creates an empty Bloom filter.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @see #func
+ */
+ public SimpleBloomFilter(Shape shape, Function<T, SimpleBuilder> func) {
+ super(shape);
+ this.func = func;
+ }
+
+ /**
+ * Constructs a SimpleBloomFilter from the shape, function and a data
object.
+ * This constructor creates an Bloom filter populated with the data from
the
+ * {@code data} parameter.
+ * @param shape the Shape of the Bloom filter.
+ * @param func a Function to convert T to a SimpleBuilder.
+ * @param data the data object to populate the filter with.
+ * @see #func
+ */
+ public SimpleBloomFilter(Shape shape, Function<T, SimpleBuilder> func, T
data) {
+ this(shape, func);
+ this.merge( data );
+ }
+
+ /**
+ * Merges a data object into this filter.
+ *
+ * <p>Note: This method should return {@code true} even if no additional
bit indexes were
+ * enabled. A {@code false} result indicates that this filter is not
ensured to contain
+ * the {@code data}.
+ *
+ * @param data the data to merge.
+ * @return true if the merge was successful
+ * @throws IllegalArgumentException if the shape of the other filter does
not match
+ * the shape of this filter
+ */
+ public boolean merge( T data ) {
+ return this.merge( this.func.apply( data ).build() );
+ }
+
+
+ /**
+ * Returns {@code true} if this filter contains the object.
+ * Specifically this returns {@code true} if this filter is enabled for
all bit indexes
+ * identified by the hashing of the object {@code data}.
+ *
+ * @param data the data to check for.
+ * @return true if this filter is enabled for all bits specified by the
data object.
+ */
+ public boolean contains( T data ) {
+ return this.contains( this.func.apply( data ).build() );
+ }
+
+ /**
+ * A Hasher.Builder for the SimpleBloom filter.
+ * This builder uses the Murmur 128 x64 cyclic hash.
+ *
+ * @see Murmur128x64Cyclic
+ */
+ public static class SimpleBuilder extends DynamicHasher.Builder {
+
+ public SimpleBuilder() {
Review comment:
public class should have a constructor comment
##########
File path:
src/test/java/org/apache/commons/collections4/bloomfilter/hasher/HasherBuilderTest.java
##########
@@ -58,10 +62,9 @@ public Builder with(byte[] item) {
public void withCharSequenceTest() {
final String ascii = "plain";
final String extended = getExtendedString();
- for (final String s : new String[] {ascii, extended}) {
- for (final Charset cs : new Charset[] {
- StandardCharsets.ISO_8859_1, StandardCharsets.UTF_8,
StandardCharsets.UTF_16
- }) {
+ for (final String s : new String[] { ascii, extended }) {
+ for (final Charset cs : new Charset[] {
StandardCharsets.ISO_8859_1, StandardCharsets.UTF_8,
+ StandardCharsets.UTF_16 }) {
TestBuilder builder = new TestBuilder();
Review comment:
Introduced new formatting
##########
File path:
src/test/java/org/apache/commons/collections4/bloomfilter/hasher/HasherBuilderTest.java
##########
@@ -76,16 +79,15 @@ public void withCharSequenceTest() {
public void withUnecodedCharSequenceTest() {
final String ascii = "plain";
final String extended = getExtendedString();
- for (final String s : new String[] {ascii, extended}) {
+ for (final String s : new String[] { ascii, extended }) {
Review comment:
Introduced new formatting
##########
File path:
src/test/java/org/apache/commons/collections4/bloomfilter/hasher/HasherBuilderTest.java
##########
@@ -98,7 +100,7 @@ public void withUnecodedCharSequenceTest() {
* @return the extended string
*/
static String getExtendedString() {
- final char[] data = {'e', 'x', 't', 'e', 'n', 'd', 'e', 'd', ' ',
+ final char[] data = { 'e', 'x', 't', 'e', 'n', 'd', 'e', 'd', ' ',
Review comment:
Introduced new formatting
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
