[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10058: [refactor] move min/max combine out

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on code in PR #10058:
URL: https://github.com/apache/pinot/pull/10058#discussion_r1061936474


##########
pinot-core/src/main/java/org/apache/pinot/core/plan/CombinePlanNode.java:
##########
@@ -190,6 +194,13 @@ public List<Operator> callJob() {
         return new SelectionOnlyCombineOperator(operators, _queryContext, _executorService);
       } else {
         // Selection order-by
+        if (QueryContextUtils.isMinMaxBasedSelectionOrderBy(_queryContext)) {

Review Comment:
   IMO no need to put it into `QueryContextUtils`. This logic should be within the combine plan



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/SelectionOrderByCombineOperator.java:
##########
@@ -66,25 +64,12 @@ public String toExplainString() {
    * {@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).
+   * into BlockingQueue.
    */
   @Override
   protected BaseResultsBlock getNextBlock() {

Review Comment:
   Let's remove this override completely



##########
pinot-core/src/main/java/org/apache/pinot/core/plan/CombinePlanNode.java:
##########
@@ -190,6 +194,13 @@ public List<Operator> callJob() {
         return new SelectionOnlyCombineOperator(operators, _queryContext, _executorService);
       } else {
         // Selection order-by
+        if (QueryContextUtils.isMinMaxBasedSelectionOrderBy(_queryContext)) {
+          try {

Review Comment:
   We don't need this try-catch as the constructor won't throw exception



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org