[COLLECTIONS-843] Implement Layered Bloom filter #402

Claudenw · 2023-06-29T18:52:12Z

Implementation to fix COLLECTIONS-843.

This is a simple Layered Bloom filter implementation with complex configuration options.

A static method to create a fixed level implementation that does not merge is provided.
An example of a complex time based Bloom filter is provided in the testing code where filters are added to until they are saturated or one second has elapsed and are removed after they are 4 seconds old.

this change is in support of Kafka KIP-936
https://cwiki.apache.org/confluence/display/KAFKA/KIP-936%3A+Throttle+number+of+active+PIDs

automatic decay.

codecov-commenter · 2023-06-29T18:55:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1df5606) 81.29% compared to head (500a252) 81.57%.
Report is 24 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #402      +/-   ##
============================================
+ Coverage     81.29%   81.57%   +0.28%     
- Complexity     4640     4742     +102     
============================================
  Files           290      295       +5     
  Lines         13545    13751     +206     
  Branches       2003     2022      +19     
============================================
+ Hits          11011    11217     +206     
  Misses         1933     1933              
  Partials        601      601

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

garydgregory · 2023-06-30T14:01:22Z

Hi @Claudenw
I took the liberty to fix the Javadocs that were failing the build. Make sure you pull this branch before editing.

PS: You can detect all failures before pushing by running the default Maven goal on the command line with a simple mvn invocation ;-)

Claudenw · 2023-06-30T17:31:33Z

mvn doesn't catch all the issues :( I just spend a number of hours working out new tests for coverage changes.

…ons into layered_filter

Claudenw · 2023-09-03T11:16:44Z

@garydgregory @aherbert

Gentlemen,

Can I get eyes on the new code please. I think it is correct and fixes all the previously identified issues.

Claudenw · 2023-09-26T06:38:58Z

@garydgregory @aherbert nudge

garydgregory

OK here. @aherbert ?

aherbert

Noted a few changes. I did not look at the test code. I will do so in the next few days.

src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilter.java

src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilterProducer.java

src/main/java/org/apache/commons/collections4/bloomfilter/LayerManager.java

src/main/java/org/apache/commons/collections4/bloomfilter/Shape.java

Claudenw · 2023-10-01T07:36:12Z

@aherbert thank you for your review. Changes have been made to the base code. One of these days I will figure out how you see all those things that I seem to miss.

…ons into layered_filter

garydgregory · 2023-10-01T13:28:31Z

Let's make sure we let GitHub squash the commits once this is approved.

Claudenw · 2023-10-08T17:23:24Z

@aherbert Have you had a chance to look at the test code? I have a series of blog posts about Bloom filters coming out "soon" (depending on when my company gets around to finalizing it). The second one in the series talks about commons-collections Bloom filters. I am hoping we have this in the 4.5-SNAPSHOT before then.

Thanks to you and @garydgregory for all your assistance in making the entire package a better submission.

aherbert

@Claudenw There are some minor things to clean up here but no major issues. Can you check the LayerManagerTest for testAdvanceOnCalculatedFull and testAdvanceOnSaturation are using the correct maxN.

src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterProducerTest.java

aherbert · 2023-10-20T18:48:47Z

src/test/java/org/apache/commons/collections4/bloomfilter/CountingPredicateTest.java

+        assertTrue(cp.forEachRemaining());
+
+        // test last item not checked
+        cp = new CountingPredicate<>(ary, (x, y) -> y == Integer.valueOf(2));


I think this is not testing what you want. You never call cp.test. So when you call cp.forEachRemaining your predicate will be called with (1, null) and then (2, null). You should check for this. But you are checking y == 2 which is not targeting any functionality of the CountingPredicate. A similar argument for the test below where you check y == 1. Since y will be null in forEachRemaining this is not testing a completion of the forEachRemaining loop.

You should add a test case that when you call cp.test(Z) that the constructor BiPredicate receives (1, Z) then (2, Z).

aherbert · 2023-10-20T18:51:56Z

src/test/java/org/apache/commons/collections4/bloomfilter/DefaultBloomFilterTest.java


-        SparseDefaultBloomFilter(final Shape shape) {
+        public SparseDefaultBloomFilter(final Shape shape) {


These do not need to be public since they are used only in the package. If I revert this change the test suite still functions.

Are you using them externally by importing collections test-jar?

Yes, that is exactly the usage. In testing a new BloomFilter implementation is is handy to have a clean implementation of a sparse filter as used in the testing to verify correct functioning of the new Bloom filter.

aherbert · 2023-10-20T18:52:05Z

src/test/java/org/apache/commons/collections4/bloomfilter/DefaultBloomFilterTest.java

+    /**
+     * A default implementation of a non-sparse Bloom filter.
+     */
+    public static class NonSparseDefaultBloomFilter extends AbstractDefaultBloomFilter {


No need to be public

same as above

src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java

src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java

aherbert · 2023-10-20T19:32:12Z

src/test/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilterTest.java

@@ -41,4 +41,16 @@ public void testMergeShortBitMapProducer() {
        assertTrue(filter.merge(producer));
        assertEquals(1, filter.cardinality());
    }
+
+    @Test
+    public void testCardinalityAndIsEmpty() {


This is just doing what the parent class does. The extra test should be in its own fixture, e.g.

@Test public void testEmptyAfterMergeWithNothing() { // test the case where is empty after merge // in this case the internal cardinality == -1 BloomFilter bf = createEmptyFilter(getTestShape()); bf.merge(IndexProducer.fromIndexArray()); assertTrue(bf.isEmpty()); }

Added as fixture in abstract test class.

src/test/java/org/apache/commons/collections4/bloomfilter/WrappedBloomFilterTest.java

garydgregory · 2023-10-21T14:44:30Z

Hi All. Is this PR ready to be merged?

aherbert · 2023-10-21T15:40:00Z

There are some changes to make to the test code. I only reviewed this yesterday.

garydgregory · 2023-10-21T15:44:41Z

There are some changes to make to the test code. I only reviewed this yesterday.

👍 TY for the update

Claudenw · 2023-11-24T15:11:35Z

@aherbert just a friendly ping. I'd like to get this closed.

aherbert

Only reviewed the changes since the last review. It looks OK. I noted a change to make the test able to support concurrent test execution.

aherbert · 2023-11-24T22:21:46Z

src/test/java/org/apache/commons/collections4/bloomfilter/CountingPredicateTest.java

 import org.junit.jupiter.api.Test;

 public class CountingPredicateTest {
+    private List<Pair<Integer, Integer>> expected = new ArrayList<>();
+    private List<Pair<Integer, Integer>> result = new ArrayList<>();


This could be an argument to makeFunc. The expected can be created for each test that uses it.

Currently using a class level object works as JUnit is only executing one test at a time on an instance. But it is cleaner and more thread safe to pass in result to the makeFunc helper (which can be static).

@aherbert, thanks for the suggestion. I made the changes.

aherbert · 2023-12-12T11:21:16Z

src/test/java/org/apache/commons/collections4/bloomfilter/CountingPredicateTest.java

@@ -28,11 +28,11 @@
 import org.junit.jupiter.api.Test;

 public class CountingPredicateTest {
-    private List<Pair<Integer, Integer>> expected = new ArrayList<>();
-    private List<Pair<Integer, Integer>> result = new ArrayList<>();
+    //private List<Pair<Integer, Integer>> expected = new ArrayList<>();


@Claudenw I don't think we need to keep these.

garydgregory · 2023-12-22T14:00:34Z

@aherbert @Claudenw
Is this PR ready to merge? If not, what remains? TY!

aherbert · 2023-12-22T18:01:45Z

I previously requested Claude to remove some commented out code in a test class. Otherwise this is fine.

Claudenw added 13 commits June 27, 2023 22:19

Adjusted tests to handle bloom filter implementations that utilized

36beec4

automatic decay.

cleaned up spacing

5be49b7

fixed indent

3a1a8c8

updated for layered testing

a6b5e81

removed spaces

301041b

fixed merge issue

7631528

initial checkin

680d2bb

cleaned up tests

ee2cdff

fixed timing on test

d581e99

fixed formatting

0dddfa2

added javadoc

756e88f

fixed typos

ccf430f

removed blank lines

b8fe880

Claudenw self-assigned this Jun 29, 2023

Claudenw and others added 4 commits June 30, 2023 08:21

fixed javadocs

a6d4f46

Fix Javadoc

4ab19a6

Add Javadoc @SInCE 4.5

ad257e1

Add Javadoc @SInCE 4.5

d5e17e8

updated tests and added BloomFilterProducer code

97ca57e

Claudenw and others added 5 commits June 30, 2023 18:32

Merge branch 'apache:master' into layered_filter

cb09fbe

Merge branch 'layered_filter' of github.com:Claudenw/commons-collecti…

0123c3f

…ons into layered_filter

Cleaned up javadoc and BiPredicate<BloomFilter,BloomFilter> processing

1a647d5

fixed javadoc issues

62eba66

fixed typography issue

4e7ab0b

Claudenw requested a review from garydgregory June 30, 2023 18:41

Fixed a documentation error

a1749d7

aherbert changed the title ~~COMMONS-843: Implement Layered Bloom filter~~ COLLECTIONS-843: Implement Layered Bloom filter Jul 4, 2023

fixed checkstyle issue

9769046

garydgregory approved these changes Sep 27, 2023

View reviewed changes

aherbert requested changes Sep 27, 2023

View reviewed changes

modified as per review

b39a472

Merge branch 'layered_filter' of github.com:Claudenw/commons-collecti…

195f987

…ons into layered_filter

aherbert requested changes Oct 20, 2023

View reviewed changes

Claudenw added 2 commits October 27, 2023 17:40

Updated tests as per review

a71b55e

fixed variable initialization issues

4babacf

Claudenw requested a review from aherbert October 28, 2023 06:25

Merge branch 'master' into layered_filter

95df800

aherbert reviewed Nov 24, 2023

View reviewed changes

Claudenw added 2 commits November 27, 2023 17:40

made suggested test changes

68201e5

fixed broken test

d806d6e

aherbert reviewed Dec 12, 2023

View reviewed changes

garydgregory referenced this pull request Dec 22, 2023

Bump to next development version

bf5e458

Remove dead comments per code reviews

500a252

garydgregory changed the title ~~COLLECTIONS-843: Implement Layered Bloom filter~~ [COLLECTIONS-843] Implement Layered Bloom filter Dec 22, 2023

garydgregory merged commit 0438ede into apache:master Dec 22, 2023
7 checks passed

garydgregory added a commit that referenced this pull request Dec 22, 2023

Implement Layered Bloom filter #402

bf9461b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[COLLECTIONS-843] Implement Layered Bloom filter #402

[COLLECTIONS-843] Implement Layered Bloom filter #402

Claudenw commented Jun 29, 2023 •

edited by garydgregory

Loading

codecov-commenter commented Jun 29, 2023 •

edited

Loading

garydgregory commented Jun 30, 2023

Claudenw commented Jun 30, 2023

Claudenw commented Sep 3, 2023

Claudenw commented Sep 26, 2023

garydgregory left a comment

aherbert left a comment

Claudenw commented Oct 1, 2023

garydgregory commented Oct 1, 2023

Claudenw commented Oct 8, 2023

aherbert left a comment

aherbert Oct 20, 2023

Claudenw Oct 27, 2023

aherbert Oct 20, 2023

Claudenw Oct 26, 2023

aherbert Oct 20, 2023

Claudenw Oct 26, 2023

aherbert Oct 20, 2023

Claudenw Oct 27, 2023

garydgregory commented Oct 21, 2023

aherbert commented Oct 21, 2023

garydgregory commented Oct 21, 2023

Claudenw commented Nov 24, 2023

aherbert left a comment

aherbert Nov 24, 2023

Claudenw Nov 27, 2023

aherbert Dec 12, 2023

garydgregory commented Dec 22, 2023

aherbert commented Dec 22, 2023


		SparseDefaultBloomFilter(final Shape shape) {
		public SparseDefaultBloomFilter(final Shape shape) {

[COLLECTIONS-843] Implement Layered Bloom filter #402

[COLLECTIONS-843] Implement Layered Bloom filter #402

Conversation

Claudenw commented Jun 29, 2023 • edited by garydgregory Loading

codecov-commenter commented Jun 29, 2023 • edited Loading

Codecov Report

garydgregory commented Jun 30, 2023

Claudenw commented Jun 30, 2023

Claudenw commented Sep 3, 2023

Claudenw commented Sep 26, 2023

garydgregory left a comment

Choose a reason for hiding this comment

aherbert left a comment

Choose a reason for hiding this comment

Claudenw commented Oct 1, 2023

garydgregory commented Oct 1, 2023

Claudenw commented Oct 8, 2023

aherbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garydgregory commented Oct 21, 2023

aherbert commented Oct 21, 2023

garydgregory commented Oct 21, 2023

Claudenw commented Nov 24, 2023

aherbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garydgregory commented Dec 22, 2023

aherbert commented Dec 22, 2023

Claudenw commented Jun 29, 2023 •

edited by garydgregory

Loading

codecov-commenter commented Jun 29, 2023 •

edited

Loading