siddharthteotia commented on a change in pull request #6672:
URL: https://github.com/apache/incubator-pinot/pull/6672#discussion_r595592627
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/SelectionOrderByCombineOperator.java
##########
@@ -79,253 +66,70 @@ public String getOperatorName() {
return OPERATOR_NAME;
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p> Execute query on one or more segments in a single thread, and store
multiple intermediate result blocks
+ * into BlockingQueue. Try to use
+ * {@link
org.apache.pinot.core.operator.combine.MinMaxValueBasedSelectionOrderByCombineOperator}
first, which
+ * will skip processing some segments based on the column min/max value.
Otherwise fall back to the default combine
+ * (process all segments).
+ */
@Override
protected IntermediateResultsBlock getNextBlock() {
List<OrderByExpressionContext> orderByExpressions =
_queryContext.getOrderByExpressions();
assert orderByExpressions != null;
if (orderByExpressions.get(0).getExpression().getType() ==
ExpressionContext.Type.IDENTIFIER) {
- return minMaxValueBasedCombine();
- } else {
- return super.getNextBlock();
- }
- }
-
- private IntermediateResultsBlock minMaxValueBasedCombine() {
- List<OrderByExpressionContext> orderByExpressions =
_queryContext.getOrderByExpressions();
- assert orderByExpressions != null;
- int numOrderByExpressions = orderByExpressions.size();
- assert numOrderByExpressions > 0;
- OrderByExpressionContext firstOrderByExpression =
orderByExpressions.get(0);
- assert firstOrderByExpression.getExpression().getType() ==
ExpressionContext.Type.IDENTIFIER;
- String firstOrderByColumn =
firstOrderByExpression.getExpression().getIdentifier();
- boolean asc = firstOrderByExpression.isAsc();
-
- int numOperators = _operators.size();
- List<MinMaxValueContext> minMaxValueContexts = new
ArrayList<>(numOperators);
- for (Operator operator : _operators) {
- minMaxValueContexts.add(new
MinMaxValueContext((SelectionOrderByOperator) operator, firstOrderByColumn));
- }
- try {
- if (asc) {
- // For ascending order, sort on column min value in ascending order
- minMaxValueContexts.sort((o1, o2) -> {
- // Put segments without column min value in the front because we
always need to process them
- if (o1._minValue == null) {
- return o2._minValue == null ? 0 : -1;
- }
- if (o2._minValue == null) {
- return 1;
- }
- return o1._minValue.compareTo(o2._minValue);
- });
- } else {
- // For descending order, sort on column max value in descending order
- minMaxValueContexts.sort((o1, o2) -> {
- // Put segments without column max value in the front because we
always need to process them
- if (o1._maxValue == null) {
- return o2._maxValue == null ? 0 : -1;
- }
- if (o2._maxValue == null) {
- return 1;
- }
- return o2._maxValue.compareTo(o1._maxValue);
- });
+ int numOrderByExpressions = orderByExpressions.size();
Review comment:
(nit) in order for this to be cleaner, I suggest moving all the code
under this if block in a separate function something like
tryMinMaxValueBasedCombine (something similar to old code). This will move the
else part (currently at line 131 all the way up)
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
