gortiz commented on code in PR #16789:
URL: https://github.com/apache/pinot/pull/16789#discussion_r2369243641
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/BitmapDocIdSetOperator.java:
##########
@@ -32,51 +33,50 @@
* <p>Should call {@link #nextBlock()} multiple times until it returns
<code>null</code> (already exhausts all the
* documents) or already gathered enough documents (for selection queries).
*/
-public class BitmapDocIdSetOperator extends BaseOperator<DocIdSetBlock> {
+public class BitmapDocIdSetOperator extends BaseDocIdSetOperator {
private static final String EXPLAIN_NAME = "DOC_ID_SET_BITMAP";
-
- // TODO: Consider using BatchIterator to fill the document ids. Currently
BatchIterator only reads bits for one
- // container instead of trying to fill up the buffer with bits from
multiple containers. If in the future
- // BatchIterator provides an API to fill up the buffer, switch to
BatchIterator.
- private final IntIterator _intIterator;
+ @Nullable
+ private IntIteratorDocIdSetOperator _docIdIteratorOperator = null;
private final int[] _docIdBuffer;
+ private final ImmutableBitmapDataProvider _docIds;
+ private final DidOrder _didOrder;
- public BitmapDocIdSetOperator(ImmutableBitmapDataProvider bitmap) {
- _intIterator = bitmap.getIntIterator();
- _docIdBuffer = new int[DocIdSetPlanNode.MAX_DOC_PER_CALL];
+ public BitmapDocIdSetOperator(ImmutableBitmapDataProvider docIds, int[]
docIdBuffer, DidOrder didOrder) {
+ _docIds = docIds;
+ _docIdBuffer = docIdBuffer;
+ _didOrder = didOrder;
}
- public BitmapDocIdSetOperator(ImmutableBitmapDataProvider bitmap, int
numDocs) {
- _intIterator = bitmap.getIntIterator();
- _docIdBuffer = new int[Math.min(numDocs,
DocIdSetPlanNode.MAX_DOC_PER_CALL)];
+ public static BitmapDocIdSetOperator ascending(ImmutableBitmapDataProvider
docIds) {
+ return ascending(docIds, new int[DocIdSetPlanNode.MAX_DOC_PER_CALL]);
}
- public BitmapDocIdSetOperator(IntIterator intIterator, int[] docIdBuffer) {
- _intIterator = intIterator;
- _docIdBuffer = docIdBuffer;
+ public static BitmapDocIdSetOperator ascending(ImmutableBitmapDataProvider
docIds, int numDocs) {
+ return ascending(docIds, new int[Math.min(numDocs,
DocIdSetPlanNode.MAX_DOC_PER_CALL)]);
}
- public BitmapDocIdSetOperator(ImmutableBitmapDataProvider bitmap, int[]
docIdBuffer) {
- _intIterator = bitmap.getIntIterator();
- _docIdBuffer = docIdBuffer;
+ public static BitmapDocIdSetOperator ascending(ImmutableBitmapDataProvider
docIds, int[] docIdBuffer) {
+ return new BitmapDocIdSetOperator(docIds, docIdBuffer, DidOrder.ASC);
+ }
+
+ public static BitmapDocIdSetOperator descending(ImmutableBitmapDataProvider
docIds, int numDocs) {
+ return descending(docIds, new int[Math.min(numDocs,
DocIdSetPlanNode.MAX_DOC_PER_CALL)]);
+ }
+
+ public static BitmapDocIdSetOperator descending(ImmutableBitmapDataProvider
bitmap, int[] docIdBuffer) {
+ return new BitmapDocIdSetOperator(bitmap, docIdBuffer, DidOrder.ASC);
}
@Override
protected DocIdSetBlock getNextBlock() {
- int bufferSize = _docIdBuffer.length;
- int index = 0;
- while (index < bufferSize && _intIterator.hasNext()) {
- _docIdBuffer[index++] = _intIterator.next();
- }
- if (index > 0) {
- return new DocIdSetBlock(_docIdBuffer, index);
- } else {
- return null;
+ if (_docIdIteratorOperator == null) {
+ IntIterator iterator = _didOrder == DidOrder.ASC ?
_docIds.getIntIterator() : _docIds.getReverseIntIterator();
+ _docIdIteratorOperator = new IntIteratorDocIdSetOperator(iterator,
_docIdBuffer, _didOrder);
Review Comment:
> Another approach is to keep a @Nullable IntIterator _intIterator and
initialize it when calling getNextBlock() if it is not initialized. I feel that
is easier to understand. Creating another operator when invoking getNextBlock()
is a little bit anti-pattern
I don't think that fixes it. The problem is not the late initialization of
the iterator. The problem is not even in `BitmapDocIdSetOperator`. The problem
is that `ExpressionScanDocIdIterator` already uses this antipattern of creating
an `Operator` when `applyAnd` is called. And it creates this
`BitmapDocIdSetOperator` without providing a bitmap but directly an iterator
(which is another antipattern).
Alternatively what we can do is to duplicate the code, removing the
constructor of `BitmapDocIdSetOperator` that accepts an interator (without
bitmap). But keep `IntIteratorDocIdSetOperator` so it can be used by
`ExpressionScanDocIdIterator`.
Wdyt?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
