Jackie-Jiang commented on code in PR #8979:
URL: https://github.com/apache/pinot/pull/8979#discussion_r976047150
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/SelectionResultsBlock.java:
##########
@@ -45,6 +56,18 @@ public Collection<Object[]> getRows() {
return _rows;
}
+ public PriorityQueue<Object[]> getRowsAsPriorityQueue() {
Review Comment:
With the initial results block, we shouldn't need to use this API
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseCombineOperator.java:
##########
@@ -162,7 +162,7 @@ protected void processSegments() {
if (operator instanceof AcquireReleaseColumnsSegmentOperator) {
((AcquireReleaseColumnsSegmentOperator) operator).acquire();
}
- resultsBlock = (T) operator.nextBlock();
+ resultsBlock = createInitialResultBlock((T) operator.nextBlock());
Review Comment:
We should not create initial result block here
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/MinMaxValueBasedSelectionOrderByCombineOperator.java:
##########
@@ -205,7 +205,7 @@ protected void processSegments() {
((AcquireReleaseColumnsSegmentOperator) operator).release();
}
}
- PriorityQueue<Object[]> selectionResult = (PriorityQueue<Object[]>)
resultsBlock.getRows();
+ PriorityQueue<Object[]> selectionResult =
resultsBlock.getRowsAsPriorityQueue();
Review Comment:
We should be able to directly get the boundary value from the list.
Converting it to PQ will eliminate the performance gain of keeping list
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseCombineOperator.java:
##########
@@ -250,6 +250,18 @@ protected boolean isQuerySatisfied(T resultsBlock) {
*/
protected abstract void mergeResultsBlocks(T mergedBlock, T blockToMerge);
+ /**
+ * Converts the given generic IntermediateResultBlock into the specific type
of IntermediateResultBlock that is
+ * supported by this combine operator.
+ *
+ * This operator may return the same object if the runtime type of the given
block already matches which what it is
+ * expected.
+ *
+ * The returned object will usually be the one received as first argument of
+ * {@link #mergeResultsBlocks(BaseResultsBlock, BaseResultsBlock)}.
+ */
+ protected abstract T createInitialResultBlock(BaseResultsBlock block);
Review Comment:
We can provide a default implementation. Also suggest renaming it to be more
specific. We should be able to take T as the input
```suggestion
protected T convertToMergableBlock(T resultsBlock) {
return resultsBlock;
}
```
--
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]
