aherbert commented on code in PR #397:
URL:
https://github.com/apache/commons-collections/pull/397#discussion_r1226375910
##########
src/main/java/org/apache/commons/collections4/bloomfilter/EnhancedDoubleHasher.java:
##########
@@ -132,7 +132,7 @@ long getIncrement() {
/**
* Performs a modulus calculation on an unsigned long and an integer
divisor.
* @param dividend a unsigned long value to calculate the modulus of.
- * @param divisor the divisor for the modulus calculation.
+ * @param divisor the divisor for the modulus calculation, must be
positive.
Review Comment:
Also not related to COLLECTIONS-841 but trivial. I have updated master in
commit 1d07ca40667849742d8712bf55770c559cd6203d
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java:
##########
@@ -424,11 +424,11 @@ public void testBitMapProducerSize() {
/**
* Testing class returns the value as the only value.
*/
- class BadHasher implements Hasher {
+ public static class BadHasher implements Hasher {
Review Comment:
Why public? If this must be public I assume you are using it in a different
package which currently does not exist. Thus the change seems unjustified at
present.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java:
##########
@@ -218,7 +218,7 @@ public void testClear() {
* Tests that the estimated intersection calculations are correct.
*/
@Test
- public final void testEstimateIntersection() {
+ public void testEstimateIntersection() {
Review Comment:
If these must be non-final due to a requirement to change the implementation
then this means that a new implementation of the `BloomFilter` interface will
not function the same as what we currently have. Without a new class as such a
use case it seems wrong to make this change.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java:
##########
@@ -247,10 +247,10 @@ public final void testEstimateIntersection() {
}
/**
- * Tests that the andCardinality calculations are correct.
+ * Tests that the estimateUnion calculations are correct.
Review Comment:
Good spot. I updated master in 1d07ca40667849742d8712bf55770c559cd6203d
##########
src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilter.java:
##########
@@ -289,12 +289,12 @@ default int estimateIntersection(final BloomFilter other)
{
// if both are infinite the union is infinite and we return
Integer.MAX_VALUE
return Integer.MAX_VALUE;
}
- long estimate;
Review Comment:
This change is unrelated to COLLECTIONS-841.
I note that `Long.numberOfLeadingZeros(46144189292 * 2) == 27` so the change
to a double with 53-bits is OK. Do you have a test that shows the estimate
being negative? Is this due to almost saturation of the filter and estimateN
being close to infinity (since infinity throws)?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
