siddharthteotia commented on a change in pull request #6320:
URL: https://github.com/apache/incubator-pinot/pull/6320#discussion_r536517138
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/inv/OnHeapBitmapInvertedIndexCreator.java
##########
@@ -19,68 +19,85 @@
package org.apache.pinot.core.segment.creator.impl.inv;
import com.google.common.base.Preconditions;
-import java.io.BufferedOutputStream;
-import java.io.DataOutputStream;
+
+
import java.io.File;
-import java.io.FileOutputStream;
import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+
import org.apache.commons.io.FileUtils;
import
org.apache.pinot.core.segment.creator.DictionaryBasedInvertedIndexCreator;
import org.apache.pinot.core.segment.creator.impl.V1Constants;
+import org.apache.pinot.core.util.CleanerUtil;
+import org.roaringbitmap.RoaringBitmapWriter;
import org.roaringbitmap.buffer.MutableRoaringBitmap;
+import static java.nio.ByteOrder.LITTLE_ENDIAN;
+
/**
* Implementation of {@link DictionaryBasedInvertedIndexCreator} that uses
on-heap memory.
*/
public final class OnHeapBitmapInvertedIndexCreator implements
DictionaryBasedInvertedIndexCreator {
private final File _invertedIndexFile;
- private final MutableRoaringBitmap[] _bitmaps;
+ private final RoaringBitmapWriter<MutableRoaringBitmap>[] _bitmapWriters;
private int _nextDocId;
+ @SuppressWarnings("unchecked")
public OnHeapBitmapInvertedIndexCreator(File indexDir, String columnName,
int cardinality) {
_invertedIndexFile = new File(indexDir, columnName +
V1Constants.Indexes.BITMAP_INVERTED_INDEX_FILE_EXTENSION);
- _bitmaps = new MutableRoaringBitmap[cardinality];
+ _bitmapWriters = new RoaringBitmapWriter[cardinality];
for (int i = 0; i < cardinality; i++) {
- _bitmaps[i] = new MutableRoaringBitmap();
+ _bitmapWriters[i] = RoaringBitmapWriter.bufferWriter().get();
}
}
@Override
public void add(int dictId) {
- _bitmaps[dictId].add(_nextDocId++);
+ _bitmapWriters[dictId].add(_nextDocId++);
}
@Override
public void add(int[] dictIds, int length) {
for (int i = 0; i < length; i++) {
- _bitmaps[dictIds[i]].add(_nextDocId);
+ _bitmapWriters[dictIds[i]].add(_nextDocId);
}
_nextDocId++;
}
@Override
public void seal()
throws IOException {
- try (DataOutputStream out = new DataOutputStream(
- new BufferedOutputStream(new FileOutputStream(_invertedIndexFile)))) {
+ // calculate file size
+ int size = (_bitmapWriters.length + 1) * Integer.BYTES;
+ for (RoaringBitmapWriter<MutableRoaringBitmap> writer : _bitmapWriters) {
+ size += writer.get().serializedSizeInBytes();
+ // Check for int overflow
+ Preconditions.checkState(size > 0, "Inverted index file: %s exceeds 2GB
limit", _invertedIndexFile);
+ }
+ ByteBuffer buffer = null;
+ try (FileChannel channel = new RandomAccessFile(_invertedIndexFile,
"rw").getChannel()) {
+ buffer = channel.map(FileChannel.MapMode.READ_WRITE, 0,
size).order(LITTLE_ENDIAN);
Review comment:
I am assuming the choice of LITTLE_ENDIAN comes from the fact that index
generation (typically happening on x86 machines) will not require byte swap
when writing the index?
When we load the index, we specifically use BIG_ENDIAN byte order because
throughout the index generation code we use BIG_ENDIAN. This works with
overhead of byte swap for both read and write.
I am confused how is this change expected to work today for the reader? The
reader will load the file specifying the order as BIG_ENDIAN. This means every
read operation will swap the bytes on x86. However, with this change index file
is generated in LE format which doesn't require any swapping. So swapping bytes
during inverted index read will result in incorrect data imo. I am surprised
that tests are passing.
See the code in index buffer loader. This code doesn't know index file was
written in LE format.
```
// Backward-compatible: index file is always big-endian
PinotDataBuffer buffer;
if (readMode == ReadMode.heap) {
buffer = PinotDataBuffer.loadFile(indexFile, fromFilePos, size,
ByteOrder.BIG_ENDIAN, context);
} else {
buffer = PinotDataBuffer.mapFile(indexFile, true, fromFilePos, size,
ByteOrder.BIG_ENDIAN, context);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
