From e8c5644d3e653c31ce326b5454f3dd5447940d3a Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Mon, 4 Nov 2019 16:13:04 -0800 Subject: [PATCH 1/9] use peekable iterator for numeric column selector null checking instead of bitmap.get for those sweet sweet nanoseconds --- ...lHandlingBitmapGetVsIteratorBenchmark.java | 180 ++++++++++++++++++ .../ConcisePeekableIteratorAdapter.java | 53 ++++++ .../collections/bitmap/ImmutableBitmap.java | 9 + .../bitmap/PeekableIteratorAdapter.java | 83 ++++++++ .../bitmap/WrappedConciseBitmap.java | 7 + .../bitmap/WrappedImmutableConciseBitmap.java | 8 +- .../bitmap/WrappedImmutableRoaringBitmap.java | 7 + .../bitmap/WrappedRoaringBitmap.java | 7 + .../druid/segment/data/ColumnarDoubles.java | 15 +- .../druid/segment/data/ColumnarFloats.java | 15 +- .../druid/segment/data/ColumnarLongs.java | 16 +- .../segment/vector/VectorSelectorUtils.java | 59 ++++++ .../bitmap/BitmapPeekableIteratorTest.java | 98 ++++++++++ .../bitmap/WrappedRoaringBitmapTest.java | 2 +- 14 files changed, 551 insertions(+), 8 deletions(-) create mode 100644 benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java create mode 100644 processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java create mode 100644 processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java create mode 100644 processing/src/test/java/org/apache/druid/collections/bitmap/BitmapPeekableIteratorTest.java diff --git a/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java b/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java new file mode 100644 index 000000000000..2576eb46b29e --- /dev/null +++ b/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java @@ -0,0 +1,180 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.benchmark; + +import org.apache.druid.collections.bitmap.ImmutableBitmap; +import org.apache.druid.collections.bitmap.WrappedImmutableConciseBitmap; +import org.apache.druid.collections.bitmap.WrappedImmutableRoaringBitmap; +import org.apache.druid.extendedset.intset.ConciseSet; +import org.apache.druid.extendedset.intset.ImmutableConciseSet; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Timeout; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; +import org.roaringbitmap.IntIterator; +import org.roaringbitmap.PeekableIntIterator; +import org.roaringbitmap.buffer.MutableRoaringBitmap; + +import java.util.BitSet; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.TimeUnit; + +@State(Scope.Benchmark) +@Fork(value = 1) +@Warmup(iterations = 5, time = 10, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 10, time = 10, timeUnit = TimeUnit.SECONDS) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MILLISECONDS) +@Timeout(time = 30, timeUnit = TimeUnit.SECONDS) +public class NullHandlingBitmapGetVsIteratorBenchmark +{ + @Param({ + //"concise", + "roaring" + }) + String bitmapType; + + @Param({"500000"}) + private int numRows; + + @Param({ + "0.001", + "0.01", + "0.1", + "0.25", + "0.5", + "0.75", + "0.99999" + }) + private double filterMatch; + + @Param({ + "0", + "0.1", + "0.25", + "0.5", + "0.75", + "0.99" + }) + private double nullDensity; + + ImmutableBitmap bitmap; + BitSet pretendFilterOffsets; + + @Setup + public void setup() + { + pretendFilterOffsets = new BitSet(numRows); + switch (bitmapType) { + case "concise": + ConciseSet conciseSet = new ConciseSet(); + for (int i = 0; i < numRows; i++) { + double rando = ThreadLocalRandom.current().nextDouble(0.0, 1.0); + if (filterMatch == 1.0 || rando < filterMatch) { + pretendFilterOffsets.set(i); + } + if (rando < nullDensity) { + conciseSet.add(i); + } + } + bitmap = new WrappedImmutableConciseBitmap(ImmutableConciseSet.newImmutableFromMutable(conciseSet)); + break; + case "roaring": + MutableRoaringBitmap roaringBitmap = new MutableRoaringBitmap(); + + for (int i = 0; i < numRows; i++) { + double rando = ThreadLocalRandom.current().nextDouble(0.0, 1.0); + if (filterMatch == 1.0 || rando < filterMatch) { + pretendFilterOffsets.set(i); + } + if (rando < nullDensity) { + roaringBitmap.add(i); + } + } + bitmap = new WrappedImmutableRoaringBitmap(roaringBitmap.toImmutableRoaringBitmap()); + break; + } + } + + @Benchmark + @BenchmarkMode(Mode.AverageTime) + @OutputTimeUnit(TimeUnit.MICROSECONDS) + public void get(final Blackhole blackhole) + { + for (int i = pretendFilterOffsets.nextSetBit(0); i >= 0; i = pretendFilterOffsets.nextSetBit(i + 1)) { + final boolean isNull = bitmap.get(i); + if (isNull) { + blackhole.consume(null); + } else { + blackhole.consume(i); + } + } + } + + @Benchmark + @BenchmarkMode(Mode.AverageTime) + @OutputTimeUnit(TimeUnit.MICROSECONDS) + public void iterator(final Blackhole blackhole) + { + IntIterator nullIterator = bitmap.iterator(); + int nullMark = -1; + for (int i = pretendFilterOffsets.nextSetBit(0); i >= 0; i = pretendFilterOffsets.nextSetBit(i + 1)) { + while (nullMark < i && nullIterator.hasNext()) { + nullMark = nullIterator.next(); + } + final boolean isNull = nullMark == i; + if (isNull) { + blackhole.consume(null); + } else { + blackhole.consume(i); + } + } + } + + @Benchmark + @BenchmarkMode(Mode.AverageTime) + @OutputTimeUnit(TimeUnit.MICROSECONDS) + public void peekableIterator(final Blackhole blackhole) + { + PeekableIntIterator nullIterator = bitmap.peekableIterator(); + int nullMark = -1; + for (int i = pretendFilterOffsets.nextSetBit(0); i >= 0; i = pretendFilterOffsets.nextSetBit(i + 1)) { + nullIterator.advanceIfNeeded(i); + while (nullMark < i && nullIterator.hasNext()) { + nullMark = nullIterator.next(); + } + final boolean isNull = nullMark == i; + if (isNull) { + blackhole.consume(null); + } else { + blackhole.consume(i); + } + } + } +} diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java new file mode 100644 index 000000000000..c975bebda27e --- /dev/null +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.collections.bitmap; + +import org.apache.druid.extendedset.intset.IntSet; +import org.roaringbitmap.PeekableIntIterator; + +public class ConcisePeekableIteratorAdapter extends PeekableIteratorAdapter +{ + ConcisePeekableIteratorAdapter(IntSet.IntIterator iterator) + { + super(iterator); + } + + ConcisePeekableIteratorAdapter(IntSet.IntIterator iterator, int mark) + { + super(iterator, mark); + } + + @Override + public void advanceIfNeeded(int i) + { + if (mark == null || i > mark) { + baseIterator.skipAllBefore(i); + if (baseIterator.hasNext()) { + mark = baseIterator.next(); + } + } + } + + @Override + public PeekableIntIterator clone() + { + return new ConcisePeekableIteratorAdapter(baseIterator.clone(), mark); + } +} diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java b/processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java index d02e2371d500..defd35ae20ad 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java @@ -21,6 +21,7 @@ import org.roaringbitmap.BatchIterator; import org.roaringbitmap.IntIterator; +import org.roaringbitmap.PeekableIntIterator; /** * This class is meant to represent a simple wrapper around an immutable bitmap @@ -33,6 +34,14 @@ public interface ImmutableBitmap */ IntIterator iterator(); + /** + * @return a peekable iterator which can skip to a position + */ + default PeekableIntIterator peekableIterator() + { + return new PeekableIteratorAdapter(iterator()); + } + /** * @return a batched iterator over the set bits of this bitmap */ diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java new file mode 100644 index 000000000000..393598cbd7e4 --- /dev/null +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java @@ -0,0 +1,83 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.collections.bitmap; + +import com.google.common.base.Preconditions; +import org.roaringbitmap.IntIterator; +import org.roaringbitmap.PeekableIntIterator; + +public class PeekableIteratorAdapter implements PeekableIntIterator +{ + final TIntIterator baseIterator; + Integer mark; + + PeekableIteratorAdapter(TIntIterator iterator) + { + this.baseIterator = Preconditions.checkNotNull(iterator, "iterator"); + } + + PeekableIteratorAdapter(TIntIterator iterator, int mark) + { + this(iterator); + this.mark = mark; + this.advanceIfNeeded(mark); + } + + @Override + public void advanceIfNeeded(int i) + { + while ((mark == null || mark < i) && baseIterator.hasNext()) { + mark = baseIterator.next(); + } + } + + @Override + public int peekNext() + { + Preconditions.checkArgument(mark != null || baseIterator.hasNext()); + if (mark == null) { + mark = baseIterator.next(); + } + return mark; + } + + @Override + public PeekableIntIterator clone() + { + return new PeekableIteratorAdapter(baseIterator.clone(), mark); + } + + @Override + public boolean hasNext() + { + return mark != null || baseIterator.hasNext(); + } + + @Override + public int next() + { + if (mark != null) { + final int currentBit = mark; + mark = null; + return currentBit; + } + return baseIterator.next(); + } +} diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedConciseBitmap.java b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedConciseBitmap.java index f0e0cfa561fe..84d151d55b45 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedConciseBitmap.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedConciseBitmap.java @@ -22,6 +22,7 @@ import org.apache.druid.extendedset.intset.ConciseSet; import org.apache.druid.extendedset.intset.ImmutableConciseSet; import org.roaringbitmap.IntIterator; +import org.roaringbitmap.PeekableIntIterator; public class WrappedConciseBitmap implements MutableBitmap { @@ -109,6 +110,12 @@ public IntIterator iterator() return bitmap.iterator(); } + @Override + public PeekableIntIterator peekableIterator() + { + return new ConcisePeekableIteratorAdapter(bitmap.iterator()); + } + @Override public boolean isEmpty() { diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedImmutableConciseBitmap.java b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedImmutableConciseBitmap.java index 85e0a547c9f6..c0cf3435ab8b 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedImmutableConciseBitmap.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedImmutableConciseBitmap.java @@ -19,9 +19,9 @@ package org.apache.druid.collections.bitmap; - import org.apache.druid.extendedset.intset.ImmutableConciseSet; import org.roaringbitmap.IntIterator; +import org.roaringbitmap.PeekableIntIterator; import java.nio.IntBuffer; @@ -76,6 +76,12 @@ public IntIterator iterator() return bitmap.iterator(); } + @Override + public PeekableIntIterator peekableIterator() + { + return new ConcisePeekableIteratorAdapter(bitmap.iterator()); + } + @Override public int size() { diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedImmutableRoaringBitmap.java b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedImmutableRoaringBitmap.java index 763445a96127..023d2fbe2efa 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedImmutableRoaringBitmap.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedImmutableRoaringBitmap.java @@ -21,6 +21,7 @@ import org.roaringbitmap.BatchIterator; import org.roaringbitmap.IntIterator; +import org.roaringbitmap.PeekableIntIterator; import org.roaringbitmap.buffer.ImmutableRoaringBitmap; import java.nio.ByteBuffer; @@ -77,6 +78,12 @@ public IntIterator iterator() return bitmap.getIntIterator(); } + @Override + public PeekableIntIterator peekableIterator() + { + return bitmap.getIntIterator(); + } + @Override public BatchIterator batchIterator() { diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmap.java b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmap.java index c3ab977bc774..73fc382d558d 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmap.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmap.java @@ -20,6 +20,7 @@ package org.apache.druid.collections.bitmap; import org.roaringbitmap.IntIterator; +import org.roaringbitmap.PeekableIntIterator; import org.roaringbitmap.RoaringBitmap; import org.roaringbitmap.RoaringBitmapWriter; import org.roaringbitmap.buffer.MutableRoaringBitmap; @@ -146,6 +147,12 @@ public IntIterator iterator() return writer.get().getIntIterator(); } + @Override + public PeekableIntIterator peekableIterator() + { + return writer.get().getIntIterator(); + } + @Override public boolean isEmpty() { diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java index 4f357af4ecb9..2f2327d94618 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java @@ -29,6 +29,7 @@ import org.apache.druid.segment.vector.ReadableVectorOffset; import org.apache.druid.segment.vector.VectorSelectorUtils; import org.apache.druid.segment.vector.VectorValueSelector; +import org.roaringbitmap.PeekableIntIterator; import javax.annotation.Nullable; import java.io.Closeable; @@ -94,10 +95,18 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) } else { class HistoricalDoubleColumnSelectorWithNulls implements DoubleColumnSelector, HistoricalColumnSelector { + private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private int nullMark = -1; + @Override public boolean isNull() { - return nullValueBitmap.get(offset.getOffset()); + final int i = offset.getOffset(); + nullIterator.advanceIfNeeded(i); + while (nullIterator.hasNext() && nullMark < i) { + nullMark = nullIterator.next(); + } + return nullMark == i; } @Override @@ -137,6 +146,8 @@ class ColumnarDoublesVectorValueSelector extends BaseDoubleVectorValueSelector private int id = ReadableVectorOffset.NULL_ID; + private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + @Nullable private boolean[] nullVector = null; @@ -173,7 +184,7 @@ private void computeVectorsIfNeeded() ColumnarDoubles.this.get(doubleVector, offset.getOffsets(), offset.getCurrentVectorSize()); } - nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullValueBitmap); + nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullIterator); id = offset.getId(); } diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java index adcffb4597a6..834080264119 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java @@ -29,6 +29,7 @@ import org.apache.druid.segment.vector.ReadableVectorOffset; import org.apache.druid.segment.vector.VectorSelectorUtils; import org.apache.druid.segment.vector.VectorValueSelector; +import org.roaringbitmap.PeekableIntIterator; import javax.annotation.Nullable; import java.io.Closeable; @@ -94,10 +95,18 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) } else { class HistoricalFloatColumnSelectorwithNulls implements FloatColumnSelector, HistoricalColumnSelector { + private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private int nullMark = -1; + @Override public boolean isNull() { - return nullValueBitmap.get(offset.getOffset()); + final int i = offset.getOffset(); + nullIterator.advanceIfNeeded(i); + while (nullIterator.hasNext() && nullMark < i) { + nullMark = nullIterator.next(); + } + return nullMark == i; } @Override @@ -137,6 +146,8 @@ class ColumnarFloatsVectorValueSelector extends BaseFloatVectorValueSelector private int id = ReadableVectorOffset.NULL_ID; + private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + @Nullable private boolean[] nullVector = null; @@ -173,7 +184,7 @@ private void computeVectorsIfNeeded() ColumnarFloats.this.get(floatVector, offset.getOffsets(), offset.getCurrentVectorSize()); } - nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullValueBitmap); + nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullIterator); id = offset.getId(); } diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java index 41a3ac104109..e7eef6c28046 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java @@ -29,6 +29,7 @@ import org.apache.druid.segment.vector.ReadableVectorOffset; import org.apache.druid.segment.vector.VectorSelectorUtils; import org.apache.druid.segment.vector.VectorValueSelector; +import org.roaringbitmap.PeekableIntIterator; import javax.annotation.Nullable; import java.io.Closeable; @@ -94,10 +95,19 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) } else { class HistoricalLongColumnSelectorWithNulls implements LongColumnSelector, HistoricalColumnSelector { + private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private int nullMark = -1; + @Override public boolean isNull() { - return nullValueBitmap.get(offset.getOffset()); + final int i = offset.getOffset(); + nullIterator.advanceIfNeeded(i); + while (nullIterator.hasNext() && nullMark < i) { + nullMark = nullIterator.next(); + } + assert ((nullMark == i) == nullValueBitmap.get(i)); + return nullMark == i; } @Override @@ -137,6 +147,8 @@ class ColumnarLongsVectorValueSelector extends BaseLongVectorValueSelector private int id = ReadableVectorOffset.NULL_ID; + private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + @Nullable private boolean[] nullVector = null; @@ -173,7 +185,7 @@ private void computeVectorsIfNeeded() ColumnarLongs.this.get(longVector, offset.getOffsets(), offset.getCurrentVectorSize()); } - nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullValueBitmap); + nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullIterator); id = offset.getId(); } diff --git a/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java b/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java index 112d1b119d51..c641f542af92 100644 --- a/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java +++ b/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java @@ -20,6 +20,7 @@ package org.apache.druid.segment.vector; import org.apache.druid.collections.bitmap.ImmutableBitmap; +import org.roaringbitmap.PeekableIntIterator; import javax.annotation.Nullable; @@ -60,4 +61,62 @@ public static boolean[] populateNullVector( return retVal; } + + @Nullable + public static boolean[] populateNullVector( + @Nullable final boolean[] nullVector, + final ReadableVectorOffset offset, + final PeekableIntIterator nullIterator + ) + { + if (!nullIterator.hasNext()) { + return null; + } + + final boolean[] retVal; + + if (nullVector != null) { + retVal = nullVector; + } else { + retVal = new boolean[offset.getMaxVectorSize()]; + } + + + int nextNullRow = nullIterator.next(); + if (offset.isContiguous()) { + nullIterator.advanceIfNeeded(offset.getStartOffset()); + final int startOffset = offset.getStartOffset(); + for (int i = 0; i < offset.getCurrentVectorSize(); i++) { + final int row = i + startOffset; + if (row == nextNullRow) { + retVal[i] = true; + if (nullIterator.hasNext()) { + nextNullRow = nullIterator.next(); + } else { + break; + } + } else { + retVal[i] = false; + } + } + } else { + final int[] currentOffsets = offset.getOffsets(); + nullIterator.advanceIfNeeded(currentOffsets[0]); + for (int i = 0; i < offset.getCurrentVectorSize(); i++) { + final int row = currentOffsets[i]; + if (row == nextNullRow) { + retVal[i] = true; + if (nullIterator.hasNext()) { + nextNullRow = nullIterator.next(); + } else { + break; + } + } else { + retVal[i] = false; + } + } + } + + return retVal; + } } diff --git a/processing/src/test/java/org/apache/druid/collections/bitmap/BitmapPeekableIteratorTest.java b/processing/src/test/java/org/apache/druid/collections/bitmap/BitmapPeekableIteratorTest.java new file mode 100644 index 000000000000..87eb4e44bd28 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/collections/bitmap/BitmapPeekableIteratorTest.java @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.collections.bitmap; + +import org.apache.druid.collections.IntSetTestUtility; +import org.apache.druid.extendedset.intset.ImmutableConciseSet; +import org.junit.Assert; +import org.junit.Test; +import org.roaringbitmap.PeekableIntIterator; + +public class BitmapPeekableIteratorTest +{ + @Test + public void testBitSet() + { + final WrappedBitSetBitmap bitmap = new WrappedBitSetBitmap(); + populate(bitmap); + assertPeekable(bitmap.peekableIterator()); + } + + @Test + public void testConciseMutable() + { + final WrappedConciseBitmap bitmap = new WrappedConciseBitmap(); + populate(bitmap); + assertPeekable(bitmap.peekableIterator()); + } + + @Test + public void testConciseImmutable() + { + final WrappedConciseBitmap bitmap = new WrappedConciseBitmap(); + populate(bitmap); + final ImmutableBitmap immutable = new WrappedImmutableConciseBitmap( + ImmutableConciseSet.newImmutableFromMutable(bitmap.getBitmap()) + ); + assertPeekable(immutable.peekableIterator()); + } + + @Test + public void testRoaringMutable() + { + WrappedRoaringBitmap bitmap = new WrappedRoaringBitmap(); + populate(bitmap); + assertPeekable(bitmap.peekableIterator()); + } + + @Test + public void testRoaringImmutable() + { + WrappedRoaringBitmap bitmap = new WrappedRoaringBitmap(); + populate(bitmap); + assertPeekable(bitmap.toImmutableBitmap().peekableIterator()); + } + + private void populate(MutableBitmap bitmap) + { + for (int i : IntSetTestUtility.getSetBits()) { + bitmap.add(i); + } + } + + private void assertPeekable(PeekableIntIterator iterator) + { + // extra calls are to make sure things that are not expected to have apparent side-effects have none + Assert.assertTrue(iterator.hasNext()); + int mark = -1; + for (int i : IntSetTestUtility.getSetBits()) { + Assert.assertTrue(iterator.hasNext()); + iterator.advanceIfNeeded(i); + Assert.assertTrue(iterator.hasNext()); + iterator.advanceIfNeeded(i); // this should do nothing + while (mark < i && iterator.hasNext()) { + Assert.assertEquals(i, iterator.peekNext()); + mark = iterator.next(); + } + Assert.assertEquals(i, mark); + } + Assert.assertFalse(iterator.hasNext()); + } +} diff --git a/processing/src/test/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmapTest.java b/processing/src/test/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmapTest.java index 798a3f49d703..6c9e1231c2f3 100644 --- a/processing/src/test/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmapTest.java +++ b/processing/src/test/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmapTest.java @@ -46,7 +46,7 @@ public static List factoryClasses() new RoaringBitmapFactory(false) }, new RoaringBitmapFactory[] { - new RoaringBitmapFactory(false) + new RoaringBitmapFactory(true) } ); } From 8d6133e87ef56bd52a480378306a88e76512233a Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 5 Nov 2019 18:50:37 -0800 Subject: [PATCH 2/9] remove unused method --- .../segment/vector/VectorSelectorUtils.java | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java b/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java index c641f542af92..0a425c204f7d 100644 --- a/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java +++ b/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java @@ -19,7 +19,6 @@ package org.apache.druid.segment.vector; -import org.apache.druid.collections.bitmap.ImmutableBitmap; import org.roaringbitmap.PeekableIntIterator; import javax.annotation.Nullable; @@ -29,39 +28,6 @@ public class VectorSelectorUtils /** * Helper used by ColumnarLongs, ColumnarDoubles, etc. to populate null-flag vectors. */ - @Nullable - public static boolean[] populateNullVector( - @Nullable final boolean[] nullVector, - final ReadableVectorOffset offset, - final ImmutableBitmap nullValueBitmap - ) - { - if (nullValueBitmap.isEmpty()) { - return null; - } - - final boolean[] retVal; - - if (nullVector != null) { - retVal = nullVector; - } else { - retVal = new boolean[offset.getMaxVectorSize()]; - } - - // Probably not super efficient to call "get" so much, but, no worse than the non-vectorized version. - if (offset.isContiguous()) { - for (int i = 0; i < offset.getCurrentVectorSize(); i++) { - retVal[i] = nullValueBitmap.get(i + offset.getStartOffset()); - } - } else { - for (int i = 0; i < offset.getCurrentVectorSize(); i++) { - retVal[i] = nullValueBitmap.get(offset.getOffsets()[i]); - } - } - - return retVal; - } - @Nullable public static boolean[] populateNullVector( @Nullable final boolean[] nullVector, From 58f9803ad2745da28bc19bc98619a4f3cf15911c Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 6 Nov 2019 03:03:51 -0800 Subject: [PATCH 3/9] slight optimization i think --- .../NullHandlingBitmapGetVsIteratorBenchmark.java | 14 ++++++++------ .../apache/druid/segment/data/ColumnarDoubles.java | 8 +++++--- .../apache/druid/segment/data/ColumnarFloats.java | 8 +++++--- .../apache/druid/segment/data/ColumnarLongs.java | 9 +++++---- .../druid/segment/vector/VectorSelectorUtils.java | 3 +-- .../bitmap/BitmapPeekableIteratorTest.java | 3 ++- 6 files changed, 26 insertions(+), 19 deletions(-) diff --git a/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java b/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java index 2576eb46b29e..4704ee7076d9 100644 --- a/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java +++ b/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java @@ -48,7 +48,7 @@ @State(Scope.Benchmark) @Fork(value = 1) @Warmup(iterations = 5, time = 10, timeUnit = TimeUnit.SECONDS) -@Measurement(iterations = 10, time = 10, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 20, time = 10, timeUnit = TimeUnit.SECONDS) @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.MILLISECONDS) @Timeout(time = 30, timeUnit = TimeUnit.SECONDS) @@ -165,15 +165,17 @@ public void peekableIterator(final Blackhole blackhole) PeekableIntIterator nullIterator = bitmap.peekableIterator(); int nullMark = -1; for (int i = pretendFilterOffsets.nextSetBit(0); i >= 0; i = pretendFilterOffsets.nextSetBit(i + 1)) { - nullIterator.advanceIfNeeded(i); - while (nullMark < i && nullIterator.hasNext()) { - nullMark = nullIterator.next(); + if (nullMark < i) { + nullIterator.advanceIfNeeded(i); + if (nullIterator.hasNext()) { + nullMark = nullIterator.next(); + } } final boolean isNull = nullMark == i; if (isNull) { - blackhole.consume(null); - } else { blackhole.consume(i); + } else { + blackhole.consume(null); } } } diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java index 2f2327d94618..51627f382840 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java @@ -102,9 +102,11 @@ class HistoricalDoubleColumnSelectorWithNulls implements DoubleColumnSelector, H public boolean isNull() { final int i = offset.getOffset(); - nullIterator.advanceIfNeeded(i); - while (nullIterator.hasNext() && nullMark < i) { - nullMark = nullIterator.next(); + if (nullMark < i) { + nullIterator.advanceIfNeeded(i); + if (nullIterator.hasNext()) { + nullMark = nullIterator.next(); + } } return nullMark == i; } diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java index 834080264119..dfd43470c521 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java @@ -102,9 +102,11 @@ class HistoricalFloatColumnSelectorwithNulls implements FloatColumnSelector, His public boolean isNull() { final int i = offset.getOffset(); - nullIterator.advanceIfNeeded(i); - while (nullIterator.hasNext() && nullMark < i) { - nullMark = nullIterator.next(); + if (nullMark < i) { + nullIterator.advanceIfNeeded(i); + if (nullIterator.hasNext()) { + nullMark = nullIterator.next(); + } } return nullMark == i; } diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java index e7eef6c28046..01034692461f 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java @@ -102,11 +102,12 @@ class HistoricalLongColumnSelectorWithNulls implements LongColumnSelector, Histo public boolean isNull() { final int i = offset.getOffset(); - nullIterator.advanceIfNeeded(i); - while (nullIterator.hasNext() && nullMark < i) { - nullMark = nullIterator.next(); + if (nullMark < i) { + nullIterator.advanceIfNeeded(i); + if (nullIterator.hasNext()) { + nullMark = nullIterator.next(); + } } - assert ((nullMark == i) == nullValueBitmap.get(i)); return nullMark == i; } diff --git a/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java b/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java index 0a425c204f7d..7d0355da5be0 100644 --- a/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java +++ b/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java @@ -47,11 +47,10 @@ public static boolean[] populateNullVector( retVal = new boolean[offset.getMaxVectorSize()]; } - int nextNullRow = nullIterator.next(); if (offset.isContiguous()) { - nullIterator.advanceIfNeeded(offset.getStartOffset()); final int startOffset = offset.getStartOffset(); + nullIterator.advanceIfNeeded(startOffset); for (int i = 0; i < offset.getCurrentVectorSize(); i++) { final int row = i + startOffset; if (row == nextNullRow) { diff --git a/processing/src/test/java/org/apache/druid/collections/bitmap/BitmapPeekableIteratorTest.java b/processing/src/test/java/org/apache/druid/collections/bitmap/BitmapPeekableIteratorTest.java index 87eb4e44bd28..ccc855e34f8d 100644 --- a/processing/src/test/java/org/apache/druid/collections/bitmap/BitmapPeekableIteratorTest.java +++ b/processing/src/test/java/org/apache/druid/collections/bitmap/BitmapPeekableIteratorTest.java @@ -87,7 +87,8 @@ private void assertPeekable(PeekableIntIterator iterator) iterator.advanceIfNeeded(i); Assert.assertTrue(iterator.hasNext()); iterator.advanceIfNeeded(i); // this should do nothing - while (mark < i && iterator.hasNext()) { + Assert.assertTrue(iterator.hasNext()); + if (iterator.hasNext()) { Assert.assertEquals(i, iterator.peekNext()); mark = iterator.next(); } From 175b8093557e32985b9d202e0f2cafab0c67210f Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 6 Nov 2019 12:22:21 -0800 Subject: [PATCH 4/9] remove clone from wrappers since we do not use and is confusing --- .../bitmap/ConcisePeekableIteratorAdapter.java | 11 ----------- .../collections/bitmap/PeekableIteratorAdapter.java | 11 +++-------- 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java index c975bebda27e..a0888bce4362 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java @@ -29,11 +29,6 @@ public class ConcisePeekableIteratorAdapter extends PeekableIteratorAdapter implement this.baseIterator = Preconditions.checkNotNull(iterator, "iterator"); } - PeekableIteratorAdapter(TIntIterator iterator, int mark) - { - this(iterator); - this.mark = mark; - this.advanceIfNeeded(mark); - } - @Override public void advanceIfNeeded(int i) { @@ -61,7 +54,9 @@ public int peekNext() @Override public PeekableIntIterator clone() { - return new PeekableIteratorAdapter(baseIterator.clone(), mark); + throw new UnsupportedOperationException( + "PeekableIteratorAdapter.clone is not implemented, but this should not happen" + ); } @Override From e1a98a7e014acb738c10f1dea162943c71d3ce2f Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 7 Nov 2019 03:22:40 -0800 Subject: [PATCH 5/9] fixes and tests --- ...lHandlingBitmapGetVsIteratorBenchmark.java | 28 ++- .../ConcisePeekableIteratorAdapter.java | 1 - .../bitmap/WrappedConciseBitmap.java | 4 +- .../bitmap/WrappedRoaringBitmap.java | 4 +- .../druid/segment/SimpleAscendingOffset.java | 2 +- .../druid/segment/data/ColumnarDoubles.java | 27 ++- .../druid/segment/data/ColumnarFloats.java | 27 ++- .../druid/segment/data/ColumnarLongs.java | 27 ++- .../segment/vector/VectorSelectorUtils.java | 33 ++- .../druid/query/topn/TopNQueryRunnerTest.java | 13 -- .../org/apache/druid/segment/TestIndex.java | 12 -- .../data/NumericNullColumnSelectorTest.java | 200 ++++++++++++++++++ .../vector/VectorSelectorUtilsTest.java | 124 +++++++++++ 13 files changed, 432 insertions(+), 70 deletions(-) create mode 100644 processing/src/test/java/org/apache/druid/segment/data/NumericNullColumnSelectorTest.java create mode 100644 processing/src/test/java/org/apache/druid/segment/vector/VectorSelectorUtilsTest.java diff --git a/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java b/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java index 4704ee7076d9..9912e1946e47 100644 --- a/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java +++ b/benchmarks/src/main/java/org/apache/druid/benchmark/NullHandlingBitmapGetVsIteratorBenchmark.java @@ -130,7 +130,7 @@ public void get(final Blackhole blackhole) for (int i = pretendFilterOffsets.nextSetBit(0); i >= 0; i = pretendFilterOffsets.nextSetBit(i + 1)) { final boolean isNull = bitmap.get(i); if (isNull) { - blackhole.consume(null); + blackhole.consume(isNull); } else { blackhole.consume(i); } @@ -143,16 +143,23 @@ public void get(final Blackhole blackhole) public void iterator(final Blackhole blackhole) { IntIterator nullIterator = bitmap.iterator(); + int offsetMark = -1; int nullMark = -1; for (int i = pretendFilterOffsets.nextSetBit(0); i >= 0; i = pretendFilterOffsets.nextSetBit(i + 1)) { + // this is totally useless, hopefully this doesn't get optimized out, try to mimic what the selector is doing + if (i < offsetMark) { + nullMark = -1; + nullIterator = bitmap.iterator(); + } + offsetMark = i; while (nullMark < i && nullIterator.hasNext()) { nullMark = nullIterator.next(); } - final boolean isNull = nullMark == i; + final boolean isNull = nullMark == offsetMark; if (isNull) { - blackhole.consume(null); + blackhole.consume(isNull); } else { - blackhole.consume(i); + blackhole.consume(offsetMark); } } } @@ -163,19 +170,26 @@ public void iterator(final Blackhole blackhole) public void peekableIterator(final Blackhole blackhole) { PeekableIntIterator nullIterator = bitmap.peekableIterator(); + int offsetMark = -1; int nullMark = -1; for (int i = pretendFilterOffsets.nextSetBit(0); i >= 0; i = pretendFilterOffsets.nextSetBit(i + 1)) { + // this is totally useless, hopefully this doesn't get optimized out, try to mimic what the selector is doing + if (i < offsetMark) { + nullMark = -1; + nullIterator = bitmap.peekableIterator(); + } + offsetMark = i; if (nullMark < i) { nullIterator.advanceIfNeeded(i); if (nullIterator.hasNext()) { nullMark = nullIterator.next(); } } - final boolean isNull = nullMark == i; + final boolean isNull = nullMark == offsetMark; if (isNull) { - blackhole.consume(i); + blackhole.consume(isNull); } else { - blackhole.consume(null); + blackhole.consume(offsetMark); } } } diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java index a0888bce4362..8ab800af242c 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java @@ -20,7 +20,6 @@ package org.apache.druid.collections.bitmap; import org.apache.druid.extendedset.intset.IntSet; -import org.roaringbitmap.PeekableIntIterator; public class ConcisePeekableIteratorAdapter extends PeekableIteratorAdapter { diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedConciseBitmap.java b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedConciseBitmap.java index 84d151d55b45..1f8816810823 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedConciseBitmap.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedConciseBitmap.java @@ -19,6 +19,7 @@ package org.apache.druid.collections.bitmap; +import com.google.common.annotations.VisibleForTesting; import org.apache.druid.extendedset.intset.ConciseSet; import org.apache.druid.extendedset.intset.ImmutableConciseSet; import org.roaringbitmap.IntIterator; @@ -49,7 +50,8 @@ public WrappedConciseBitmap(ConciseSet conciseSet) this.bitmap = conciseSet; } - ConciseSet getBitmap() + @VisibleForTesting + public ConciseSet getBitmap() { return bitmap; } diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmap.java b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmap.java index 73fc382d558d..ef0bb111409a 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmap.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/WrappedRoaringBitmap.java @@ -19,6 +19,7 @@ package org.apache.druid.collections.bitmap; +import com.google.common.annotations.VisibleForTesting; import org.roaringbitmap.IntIterator; import org.roaringbitmap.PeekableIntIterator; import org.roaringbitmap.RoaringBitmap; @@ -57,7 +58,8 @@ public WrappedRoaringBitmap(boolean compressRunOnSerialization) this.compressRunOnSerialization = compressRunOnSerialization; } - ImmutableBitmap toImmutableBitmap() + @VisibleForTesting + public ImmutableBitmap toImmutableBitmap() { MutableRoaringBitmap bitmap = writer.get().clone(); if (compressRunOnSerialization) { diff --git a/processing/src/main/java/org/apache/druid/segment/SimpleAscendingOffset.java b/processing/src/main/java/org/apache/druid/segment/SimpleAscendingOffset.java index 31b635b5d03b..e730e96b368e 100644 --- a/processing/src/main/java/org/apache/druid/segment/SimpleAscendingOffset.java +++ b/processing/src/main/java/org/apache/druid/segment/SimpleAscendingOffset.java @@ -29,7 +29,7 @@ public class SimpleAscendingOffset extends Offset private final int initialOffset; private int currentOffset; - SimpleAscendingOffset(int rowCount) + public SimpleAscendingOffset(int rowCount) { this(0, rowCount); } diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java index 51627f382840..3544576fdf26 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java @@ -95,20 +95,27 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) } else { class HistoricalDoubleColumnSelectorWithNulls implements DoubleColumnSelector, HistoricalColumnSelector { - private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); private int nullMark = -1; + private int offsetMark = -1; @Override public boolean isNull() { final int i = offset.getOffset(); + if (i < offsetMark) { + // offset was reset, reset iterator state + nullMark = -1; + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = i; if (nullMark < i) { - nullIterator.advanceIfNeeded(i); + nullIterator.advanceIfNeeded(offsetMark); if (nullIterator.hasNext()) { nullMark = nullIterator.next(); } } - return nullMark == i; + return nullMark == offsetMark; } @Override @@ -148,7 +155,8 @@ class ColumnarDoublesVectorValueSelector extends BaseDoubleVectorValueSelector private int id = ReadableVectorOffset.NULL_ID; - private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private int offsetMark = -1; @Nullable private boolean[] nullVector = null; @@ -181,9 +189,18 @@ private void computeVectorsIfNeeded() } if (offset.isContiguous()) { + if (offset.getStartOffset() < offsetMark) { + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = offset.getStartOffset(); ColumnarDoubles.this.get(doubleVector, offset.getStartOffset(), offset.getCurrentVectorSize()); } else { - ColumnarDoubles.this.get(doubleVector, offset.getOffsets(), offset.getCurrentVectorSize()); + final int[] offsets = offset.getOffsets(); + if (offsets[0] < offsetMark) { + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = offsets[0]; + ColumnarDoubles.this.get(doubleVector, offsets, offset.getCurrentVectorSize()); } nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullIterator); diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java index dfd43470c521..483fbde0129b 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java @@ -95,20 +95,27 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) } else { class HistoricalFloatColumnSelectorwithNulls implements FloatColumnSelector, HistoricalColumnSelector { - private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); private int nullMark = -1; + private int offsetMark = -1; @Override public boolean isNull() { final int i = offset.getOffset(); + if (i < offsetMark) { + // offset was reset, reset iterator state + nullMark = -1; + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = i; if (nullMark < i) { - nullIterator.advanceIfNeeded(i); + nullIterator.advanceIfNeeded(offsetMark); if (nullIterator.hasNext()) { nullMark = nullIterator.next(); } } - return nullMark == i; + return nullMark == offsetMark; } @Override @@ -148,7 +155,8 @@ class ColumnarFloatsVectorValueSelector extends BaseFloatVectorValueSelector private int id = ReadableVectorOffset.NULL_ID; - private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private int offsetMark = -1; @Nullable private boolean[] nullVector = null; @@ -181,9 +189,18 @@ private void computeVectorsIfNeeded() } if (offset.isContiguous()) { + if (offset.getStartOffset() < offsetMark) { + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = offset.getStartOffset(); ColumnarFloats.this.get(floatVector, offset.getStartOffset(), offset.getCurrentVectorSize()); } else { - ColumnarFloats.this.get(floatVector, offset.getOffsets(), offset.getCurrentVectorSize()); + final int[] offsets = offset.getOffsets(); + if (offsets[0] < offsetMark) { + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = offsets[0]; + ColumnarFloats.this.get(floatVector, offsets, offset.getCurrentVectorSize()); } nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullIterator); diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java index 01034692461f..3edc28855e54 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java @@ -95,20 +95,27 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) } else { class HistoricalLongColumnSelectorWithNulls implements LongColumnSelector, HistoricalColumnSelector { - private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); private int nullMark = -1; + private int offsetMark = -1; @Override public boolean isNull() { final int i = offset.getOffset(); + if (i < offsetMark) { + // offset was reset, reset iterator state + nullMark = -1; + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = i; if (nullMark < i) { - nullIterator.advanceIfNeeded(i); + nullIterator.advanceIfNeeded(offsetMark); if (nullIterator.hasNext()) { nullMark = nullIterator.next(); } } - return nullMark == i; + return nullMark == offsetMark; } @Override @@ -148,7 +155,8 @@ class ColumnarLongsVectorValueSelector extends BaseLongVectorValueSelector private int id = ReadableVectorOffset.NULL_ID; - private final PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private PeekableIntIterator nullIterator = nullValueBitmap.peekableIterator(); + private int offsetMark = -1; @Nullable private boolean[] nullVector = null; @@ -181,9 +189,18 @@ private void computeVectorsIfNeeded() } if (offset.isContiguous()) { + if (offset.getStartOffset() < offsetMark) { + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = offset.getStartOffset(); ColumnarLongs.this.get(longVector, offset.getStartOffset(), offset.getCurrentVectorSize()); } else { - ColumnarLongs.this.get(longVector, offset.getOffsets(), offset.getCurrentVectorSize()); + final int[] offsets = offset.getOffsets(); + if (offsets[0] < offsetMark) { + nullIterator = nullValueBitmap.peekableIterator(); + } + offsetMark = offsets[0]; + ColumnarLongs.this.get(longVector, offsets, offset.getCurrentVectorSize()); } nullVector = VectorSelectorUtils.populateNullVector(nullVector, offset, nullIterator); diff --git a/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java b/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java index 7d0355da5be0..fa97b79c21d2 100644 --- a/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java +++ b/processing/src/main/java/org/apache/druid/segment/vector/VectorSelectorUtils.java @@ -47,38 +47,33 @@ public static boolean[] populateNullVector( retVal = new boolean[offset.getMaxVectorSize()]; } - int nextNullRow = nullIterator.next(); if (offset.isContiguous()) { final int startOffset = offset.getStartOffset(); nullIterator.advanceIfNeeded(startOffset); + if (!nullIterator.hasNext()) { + return null; + } for (int i = 0; i < offset.getCurrentVectorSize(); i++) { final int row = i + startOffset; - if (row == nextNullRow) { - retVal[i] = true; - if (nullIterator.hasNext()) { - nextNullRow = nullIterator.next(); - } else { - break; - } - } else { - retVal[i] = false; + nullIterator.advanceIfNeeded(row); + if (!nullIterator.hasNext()) { + break; } + retVal[i] = row == nullIterator.peekNext(); } } else { final int[] currentOffsets = offset.getOffsets(); nullIterator.advanceIfNeeded(currentOffsets[0]); + if (!nullIterator.hasNext()) { + return null; + } for (int i = 0; i < offset.getCurrentVectorSize(); i++) { final int row = currentOffsets[i]; - if (row == nextNullRow) { - retVal[i] = true; - if (nullIterator.hasNext()) { - nextNullRow = nullIterator.next(); - } else { - break; - } - } else { - retVal[i] = false; + nullIterator.advanceIfNeeded(row); + if (!nullIterator.hasNext()) { + break; } + retVal[i] = row == nullIterator.peekNext(); } } diff --git a/processing/src/test/java/org/apache/druid/query/topn/TopNQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/topn/TopNQueryRunnerTest.java index c6f894ad5ddd..600329a245c8 100644 --- a/processing/src/test/java/org/apache/druid/query/topn/TopNQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/topn/TopNQueryRunnerTest.java @@ -4292,19 +4292,6 @@ private Sequence> runWithPreMergeAndMerge(TopNQuery quer return runWithMerge(query, ResponseContext.createEmpty()); } - private Sequence> runWithPreMergeAndMerge(TopNQuery query, ResponseContext context) - { - final TopNQueryQueryToolChest chest = new TopNQueryQueryToolChest( - new TopNQueryConfig(), - QueryRunnerTestHelper.noopIntervalChunkingQueryRunnerDecorator() - ); - final QueryRunner> _runner = new FinalizeResultsQueryRunner( - chest.mergeResults(chest.preMergeQueryDecoration(runner)), - chest - ); - return _runner.run(QueryPlus.wrap(query), context); - } - @Test public void testTopNWithExtractionFilterNoExistingValue() { diff --git a/processing/src/test/java/org/apache/druid/segment/TestIndex.java b/processing/src/test/java/org/apache/druid/segment/TestIndex.java index 9470b59903d7..433a72cfa8b1 100644 --- a/processing/src/test/java/org/apache/druid/segment/TestIndex.java +++ b/processing/src/test/java/org/apache/druid/segment/TestIndex.java @@ -84,18 +84,6 @@ public class TestIndex "indexMin", "indexMaxPlusTen" }; - public static final String[] DIMENSIONS = new String[]{ - "market", - "quality", - "qualityLong", - "qualityFloat", - "qualityDouble", - "qualityNumericString", - "placement", - "placementish", - "partial_null_column", - "null_column" - }; public static final List DIMENSION_SCHEMAS = Arrays.asList( new StringDimensionSchema("market"), diff --git a/processing/src/test/java/org/apache/druid/segment/data/NumericNullColumnSelectorTest.java b/processing/src/test/java/org/apache/druid/segment/data/NumericNullColumnSelectorTest.java new file mode 100644 index 000000000000..afafe66f55dd --- /dev/null +++ b/processing/src/test/java/org/apache/druid/segment/data/NumericNullColumnSelectorTest.java @@ -0,0 +1,200 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.segment.data; + +import org.apache.druid.collections.bitmap.ImmutableBitmap; +import org.apache.druid.collections.bitmap.WrappedRoaringBitmap; +import org.apache.druid.segment.BaseNullableColumnValueSelector; +import org.apache.druid.segment.ColumnValueSelector; +import org.apache.druid.segment.SimpleAscendingOffset; +import org.apache.druid.segment.column.DoublesColumn; +import org.apache.druid.segment.column.FloatsColumn; +import org.apache.druid.segment.column.LongsColumn; +import org.apache.druid.segment.vector.NoFilterVectorOffset; +import org.apache.druid.segment.vector.VectorValueSelector; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.util.concurrent.ThreadLocalRandom; + +public class NumericNullColumnSelectorTest +{ + private final int numRows = 1024; + private final int vectorSize = 128; + private final SimpleAscendingOffset offset = new SimpleAscendingOffset(numRows); + private final NoFilterVectorOffset vectorOffset = new NoFilterVectorOffset(vectorSize, 0, numRows); + private ImmutableBitmap bitmap; + + @Before + public void setup() + { + WrappedRoaringBitmap mutable = new WrappedRoaringBitmap(); + for (int i = 0; i < numRows; i++) { + if (ThreadLocalRandom.current().nextDouble(0.0, 1.0) > 0.7) { + mutable.add(i); + } + } + bitmap = mutable.toImmutableBitmap(); + } + + @Test + public void testLongSelectorWithNullsCanResetOffset() + { + ColumnarLongs longs = new ColumnarLongs() + { + @Override + public int size() + { + return numRows; + } + + @Override + public long get(int index) + { + return ThreadLocalRandom.current().nextLong(); + } + + @Override + public void close() + { + + } + }; + LongsColumn columnWithNulls = LongsColumn.create(longs, bitmap); + ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); + assertOffsetCanReset(selector, bitmap, offset); + VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); + assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + } + + @Test + public void testFloatSelectorWithNullsCanResetOffset() + { + ColumnarFloats floats = new ColumnarFloats() + { + @Override + public int size() + { + return numRows; + } + + @Override + public float get(int index) + { + return ThreadLocalRandom.current().nextFloat(); + } + + @Override + public void close() + { + + } + }; + FloatsColumn columnWithNulls = FloatsColumn.create(floats, bitmap); + ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); + assertOffsetCanReset(selector, bitmap, offset); + VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); + assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + } + + @Test + public void testDoubleSelectorWithNullsCanResetOffset() + { + ColumnarDoubles doubles = new ColumnarDoubles() + { + @Override + public int size() + { + return numRows; + } + + @Override + public double get(int index) + { + return ThreadLocalRandom.current().nextDouble(); + } + + @Override + public void close() + { + + } + }; + DoublesColumn columnWithNulls = DoublesColumn.create(doubles, bitmap); + ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); + assertOffsetCanReset(selector, bitmap, offset); + VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); + assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + } + + public static void assertOffsetCanReset( + BaseNullableColumnValueSelector selector, + ImmutableBitmap bitmap, + SimpleAscendingOffset readItAll + ) + { + boolean encounteredNull = false; + while (readItAll.withinBounds()) { + Assert.assertEquals(bitmap.get(readItAll.getOffset()), selector.isNull()); + encounteredNull |= selector.isNull(); + readItAll.increment(); + } + readItAll.reset(); + Assert.assertTrue(encounteredNull); + encounteredNull = false; + while (readItAll.withinBounds()) { + Assert.assertEquals(bitmap.get(readItAll.getOffset()), selector.isNull()); + encounteredNull |= selector.isNull(); + readItAll.increment(); + } + Assert.assertTrue(encounteredNull); + } + + public static void assertVectorOffsetCanReset( + VectorValueSelector selector, + ImmutableBitmap bitmap, + NoFilterVectorOffset readAllVectors + ) + { + boolean encounteredNull = false; + boolean nullVector[]; + while (!readAllVectors.isDone()) { + nullVector = selector.getNullVector(); + for (int i = readAllVectors.getStartOffset(); i < readAllVectors.getCurrentVectorSize(); i++) { + Assert.assertEquals(bitmap.get(readAllVectors.getStartOffset() + i), nullVector[i]); + encounteredNull |= nullVector[i]; + } + readAllVectors.advance(); + } + readAllVectors.reset(); + Assert.assertTrue(encounteredNull); + encounteredNull = false; + while (!readAllVectors.isDone()) { + nullVector = selector.getNullVector(); + for (int i = readAllVectors.getStartOffset(); i < readAllVectors.getCurrentVectorSize(); i++) { + Assert.assertEquals(bitmap.get(readAllVectors.getStartOffset() + i), nullVector[i]); + encounteredNull |= nullVector[i]; + } + readAllVectors.advance(); + } + Assert.assertTrue(encounteredNull); + } +} diff --git a/processing/src/test/java/org/apache/druid/segment/vector/VectorSelectorUtilsTest.java b/processing/src/test/java/org/apache/druid/segment/vector/VectorSelectorUtilsTest.java new file mode 100644 index 000000000000..e8b69735dcc8 --- /dev/null +++ b/processing/src/test/java/org/apache/druid/segment/vector/VectorSelectorUtilsTest.java @@ -0,0 +1,124 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF 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 org.apache.druid.segment.vector; + +import org.apache.druid.collections.IntSetTestUtility; +import org.apache.druid.collections.bitmap.ImmutableBitmap; +import org.apache.druid.collections.bitmap.MutableBitmap; +import org.apache.druid.collections.bitmap.WrappedBitSetBitmap; +import org.apache.druid.collections.bitmap.WrappedConciseBitmap; +import org.apache.druid.collections.bitmap.WrappedImmutableConciseBitmap; +import org.apache.druid.collections.bitmap.WrappedRoaringBitmap; +import org.apache.druid.extendedset.intset.ImmutableConciseSet; +import org.junit.Assert; +import org.junit.Test; +import org.roaringbitmap.PeekableIntIterator; + +import java.util.Set; + +public class VectorSelectorUtilsTest +{ + @Test + public void testBitSetNullVector() + { + final WrappedBitSetBitmap bitmap = new WrappedBitSetBitmap(); + populate(bitmap); + assertNullVector(bitmap); + } + + @Test + public void testConciseMutableNullVector() + { + final WrappedConciseBitmap bitmap = new WrappedConciseBitmap(); + populate(bitmap); + assertNullVector(bitmap); + } + + @Test + public void testConciseImmutableNullVector() + { + final WrappedConciseBitmap bitmap = new WrappedConciseBitmap(); + populate(bitmap); + final ImmutableBitmap immutable = new WrappedImmutableConciseBitmap( + ImmutableConciseSet.newImmutableFromMutable(bitmap.getBitmap()) + ); + assertNullVector(immutable); + } + + @Test + public void testRoaringMutableNullVector() + { + WrappedRoaringBitmap bitmap = new WrappedRoaringBitmap(); + populate(bitmap); + assertNullVector(bitmap); + } + + @Test + public void testRoaringImmutableNullVector() + { + WrappedRoaringBitmap bitmap = new WrappedRoaringBitmap(); + populate(bitmap); + assertNullVector(bitmap.toImmutableBitmap()); + } + + public static void populate(MutableBitmap bitmap) + { + for (int i : IntSetTestUtility.getSetBits()) { + bitmap.add(i); + } + } + + private void assertNullVector(ImmutableBitmap bitmap) + { + PeekableIntIterator iterator = bitmap.peekableIterator(); + Set nulls = IntSetTestUtility.getSetBits(); + final int vectorSize = 32; + final boolean[] nullVector = new boolean[vectorSize]; + ReadableVectorOffset someOffset = new NoFilterVectorOffset(vectorSize, 0, vectorSize); + + VectorSelectorUtils.populateNullVector(nullVector, someOffset, iterator); + for (int i = 0; i < vectorSize; i++) { + Assert.assertEquals(nulls.contains(i), nullVector[i]); + } + + iterator = bitmap.peekableIterator(); + final int smallerVectorSize = 8; + boolean[] smallVector = null; + for (int offset = 0; offset < smallerVectorSize * 4; offset += smallerVectorSize) { + ReadableVectorOffset smallOffset = new NoFilterVectorOffset(smallerVectorSize, offset, offset + smallerVectorSize); + smallVector = VectorSelectorUtils.populateNullVector(smallVector, smallOffset, iterator); + for (int i = 0; i < smallerVectorSize; i++) { + if (smallVector == null) { + Assert.assertFalse(nulls.contains(offset + i)); + } else { + Assert.assertEquals(nulls.contains(offset + i), smallVector[i]); + } + } + smallVector = null; + } + + iterator = bitmap.peekableIterator(); + ReadableVectorOffset allTheNulls = new BitmapVectorOffset(8, bitmap, 0, 22); + smallVector = VectorSelectorUtils.populateNullVector(smallVector, allTheNulls, iterator); + for (int i = 0; i < nulls.size(); i++) { + Assert.assertTrue(smallVector[i]); + } + } +} From 7d9063e1b32e00f66c436970ad39707028c1bb25 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 7 Nov 2019 12:21:24 -0800 Subject: [PATCH 6/9] int instead of Integer --- .../bitmap/ConcisePeekableIteratorAdapter.java | 2 +- .../bitmap/PeekableIteratorAdapter.java | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java index 8ab800af242c..1d56bab0e7e0 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java @@ -31,7 +31,7 @@ public class ConcisePeekableIteratorAdapter extends PeekableIteratorAdapter mark) { + if (mark < i) { baseIterator.skipAllBefore(i); if (baseIterator.hasNext()) { mark = baseIterator.next(); diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java index 3561849790f3..aab3a3efd2f5 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java @@ -26,7 +26,7 @@ public class PeekableIteratorAdapter implements PeekableIntIterator { final TIntIterator baseIterator; - Integer mark; + int mark = -1; PeekableIteratorAdapter(TIntIterator iterator) { @@ -36,7 +36,7 @@ public class PeekableIteratorAdapter implement @Override public void advanceIfNeeded(int i) { - while ((mark == null || mark < i) && baseIterator.hasNext()) { + while (mark < i && baseIterator.hasNext()) { mark = baseIterator.next(); } } @@ -44,8 +44,8 @@ public void advanceIfNeeded(int i) @Override public int peekNext() { - Preconditions.checkArgument(mark != null || baseIterator.hasNext()); - if (mark == null) { + Preconditions.checkArgument(mark > 0 || baseIterator.hasNext()); + if (mark < 0) { mark = baseIterator.next(); } return mark; @@ -62,15 +62,15 @@ public PeekableIntIterator clone() @Override public boolean hasNext() { - return mark != null || baseIterator.hasNext(); + return mark > 0 || baseIterator.hasNext(); } @Override public int next() { - if (mark != null) { + if (mark > 0) { final int currentBit = mark; - mark = null; + mark = -1; return currentBit; } return baseIterator.next(); From d6cd124a6a59a7d74a709acf62cb3dec6c277aac Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 7 Nov 2019 15:30:57 -0800 Subject: [PATCH 7/9] fix it --- .../collections/bitmap/PeekableIteratorAdapter.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java index aab3a3efd2f5..b3ef409e88d4 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java @@ -25,8 +25,9 @@ public class PeekableIteratorAdapter implements PeekableIntIterator { + private static final int NOT_SET = -1; final TIntIterator baseIterator; - int mark = -1; + int mark = NOT_SET; PeekableIteratorAdapter(TIntIterator iterator) { @@ -44,8 +45,7 @@ public void advanceIfNeeded(int i) @Override public int peekNext() { - Preconditions.checkArgument(mark > 0 || baseIterator.hasNext()); - if (mark < 0) { + if (mark == NOT_SET) { mark = baseIterator.next(); } return mark; @@ -62,15 +62,15 @@ public PeekableIntIterator clone() @Override public boolean hasNext() { - return mark > 0 || baseIterator.hasNext(); + return mark != NOT_SET || baseIterator.hasNext(); } @Override public int next() { - if (mark > 0) { + if (mark != NOT_SET) { final int currentBit = mark; - mark = -1; + mark = NOT_SET; return currentBit; } return baseIterator.next(); From 52a6f3bcf175768391dc56b701e3634cfad039bd Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Fri, 8 Nov 2019 16:12:50 -0800 Subject: [PATCH 8/9] fixes, more tests --- .../ConcisePeekableIteratorAdapter.java | 2 + .../bitmap/PeekableIteratorAdapter.java | 5 +- .../druid/segment/data/ColumnarDoubles.java | 6 +- .../druid/segment/data/ColumnarFloats.java | 6 +- .../druid/segment/data/ColumnarLongs.java | 6 +- .../data/NumericNullColumnSelectorTest.java | 270 +++++++++++++----- 6 files changed, 214 insertions(+), 81 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java index 1d56bab0e7e0..e512bb11208b 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/ConcisePeekableIteratorAdapter.java @@ -35,6 +35,8 @@ public void advanceIfNeeded(int i) baseIterator.skipAllBefore(i); if (baseIterator.hasNext()) { mark = baseIterator.next(); + } else { + mark = NOT_SET; } } } diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java b/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java index b3ef409e88d4..fb9d68e91bd0 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java @@ -25,7 +25,7 @@ public class PeekableIteratorAdapter implements PeekableIntIterator { - private static final int NOT_SET = -1; + static final int NOT_SET = -1; final TIntIterator baseIterator; int mark = NOT_SET; @@ -40,6 +40,9 @@ public void advanceIfNeeded(int i) while (mark < i && baseIterator.hasNext()) { mark = baseIterator.next(); } + if (mark < i) { + mark = NOT_SET; + } } @Override diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java index 3544576fdf26..641ec621d966 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarDoubles.java @@ -192,14 +192,14 @@ private void computeVectorsIfNeeded() if (offset.getStartOffset() < offsetMark) { nullIterator = nullValueBitmap.peekableIterator(); } - offsetMark = offset.getStartOffset(); + offsetMark = offset.getStartOffset() + offset.getCurrentVectorSize(); ColumnarDoubles.this.get(doubleVector, offset.getStartOffset(), offset.getCurrentVectorSize()); } else { final int[] offsets = offset.getOffsets(); - if (offsets[0] < offsetMark) { + if (offsets[offsets.length - 1] < offsetMark) { nullIterator = nullValueBitmap.peekableIterator(); } - offsetMark = offsets[0]; + offsetMark = offsets[offsets.length - 1]; ColumnarDoubles.this.get(doubleVector, offsets, offset.getCurrentVectorSize()); } diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java index 483fbde0129b..af850ddde8c5 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarFloats.java @@ -192,14 +192,14 @@ private void computeVectorsIfNeeded() if (offset.getStartOffset() < offsetMark) { nullIterator = nullValueBitmap.peekableIterator(); } - offsetMark = offset.getStartOffset(); + offsetMark = offset.getStartOffset() + offset.getCurrentVectorSize(); ColumnarFloats.this.get(floatVector, offset.getStartOffset(), offset.getCurrentVectorSize()); } else { final int[] offsets = offset.getOffsets(); - if (offsets[0] < offsetMark) { + if (offsets[offsets.length - 1] < offsetMark) { nullIterator = nullValueBitmap.peekableIterator(); } - offsetMark = offsets[0]; + offsetMark = offsets[offsets.length - 1]; ColumnarFloats.this.get(floatVector, offsets, offset.getCurrentVectorSize()); } diff --git a/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java b/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java index 3edc28855e54..850876142350 100644 --- a/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java +++ b/processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java @@ -192,14 +192,14 @@ private void computeVectorsIfNeeded() if (offset.getStartOffset() < offsetMark) { nullIterator = nullValueBitmap.peekableIterator(); } - offsetMark = offset.getStartOffset(); + offsetMark = offset.getStartOffset() + offset.getCurrentVectorSize(); ColumnarLongs.this.get(longVector, offset.getStartOffset(), offset.getCurrentVectorSize()); } else { final int[] offsets = offset.getOffsets(); - if (offsets[0] < offsetMark) { + if (offsets[offsets.length - 1] < offsetMark) { nullIterator = nullValueBitmap.peekableIterator(); } - offsetMark = offsets[0]; + offsetMark = offsets[offsets.length - 1]; ColumnarLongs.this.get(longVector, offsets, offset.getCurrentVectorSize()); } diff --git a/processing/src/test/java/org/apache/druid/segment/data/NumericNullColumnSelectorTest.java b/processing/src/test/java/org/apache/druid/segment/data/NumericNullColumnSelectorTest.java index afafe66f55dd..0966b05c9b14 100644 --- a/processing/src/test/java/org/apache/druid/segment/data/NumericNullColumnSelectorTest.java +++ b/processing/src/test/java/org/apache/druid/segment/data/NumericNullColumnSelectorTest.java @@ -28,124 +28,142 @@ import org.apache.druid.segment.column.FloatsColumn; import org.apache.druid.segment.column.LongsColumn; import org.apache.druid.segment.vector.NoFilterVectorOffset; +import org.apache.druid.segment.vector.VectorOffset; import org.apache.druid.segment.vector.VectorValueSelector; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import java.util.Random; import java.util.concurrent.ThreadLocalRandom; public class NumericNullColumnSelectorTest { + private final int seed = 1337; + private final Random rando = new Random(seed); + private final int numBitmaps = 32; private final int numRows = 1024; private final int vectorSize = 128; private final SimpleAscendingOffset offset = new SimpleAscendingOffset(numRows); private final NoFilterVectorOffset vectorOffset = new NoFilterVectorOffset(vectorSize, 0, numRows); - private ImmutableBitmap bitmap; + private final NoFilterOffsetThatCanBeMangledToTestOverlapping anotherVectorOffset = + new NoFilterOffsetThatCanBeMangledToTestOverlapping(vectorSize, 0, numRows); + private ImmutableBitmap[] bitmaps; @Before public void setup() { - WrappedRoaringBitmap mutable = new WrappedRoaringBitmap(); - for (int i = 0; i < numRows; i++) { - if (ThreadLocalRandom.current().nextDouble(0.0, 1.0) > 0.7) { - mutable.add(i); + bitmaps = new ImmutableBitmap[numBitmaps]; + for (int bitmap = 0; bitmap < numBitmaps; bitmap++) { + WrappedRoaringBitmap mutable = new WrappedRoaringBitmap(); + for (int i = 0; i < numRows; i++) { + if (rando.nextDouble() > 0.2) { + mutable.add(i); + } } + bitmaps[bitmap] = mutable.toImmutableBitmap(); } - bitmap = mutable.toImmutableBitmap(); } @Test public void testLongSelectorWithNullsCanResetOffset() { - ColumnarLongs longs = new ColumnarLongs() - { - @Override - public int size() + for (ImmutableBitmap bitmap : bitmaps) { + ColumnarLongs longs = new ColumnarLongs() { - return numRows; - } + @Override + public int size() + { + return numRows; + } - @Override - public long get(int index) - { - return ThreadLocalRandom.current().nextLong(); - } + @Override + public long get(int index) + { + return ThreadLocalRandom.current().nextLong(); + } - @Override - public void close() - { + @Override + public void close() + { - } - }; - LongsColumn columnWithNulls = LongsColumn.create(longs, bitmap); - ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); - assertOffsetCanReset(selector, bitmap, offset); - VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); - assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + } + }; + LongsColumn columnWithNulls = LongsColumn.create(longs, bitmap); + ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); + assertOffsetCanReset(selector, bitmap, offset); + VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); + assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + } } @Test public void testFloatSelectorWithNullsCanResetOffset() { - ColumnarFloats floats = new ColumnarFloats() - { - @Override - public int size() + for (ImmutableBitmap bitmap : bitmaps) { + ColumnarFloats floats = new ColumnarFloats() { - return numRows; - } + @Override + public int size() + { + return numRows; + } - @Override - public float get(int index) - { - return ThreadLocalRandom.current().nextFloat(); - } + @Override + public float get(int index) + { + return ThreadLocalRandom.current().nextFloat(); + } - @Override - public void close() - { + @Override + public void close() + { - } - }; - FloatsColumn columnWithNulls = FloatsColumn.create(floats, bitmap); - ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); - assertOffsetCanReset(selector, bitmap, offset); - VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); - assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + } + }; + FloatsColumn columnWithNulls = FloatsColumn.create(floats, bitmap); + ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); + assertOffsetCanReset(selector, bitmap, offset); + VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); + assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + VectorValueSelector anotherSelector = columnWithNulls.makeVectorValueSelector(anotherVectorOffset); + assertVectorChillWhenOffsetsOverlap(anotherSelector, bitmap, anotherVectorOffset); + } } @Test public void testDoubleSelectorWithNullsCanResetOffset() { - ColumnarDoubles doubles = new ColumnarDoubles() - { - @Override - public int size() + for (ImmutableBitmap bitmap : bitmaps) { + ColumnarDoubles doubles = new ColumnarDoubles() { - return numRows; - } + @Override + public int size() + { + return numRows; + } - @Override - public double get(int index) - { - return ThreadLocalRandom.current().nextDouble(); - } + @Override + public double get(int index) + { + return ThreadLocalRandom.current().nextDouble(); + } - @Override - public void close() - { + @Override + public void close() + { - } - }; - DoublesColumn columnWithNulls = DoublesColumn.create(doubles, bitmap); - ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); - assertOffsetCanReset(selector, bitmap, offset); - VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); - assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + } + }; + DoublesColumn columnWithNulls = DoublesColumn.create(doubles, bitmap); + ColumnValueSelector selector = columnWithNulls.makeColumnValueSelector(offset); + assertOffsetCanReset(selector, bitmap, offset); + VectorValueSelector vectorSelector = columnWithNulls.makeVectorValueSelector(vectorOffset); + assertVectorOffsetCanReset(vectorSelector, bitmap, vectorOffset); + } } - public static void assertOffsetCanReset( + private static void assertOffsetCanReset( BaseNullableColumnValueSelector selector, ImmutableBitmap bitmap, SimpleAscendingOffset readItAll @@ -166,9 +184,10 @@ public static void assertOffsetCanReset( readItAll.increment(); } Assert.assertTrue(encounteredNull); + readItAll.reset(); } - public static void assertVectorOffsetCanReset( + private static void assertVectorOffsetCanReset( VectorValueSelector selector, ImmutableBitmap bitmap, NoFilterVectorOffset readAllVectors @@ -176,6 +195,8 @@ public static void assertVectorOffsetCanReset( { boolean encounteredNull = false; boolean nullVector[]; + + // read it all, advancing offset while (!readAllVectors.isDone()) { nullVector = selector.getNullVector(); for (int i = readAllVectors.getStartOffset(); i < readAllVectors.getCurrentVectorSize(); i++) { @@ -184,6 +205,7 @@ public static void assertVectorOffsetCanReset( } readAllVectors.advance(); } + // reset and read it all again to make sure matches readAllVectors.reset(); Assert.assertTrue(encounteredNull); encounteredNull = false; @@ -196,5 +218,111 @@ public static void assertVectorOffsetCanReset( readAllVectors.advance(); } Assert.assertTrue(encounteredNull); + readAllVectors.reset(); + } + + public static void assertVectorChillWhenOffsetsOverlap( + VectorValueSelector selector, + ImmutableBitmap bitmap, + NoFilterOffsetThatCanBeMangledToTestOverlapping readAllVectors + ) + { + boolean encounteredNull = false; + boolean nullVector[]; + + // test overlapping reads (should reset iterator anyway) + readAllVectors.mangleOffset(0); + nullVector = selector.getNullVector(); + for (int i = readAllVectors.getStartOffset(); i < readAllVectors.getCurrentVectorSize(); i++) { + Assert.assertEquals(bitmap.get(readAllVectors.getStartOffset() + i), nullVector[i]); + encounteredNull |= nullVector[i]; + } + Assert.assertTrue(encounteredNull); + // this can't currently happen, but we want to protect selectors in case offsets ever try to overlap + readAllVectors.mangleOffset(1); + + nullVector = selector.getNullVector(); + for (int i = readAllVectors.getStartOffset(); i < readAllVectors.getCurrentVectorSize(); i++) { + Assert.assertEquals(bitmap.get(readAllVectors.getStartOffset() + i), nullVector[i]); + encounteredNull |= nullVector[i]; + } + readAllVectors.reset(); + + Assert.assertTrue(encounteredNull); + } + + private static class NoFilterOffsetThatCanBeMangledToTestOverlapping implements VectorOffset + { + private final int maxVectorSize; + private final int start; + private final int end; + private int theOffset; + + NoFilterOffsetThatCanBeMangledToTestOverlapping(final int maxVectorSize, final int start, final int end) + { + this.maxVectorSize = maxVectorSize; + this.start = start; + this.end = end; + reset(); + } + + public void mangleOffset(int replacement) + { + theOffset = replacement; + } + + @Override + public int getId() + { + return theOffset; + } + + @Override + public void advance() + { + theOffset += maxVectorSize; + } + + @Override + public boolean isDone() + { + return theOffset >= end; + } + + @Override + public boolean isContiguous() + { + return true; + } + + @Override + public int getMaxVectorSize() + { + return maxVectorSize; + } + + @Override + public int getCurrentVectorSize() + { + return Math.min(maxVectorSize, end - theOffset); + } + + @Override + public int getStartOffset() + { + return theOffset; + } + + @Override + public int[] getOffsets() + { + throw new UnsupportedOperationException("no filter"); + } + + @Override + public void reset() + { + theOffset = start; + } } } From 0dbc0ccf86fe8ee47ea025dcf122012b0edab1b2 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Mon, 11 Nov 2019 13:46:30 -0800 Subject: [PATCH 9/9] fix --- .../org/apache/druid/collections/bitmap/ImmutableBitmap.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java b/processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java index defd35ae20ad..c73a48c0ae4c 100644 --- a/processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java +++ b/processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java @@ -39,7 +39,7 @@ public interface ImmutableBitmap */ default PeekableIntIterator peekableIterator() { - return new PeekableIteratorAdapter(iterator()); + return new PeekableIteratorAdapter<>(iterator()); } /**