[GitHub] [pinot] walterddr commented on a diff in pull request #9282: [Cleanup] Break IntermediateResultsBlock into different results blocks

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

walterddr commented on code in PR #9282:
URL: https://github.com/apache/pinot/pull/9282#discussion_r957726100


##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -273,14 +273,13 @@ public Plan makeStreamingInstancePlan(List<IndexSegment> indexSegments, QueryCon
     if (QueryContextUtils.isSelectionOnlyQuery(queryContext)) {
       // selection-only is streamed in StreamingSelectionPlanNode --> here only metadata block is returned.
       return new GlobalPlanImplV0(
-          new InstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext,
-              serverMetrics));
+          new InstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext));
     } else {
       // non-selection-only requires a StreamingInstanceResponsePlanNode to stream data block back and metadata block
       // as final return.
       return new GlobalPlanImplV0(
-          new StreamingInstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(),
-              streamObserver, queryContext, serverMetrics));
+          new StreamingInstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext,
+              streamObserver));

Review Comment:
   why are we switching the order of the stream observer and query context?



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java:
##########
@@ -34,24 +34,21 @@
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
-
   private static final String EXPLAIN_NAME = "INSTANCE_RESPONSE";
 
-  private final BaseCombineOperator _combineOperator;
+  private final BaseCombineOperator<?> _combineOperator;

Review Comment:
   should we also put type generic here for InstanceResponseOperator<T extends BaseResultBlock> ? 
   eventually resultsBlock.getDataTable(QueryContext) is implemented at each individual `T` correct?



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java:
##########
@@ -34,24 +34,21 @@
 
 
 public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
-
   private static final String EXPLAIN_NAME = "INSTANCE_RESPONSE";
 
-  private final BaseCombineOperator _combineOperator;
+  private final BaseCombineOperator<?> _combineOperator;
   private final List<IndexSegment> _indexSegments;
   private final List<FetchContext> _fetchContexts;
   private final int _fetchContextSize;
   private final QueryContext _queryContext;
-  private final ServerMetrics _serverMetrics;
 
-  public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
-      List<FetchContext> fetchContexts, QueryContext queryContext, ServerMetrics serverMetrics) {
+  public InstanceResponseOperator(BaseCombineOperator<?> combinedOperator, List<IndexSegment> indexSegments,
+      List<FetchContext> fetchContexts, QueryContext queryContext) {
     _combineOperator = combinedOperator;

Review Comment:
   nit: this and all others, `combinedOperator` -> `combineOperator`



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/AcquireReleaseColumnsSegmentOperator.java:
##########
@@ -58,8 +58,8 @@ public AcquireReleaseColumnsSegmentOperator(PlanNode planNode, IndexSegment inde
    * Runs the planNode to get the childOperator, and then proceeds with execution.
    */
   @Override
-  protected IntermediateResultsBlock getNextBlock() {
-    _childOperator = (Operator<IntermediateResultsBlock>) _planNode.run();
+  protected BaseResultsBlock getNextBlock() {
+    _childOperator = (Operator<BaseResultsBlock>) _planNode.run();
     return _childOperator.nextBlock();

Review Comment:
   unrelated to this PR but why are we running the plannode everytime to get child operator? i know it will access segment before running the plan, but does it change on a per segment bases?



-- 
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