From 21725153c8c04bb33b6267fcd578e354211b73d9 Mon Sep 17 00:00:00 2001 From: leventov Date: Tue, 6 Dec 2016 21:51:28 -0600 Subject: [PATCH 1/3] Deduplicate looking for bitset.nextSetBit() in BitSetIterator.next() and hasNext() --- .../bitmap/WrappedImmutableBitSetBitmap.java | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/bytebuffer-collections/src/main/java/io/druid/collections/bitmap/WrappedImmutableBitSetBitmap.java b/bytebuffer-collections/src/main/java/io/druid/collections/bitmap/WrappedImmutableBitSetBitmap.java index 633a1de063ab..a567c1b2966a 100755 --- a/bytebuffer-collections/src/main/java/io/druid/collections/bitmap/WrappedImmutableBitSetBitmap.java +++ b/bytebuffer-collections/src/main/java/io/druid/collections/bitmap/WrappedImmutableBitSetBitmap.java @@ -23,6 +23,7 @@ import java.nio.ByteBuffer; import java.util.BitSet; +import java.util.NoSuchElementException; /** * WrappedImmutableBitSetBitmap implements ImmutableBitmap for java.util.BitSet @@ -117,18 +118,27 @@ public ImmutableBitmap difference(ImmutableBitmap otherBitmap) private class BitSetIterator implements IntIterator { - private int pos = -1; + private int nextPos; + + BitSetIterator() + { + nextPos = bitmap.nextSetBit(0); + } @Override public boolean hasNext() { - return bitmap.nextSetBit(pos + 1) >= 0; + return nextPos >= 0; } @Override public int next() { - pos = bitmap.nextSetBit(pos + 1); + int pos = nextPos; + if (pos < 0) { + throw new NoSuchElementException(); + } + nextPos = bitmap.nextSetBit(pos + 1); return pos; } @@ -136,7 +146,7 @@ public int next() public IntIterator clone() { BitSetIterator newIt = new BitSetIterator(); - newIt.pos = pos; + newIt.nextPos = nextPos; return newIt; } } From 0ee24b2d94c1c6231eacf4fcf96bf1f0e2c814f3 Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 7 Dec 2016 00:36:03 -0600 Subject: [PATCH 2/3] Add BitmapIterationTest --- bytebuffer-collections/pom.xml | 5 + .../bitmap/BitmapIterationTest.java | 144 ++++++++++++++++++ pom.xml | 9 +- 3 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 bytebuffer-collections/src/test/java/io/druid/collections/bitmap/BitmapIterationTest.java diff --git a/bytebuffer-collections/pom.xml b/bytebuffer-collections/pom.xml index 7a11fed8b8e1..108ac3cec0f8 100755 --- a/bytebuffer-collections/pom.xml +++ b/bytebuffer-collections/pom.xml @@ -88,6 +88,11 @@ 1.4.182 test + + com.google.guava + guava-testlib + test + diff --git a/bytebuffer-collections/src/test/java/io/druid/collections/bitmap/BitmapIterationTest.java b/bytebuffer-collections/src/test/java/io/druid/collections/bitmap/BitmapIterationTest.java new file mode 100644 index 000000000000..488a732cf65a --- /dev/null +++ b/bytebuffer-collections/src/test/java/io/druid/collections/bitmap/BitmapIterationTest.java @@ -0,0 +1,144 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets 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 io.druid.collections.bitmap; + +import com.google.common.collect.UnmodifiableIterator; +import com.google.common.collect.testing.CollectionTestSuiteBuilder; +import com.google.common.collect.testing.SampleElements; +import com.google.common.collect.testing.TestCollectionGenerator; +import com.google.common.collect.testing.features.CollectionFeature; +import com.google.common.collect.testing.features.CollectionSize; +import junit.framework.Test; +import junit.framework.TestCase; +import junit.framework.TestSuite; +import org.roaringbitmap.IntIterator; + +import java.util.AbstractCollection; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +public class BitmapIterationTest extends TestCase +{ + public static Test suite() + { + List factories = Arrays.asList( + new BitSetBitmapFactory(), + new ConciseBitmapFactory() + // Exclude Roaring because it fails yet! + //new RoaringBitmapFactory() + ); + + TestSuite suite = new TestSuite(); + for (BitmapFactory factory : factories) { + suite.addTest(suiteForFactory(factory)); + } + return suite; + } + + private static Test suiteForFactory(BitmapFactory factory) + { + return CollectionTestSuiteBuilder + .using(new BitmapCollectionGenerator(factory)) + .named("bitmap iteration tests of " + factory) + .withFeatures(CollectionFeature.KNOWN_ORDER) + .withFeatures(CollectionFeature.REJECTS_DUPLICATES_AT_CREATION) + .withFeatures(CollectionFeature.RESTRICTS_ELEMENTS) + .withFeatures(CollectionSize.ANY) + .createTestSuite(); + } + + private static class BitmapCollection extends AbstractCollection + { + private final ImmutableBitmap bitmap; + private final int size; + + private BitmapCollection(ImmutableBitmap bitmap, int size) + { + this.bitmap = bitmap; + this.size = size; + } + + @Override + public UnmodifiableIterator iterator() + { + final IntIterator iterator = bitmap.iterator(); + return new UnmodifiableIterator() + { + @Override + public boolean hasNext() + { + return iterator.hasNext(); + } + + @Override + public Integer next() + { + return iterator.next(); + } + }; + } + + @Override + public int size() + { + return size; + } + } + + private static class BitmapCollectionGenerator implements TestCollectionGenerator + { + private final BitmapFactory factory; + + private BitmapCollectionGenerator(BitmapFactory factory) + { + this.factory = factory; + } + + @Override + public SampleElements samples() + { + return new SampleElements.Ints(); + } + + @Override + public BitmapCollection create(Object... objects) + { + MutableBitmap mutableBitmap = factory.makeEmptyMutableBitmap(); + for (Object element : objects) { + mutableBitmap.add(((Integer) element)); + } + return new BitmapCollection(factory.makeImmutableBitmap(mutableBitmap), objects.length); + } + + @Override + public Integer[] createArray(int n) + { + return new Integer[n]; + } + + @Override + public Iterable order(List list) + { + Collections.sort(list); + return list; + } + } +} diff --git a/pom.xml b/pom.xml index ee2c741478e2..23752585333c 100644 --- a/pom.xml +++ b/pom.xml @@ -59,6 +59,7 @@ 2.11.0 + 16.0.1 4.1.0 9.2.5.v20141112 1.19 @@ -252,7 +253,7 @@ com.google.guava guava - 16.0.1 + ${guava.version} com.google.inject @@ -660,6 +661,12 @@ 1.0.4 test + + com.google.guava + guava-testlib + ${guava.version} + test + From 4b05b12860d7e8c565a8b4f73024354715fbba55 Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 7 Dec 2016 14:32:12 -0600 Subject: [PATCH 3/3] More elaborate comment on why Roaring is not tested in BitmapIterationTest --- .../druid/collections/bitmap/BitmapIterationTest.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/bytebuffer-collections/src/test/java/io/druid/collections/bitmap/BitmapIterationTest.java b/bytebuffer-collections/src/test/java/io/druid/collections/bitmap/BitmapIterationTest.java index 488a732cf65a..0a08b5a7f3fa 100644 --- a/bytebuffer-collections/src/test/java/io/druid/collections/bitmap/BitmapIterationTest.java +++ b/bytebuffer-collections/src/test/java/io/druid/collections/bitmap/BitmapIterationTest.java @@ -42,7 +42,16 @@ public static Test suite() List factories = Arrays.asList( new BitSetBitmapFactory(), new ConciseBitmapFactory() - // Exclude Roaring because it fails yet! + // Roaring iteration fails because it doesn't throw NoSuchElementException on next() call when there are no more + // elements. Instead, it either throws NullPointerException or returns some unspecified value. If bitmap + // iterators are always used correctly in shouldn't be a problem, but if next() is occasionally called after + // hasNext() returned false and it returns some unspecified value, and everything continues to work without + // indication that there was a error, it would be a bug that is very hard to catch. + // + // This line should be uncommented when Druid updates RoaringBitmap dependency to a version which includes a fix + // for https://github.com/RoaringBitmap/RoaringBitmap/issues/129, or when RoaringBitmap is included into Druid + // as a module and the issue is fixed there. + //new RoaringBitmapFactory() );