Re: [collections] Does WrappedBloomFilterTest make sense?
Minor difference in what the time is. In the Kip it is an expires time and
the filter is created with a TTL value. The copy is also slightly
different as the expires time is reset.
On Sat, Oct 5, 2024 at 8:53 PM Gary Gregory wrote:
> On Sat, Oct 5, 2024 at 11:26 AM Claude Warren wrote:
>
> > >
> > > WrappedBloomFilter is not a WrappedBloomFilter,
> >
> >
> > This is true in most useful cases for the test `
> > assertEquals(bf1.getClass(), copy.getClass());
> >
> > In a proposed KIP (Kafka Improvement Proposal) there is a Bloom filter
> that
> > is decorated with additional data (The timestamp for when it was
> created).
> > This class is called TimestampedBloomFilter and it extends
> > WrappedBloomFilter.
> >
>
> Hi Claude,
>
> We
> have
> org.apache.commons.collections4.bloomfilter.LayeredBloomFilterTest.TimestampedBloomFilter.
> How is the KIP different?
>
> Gary
>
>
> >
> > When TimestampedBloomFilter is passed to the WrappedBloomFilterTest
> classes
> > and the "copy" method is called, the result is not a WrappedBloomFilter
> but
> > a TimestampedBloomFilter.
> >
> > If the TimestampedBloomFilterTest extends the WrappedBloomFilterTest the
> > test fails.
> >
> >
> >
> >
> > On Sat, Oct 5, 2024 at 3:21 PM Gary Gregory
> > wrote:
> >
> > > The WrappedBloomFilterTest currently says [0][1] that a copy of a
> > > WrappedBloomFilter is not a WrappedBloomFilter, which does not make
> sense
> > > to me as non-SME.
> > >
> > > Should the test be adjusted? It looks like the test is written in a
> > > contrived way due to WrappedBloomFilter being abstract. I would make
> more
> > > sense to me to have private static WrappedBloomFilter in the test and
> use
> > > it, like this:
> > >
> > > private static class Fixture extends WrappedBloomFilter {
> > >
> > > public Fixture(BloomFilter wrapped) {
> > > super(wrapped);
> > > }
> > >
> > > @Override
> > > public WrappedBloomFilter copy() {
> > > return new Fixture(getWrapped());
> > > }
> > >
> > > }
> > >
> > > @Override
> > > protected WrappedBloomFilter createEmptyFilter(final Shape shape) {
> > > return new Fixture(new
> > > DefaultBloomFilterTest.SparseDefaultBloomFilter(shape));
> > > }
> > >
> > > @ParameterizedTest
> > > @ValueSource(ints = {0, 1, 34})
> > > public void testCharacteristics(final int characteristics) {
> > > final Shape shape = getTestShape();
> > > final BloomFilter inner = new
> > > DefaultBloomFilterTest.SparseDefaultBloomFilter(shape) {
> > > @Override
> > > public int characteristics() {
> > > return characteristics;
> > > }
> > > };
> > > final WrappedBloomFilter underTest = new Fixture(inner);
> > > assertEquals(characteristics, underTest.characteristics());
> > > }
> > >
> > > WDYT?
> > >
> > > Let's skip discussing the signature of the copy() method, I'll write a
> > > separate email later about that.
> > >
> > > TY,
> > > Gary
> > > [0]
> > >
> > >
> >
> https://github.com/apache/commons-collections/blob/730d972cdebb13dd3a896eb5b90ebc9e1f594d5b/src/test/java/org/apache/commons/collections4/bloomfilter/WrappedBloomFilterTest.java#L26-L56
> > > [1] The above link in WrappedBloomFilterTest is:
> > >
> > > @Override
> > > protected WrappedBloomFilter createEmptyFilter(final Shape shape) {
> > > return new WrappedBloomFilter(new
> > > DefaultBloomFilterTest.SparseDefaultBloomFilter(shape)) {
> > > @Override
> > > public BloomFilter copy() {
> > > final BloomFilter result = new
> > > DefaultBloomFilterTest.SparseDefaultBloomFilter(shape);
> > > result.merge(getWrapped());
> > > return result;
> > > }
> > > };
> > > }
> > >
> > > @ParameterizedTest
> > > @ValueSource(ints = {0, 1, 34})
> > > public void testCharacteristics(final int characteristics) {
> > > final Shape shape = getTestShape();
> > > final BloomFilter inner = new
> > > DefaultBloomFilterTest.SparseDefaultBloomFilter(shape) {
> > > @Override
> > > public int characteristics() {
> > > return characteristics;
> > > }
> > > };
> > > final WrappedBloomFilter underTest = new
> > WrappedBloomFilter(inner)
> > > {
> > > @Override
> > > public BloomFilter copy() {
> > > final BloomFilter result = new
> > > DefaultBloomFilterTest.SparseDefaultBloomFilter(shape);
> > > result.merge(getWrapped());
> > > return result;
> > > }
> > > };
> > > assertEquals(characteristics, underTest.characteristics());
> > > }
> > >
> >
> >
> > --
> > LinkedIn: http://www.linkedin.com/in/claudewarren
> >
>
--
LinkedIn: http://www.linkedin.com/in/claudewarren
Re: [collections] BloomFilterExtractor.flatten()
Thank you Claude, the git master code now throws instead of returning null. We can delay MultidimensionalBloomFilter unless introducing it later would break binary compatibility. How beneficial would introducing this interface for users? Gary On Sun, Oct 6, 2024, 10:58 AM Claude Warren wrote: > This is starting tondelve into the realm of multidimensional bloom filters. > > I think throwing an exception on a null or zero length array is acceptable. > > We could define an interface MultidimensionalBloomFilter that could be > implemented by any Bloom filter that comprises multiple filters and givr it > the method flatten(). But this is a can of worms I would like to keep > sealed for a bit longer. > > Claude > > On Sat 5 Oct 2024, 20:44 Gary Gregory, wrote: > > > On Fri, Oct 4, 2024 at 1:30 PM Alex Herbert > > wrote: > > > > > You cannot return an empty BloomFilter when there is no array of > > > BloomFilters to flatten. The BloomFilter is tied to a Shape. Without a > > > shape you cannot know what size filter to create. So we have to return > > > null or throw an exception. Whatever choice must be documented. > > > > > > > The code now throws an exception instead of returning null. I also > updated > > the Javadoc. > > > > > > > > > > Note we do not currently check if the Shapes are all equal when > > > creating the BloomFilterExtractor. This is not essential but when > > > merging two Bloom filters in flatten an error will be thrown if the > > > filters are not compatible. I say it is not essential as the > > > functionality of the BloomFilterExtract in processBloomFilters does > > > not mandate any prerequisites on the Predicate for the filters. So for > > > example you could have many filters of different sizes and a predicate > > > that tests if they all contain a given IndexProducer. > > > > > > > I'll leave that deeper topic alone for now, and for you and Claude to > > further discuss if warranted. > > > > TY, > > Gary > > > > > > > > > > Alex > > > > > > On Fri, 4 Oct 2024 at 18:02, Gary Gregory > > wrote: > > > > > > > > OK, but how do you create an empty filter? > > > > > > > > Gary > > > > > > > > On Fri, Oct 4, 2024, 12:54 PM Claude Warren > wrote: > > > > > > > > > I think that it should return an empty filter as the javadoc says > > > > > > > > > > > > > > > On Fri 4 Oct 2024, 15:53 Gary D. Gregory, > > wrote: > > > > > > > > > > > See the new disabled test BloomFilterExtractorTest. > > > > > > > > > > > > Gary > > > > > > > > > > > > On 2024/10/04 14:51:55 "Gary D. Gregory" wrote: > > > > > > > Hi Claude and all, > > > > > > > > > > > > > > The method: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > org.apache.commons.collections4.bloomfilter.BloomFilterExtractor.flatten() > > > > > > > > > > > > > > is documented as always returning a BloomFilter: > > > > > > > > > > > > > > /** > > > > > > > * Create a standard (non-layered) Bloom filter by merging > > all > > > of > > > > > > the layers. If > > > > > > > * the filter is empty this method will return an empty > Bloom > > > > > filter. > > > > > > > * > > > > > > > * @return the merged bloom filter. > > > > > > > */ > > > > > > > > > > > > > > But that's not how it's coded, it can return null, the simplest > > > > > > reproducer is: > > > > > > > > > > > > > > BloomFilterExtractor.fromBloomFilterArray(new > > > BloomFilter[0]).flatten() > > > > > > -> null > > > > > > > > > > > > > > Should we: > > > > > > > > > > > > > > - Change the Javadoc > > > > > > > - Change the code > > > > > > > -- How should this code be changed? > > > > > > > -- Should the method be moved to BloomFilter and use "this" as > > the > > > > > > default filter? > > > > > > > > > > > > > > TY, > > > > > > > Gary > > > > > > > > > > > > > > > > > - > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > - > > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > > > > >
[validator] Java 23 failures and the Narrow No-Break Space (NNBSP)
Hi All, The build fails on Java 23 because of at least one behavior change when running on Java 23: When dealing with strings like "12:34 PM", the space is now expected to be a Narrow No-Break Space (NNBSP, U+202F) and not a "traditional" space (U+0020). Please have a look at the code and help figure out if we should just update the tests to have different expectations on Java 23 or if we should make our main code smarter. TY Gary - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [collections] BloomFilterExtractor.flatten()
This is starting tondelve into the realm of multidimensional bloom filters. I think throwing an exception on a null or zero length array is acceptable. We could define an interface MultidimensionalBloomFilter that could be implemented by any Bloom filter that comprises multiple filters and givr it the method flatten(). But this is a can of worms I would like to keep sealed for a bit longer. Claude On Sat 5 Oct 2024, 20:44 Gary Gregory, wrote: > On Fri, Oct 4, 2024 at 1:30 PM Alex Herbert > wrote: > > > You cannot return an empty BloomFilter when there is no array of > > BloomFilters to flatten. The BloomFilter is tied to a Shape. Without a > > shape you cannot know what size filter to create. So we have to return > > null or throw an exception. Whatever choice must be documented. > > > > The code now throws an exception instead of returning null. I also updated > the Javadoc. > > > > > > Note we do not currently check if the Shapes are all equal when > > creating the BloomFilterExtractor. This is not essential but when > > merging two Bloom filters in flatten an error will be thrown if the > > filters are not compatible. I say it is not essential as the > > functionality of the BloomFilterExtract in processBloomFilters does > > not mandate any prerequisites on the Predicate for the filters. So for > > example you could have many filters of different sizes and a predicate > > that tests if they all contain a given IndexProducer. > > > > I'll leave that deeper topic alone for now, and for you and Claude to > further discuss if warranted. > > TY, > Gary > > > > > > Alex > > > > On Fri, 4 Oct 2024 at 18:02, Gary Gregory > wrote: > > > > > > OK, but how do you create an empty filter? > > > > > > Gary > > > > > > On Fri, Oct 4, 2024, 12:54 PM Claude Warren wrote: > > > > > > > I think that it should return an empty filter as the javadoc says > > > > > > > > > > > > On Fri 4 Oct 2024, 15:53 Gary D. Gregory, > wrote: > > > > > > > > > See the new disabled test BloomFilterExtractorTest. > > > > > > > > > > Gary > > > > > > > > > > On 2024/10/04 14:51:55 "Gary D. Gregory" wrote: > > > > > > Hi Claude and all, > > > > > > > > > > > > The method: > > > > > > > > > > > > > > > > > > > > > > > > org.apache.commons.collections4.bloomfilter.BloomFilterExtractor.flatten() > > > > > > > > > > > > is documented as always returning a BloomFilter: > > > > > > > > > > > > /** > > > > > > * Create a standard (non-layered) Bloom filter by merging > all > > of > > > > > the layers. If > > > > > > * the filter is empty this method will return an empty Bloom > > > > filter. > > > > > > * > > > > > > * @return the merged bloom filter. > > > > > > */ > > > > > > > > > > > > But that's not how it's coded, it can return null, the simplest > > > > > reproducer is: > > > > > > > > > > > > BloomFilterExtractor.fromBloomFilterArray(new > > BloomFilter[0]).flatten() > > > > > -> null > > > > > > > > > > > > Should we: > > > > > > > > > > > > - Change the Javadoc > > > > > > - Change the code > > > > > > -- How should this code be changed? > > > > > > -- Should the method be moved to BloomFilter and use "this" as > the > > > > > default filter? > > > > > > > > > > > > TY, > > > > > > Gary > > > > > > > > > > > > > > - > > > > > > 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 > > > > > > > > > > > > > > > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > >
