Jackie-Jiang commented on code in PR #16789:
URL: https://github.com/apache/pinot/pull/16789#discussion_r2369707669
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -576,4 +571,12 @@ private static IllegalArgumentException
longParseException(String optionName, @N
public static String getWorkloadName(Map<String, String> queryOptions) {
return queryOptions.getOrDefault(QueryOptionKey.WORKLOAD_NAME,
CommonConstants.Accounting.DEFAULT_WORKLOAD_NAME);
}
+
+ public static boolean reverseOptimizationEnabled(Map<String, String>
queryOptions) {
+ String value =
queryOptions.get(CommonConstants.Broker.Request.QueryOptionKey.REVERSE_ORDER);
Review Comment:
(minor) You can directly use `QueryOptionKey`
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/DidOrderedOperator.java:
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.pinot.core.operator;
+
+import org.apache.pinot.core.common.Block;
+import org.apache.pinot.core.common.Operator;
+
+
+/// An operator that is bound to a specific segment.
+public interface DidOrderedOperator<T extends Block> extends Operator<T> {
+ /// Returns true if the operator is ordered by docId in the specified order.
+ ///
+ /// Remember that empty operators or operators that return a single row are
considered ordered.
+ boolean isCompatibleWith(DidOrder order);
+
+ enum DidOrder {
Review Comment:
Given `Did` can also represent `DictId`, suggest making it more explicit
`DocIdOrder`
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/DidOrderedOperator.java:
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.pinot.core.operator;
+
+import org.apache.pinot.core.common.Block;
+import org.apache.pinot.core.common.Operator;
+
+
+/// An operator that is bound to a specific segment.
+public interface DidOrderedOperator<T extends Block> extends Operator<T> {
Review Comment:
Consider picking a name more specific about being able to iterate the docs
in reverse order, something like `ReversibleDocIteratorOperator`
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -576,4 +571,12 @@ private static IllegalArgumentException
longParseException(String optionName, @N
public static String getWorkloadName(Map<String, String> queryOptions) {
return queryOptions.getOrDefault(QueryOptionKey.WORKLOAD_NAME,
CommonConstants.Accounting.DEFAULT_WORKLOAD_NAME);
}
+
+ public static boolean reverseOptimizationEnabled(Map<String, String>
queryOptions) {
Review Comment:
(minor) Keep the name consistent? (`isAllowReverseOrder` or
`isReverseOrderAllowed`)
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/SegmentBlockOperator.java:
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.pinot.core.operator;
+
+import org.apache.pinot.core.common.Block;
+import org.apache.pinot.core.common.Operator;
+
+
+/// An operator that is bound to a specific segment.
Review Comment:
Good point. Then let's keep the current way and we can revisit later
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -673,7 +673,8 @@ public static class QueryOptionKey {
// Query option key used to enable a given set of defaultly disabled
rules
public static final String USE_PLANNER_RULES = "usePlannerRules";
- public static final String ORDER_BY_ALGORITHM = "orderByAlgorithm";
+ public static final String REVERSE_ORDER = "allowReverseOrder";
Review Comment:
Make the name consistent?
```suggestion
public static final String ALLOW_REVERSE_ORDER = "allowReverseOrder";
```
##########
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:
I applied a commit to demonstrate the idea. It is optional and either way is
fine
--
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]
