You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Claudenw (via GitHub)" <gi...@apache.org> on 2023/06/12 09:18:56 UTC
[GitHub] [commons-collections] Claudenw opened a new pull request, #397: open up Bloom filter tests
Claudenw opened a new pull request, #397:
URL: https://github.com/apache/commons-collections/pull/397
Removed final and converted some methods to public. Also fixed issue in Bloom filter estimate of intersection for unusual cases.
Implements Collections 841
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] aherbert commented on a diff in pull request #397: open up Bloom filter tests
Posted by "aherbert (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw commented on a diff in pull request #397: open up Bloom filter tests
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #397:
URL: https://github.com/apache/commons-collections/pull/397#discussion_r1227011235
##########
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:
>
I am working on a stable Bloom filter implementation -- I think I have it working but I have a lot more checking to do. The implementation and definition is based on http://webdocs.cs.ualberta.ca/~drafiei/papers/DupDet06Sigmod.pdf
The stable Bloom filter is intended to be used in a streaming environment so it disables some random bits before each item is added. The upshot is that when the system attempts to turn on all the bits in the filter it will most likely fail. So some tests do not function as for the normal Bloom filter.
If you are interested in looking at the code it can be found in https://github.com/Claudenw/IdSupplier
I have proposed that Kafka use a stable Bloom filter to resolve one of the issues they are working on.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw commented on pull request #397: open up Bloom filter tests
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #397:
URL: https://github.com/apache/commons-collections/pull/397#issuecomment-1591415666
Closing pull request for cleaner changes in a different branch
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw closed pull request #397: open up Bloom filter tests
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw closed pull request #397: open up Bloom filter tests
URL: https://github.com/apache/commons-collections/pull/397
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] codecov-commenter commented on pull request #397: open up Bloom filter tests
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #397:
URL: https://github.com/apache/commons-collections/pull/397#issuecomment-1586941943
## [Codecov](https://app.codecov.io/gh/apache/commons-collections/pull/397?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#397](https://app.codecov.io/gh/apache/commons-collections/pull/397?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9bb2730) into [master](https://app.codecov.io/gh/apache/commons-collections/commit/98e2fc9683819df667abaab3ce74939ec2e08c33?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (98e2fc9) will **decrease** coverage by `0.01%`.
> The diff coverage is `83.33%`.
```diff
@@ Coverage Diff @@
## master #397 +/- ##
============================================
- Coverage 81.19% 81.18% -0.01%
Complexity 4604 4604
============================================
Files 288 288
Lines 13423 13423
Branches 1982 1982
============================================
- Hits 10899 10898 -1
Misses 1932 1932
- Partials 592 593 +1
```
| [Impacted Files](https://app.codecov.io/gh/apache/commons-collections/pull/397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../commons/collections4/bloomfilter/BloomFilter.java](https://app.codecov.io/gh/apache/commons-collections/pull/397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29sbGVjdGlvbnM0L2Jsb29tZmlsdGVyL0Jsb29tRmlsdGVyLmphdmE=) | `97.29% <75.00%> (-2.71%)` | :arrow_down: |
| [...collections4/bloomfilter/EnhancedDoubleHasher.java](https://app.codecov.io/gh/apache/commons-collections/pull/397?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29sbGVjdGlvbnM0L2Jsb29tZmlsdGVyL0VuaGFuY2VkRG91YmxlSGFzaGVyLmphdmE=) | `100.00% <100.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org