gortiz commented on code in PR #8979:
URL: https://github.com/apache/pinot/pull/8979#discussion_r982027120
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/MinMaxValueBasedSelectionOrderByCombineOperator.java:
##########
@@ -211,30 +211,51 @@ protected void processSegments() {
((AcquireReleaseColumnsSegmentOperator) operator).release();
}
}
- PriorityQueue<Object[]> selectionResult = (PriorityQueue<Object[]>)
resultsBlock.getRows();
- if (selectionResult != null && selectionResult.size() == _numRowsToKeep)
{
- // Segment result has enough rows, update the boundary value
- assert selectionResult.peek() != null;
- Comparable segmentBoundaryValue = (Comparable)
selectionResult.peek()[0];
- if (boundaryValue == null) {
- boundaryValue = segmentBoundaryValue;
- } else {
- if (asc) {
- if (segmentBoundaryValue.compareTo(boundaryValue) < 0) {
- boundaryValue = segmentBoundaryValue;
- }
- } else {
- if (segmentBoundaryValue.compareTo(boundaryValue) > 0) {
- boundaryValue = segmentBoundaryValue;
- }
- }
- }
+ Collection<Object[]> rows = resultsBlock.getRows();
+ if (rows != null && rows.size() >= _numRowsToKeep) {
+ boundaryValue = updateBoundaryValue(boundaryValue,
extractBoundaryValue(rows), asc);
}
threadBoundaryValue = boundaryValue;
_blockingQueue.offer(resultsBlock);
}
}
+ private Comparable extractBoundaryValue(Collection<Object[]> rows) {
+ if (rows instanceof PriorityQueue) {
+ PriorityQueue<Object[]> selectionResult = (PriorityQueue<Object[]>) rows;
+ assert selectionResult.peek() != null;
+ // at this point, priority queues are sorted in the inverse order
+ return (Comparable) selectionResult.peek()[0];
+ }
+ List<Object[]> selectionResult;
+ if (rows instanceof List) {
+ selectionResult = (List<Object[]>) rows;
+ } else {
+ LOGGER.warn("Selection result stored in an unexpected data structure of
type {}. A copy is needed",
Review Comment:
I mean, right now rows are always List or PriorityQueue. By specifying that
in the constructor, we make it clear. Ideally we should try to move to
something closer to my alternative solution with two subclasses, as it was even
clearer and type safe in the usage side (ie in case we need to add a
`SelectionResultsBlock` whose rows are a set, then the compiler would tell us
that we have to do plan how to get the `extractBoundaryValue` in this new
class).
But yeah, we can also do what you propose. Also, I think list should also
fail if the instance doesn't have a Comparator. Semantically, if there is no
comparator, the list doesn't have to be ordered.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
