[GitHub] [pinot] walterddr commented on a diff in pull request #10473: Do not serialize metrics in each Operator

["walterddr (via GitHub)" <gi...@apache.org>]

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


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java:
##########
@@ -151,10 +152,18 @@ protected TransferableBlock getNextBlock() {
     try {
       transferableBlock = _dataTableBlockBaseOperator.nextBlock();
       while (!transferableBlock.isNoOpBlock()) {
-        _exchange.send(transferableBlock);
         if (transferableBlock.isEndOfStreamBlock()) {
-          return transferableBlock;
+          if (transferableBlock.isSuccessfulEndOfStreamBlock()) {
+            TransferableBlock eosBlockWithStats = TransferableBlockUtils.getEndOfStreamTransferableBlock(
+                OperatorUtils.getMetadataFromOperatorStats(_opChainStats.getOperatorStatsMap()));
+            _exchange.send(eosBlockWithStats);
+            return eosBlockWithStats;

Review Comment:
   why do we need to return this instead of just transferableBlock? we don't have any further processing of the stats right?



##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -39,21 +35,18 @@ public abstract class MultiStageOperator implements Operator<TransferableBlock>,
 
   // TODO: Move to OperatorContext class.
   protected final OperatorStats _operatorStats;
-  protected final Map<String, OperatorStats> _operatorStatsMap;
-  private final String _operatorId;
+  protected final String _operatorId;
+  protected OpChainStats _opChainStats;

Review Comment:
   this should be final yes?



##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxReceiveOperator.java:
##########
@@ -215,8 +216,10 @@ protected TransferableBlock getNextBlock() {
                 return block;
               }
             } else {
-              if (!block.getResultMetadata().isEmpty()) {
-                _operatorStatsMap.putAll(block.getResultMetadata());
+              if (_opChainStats != null && !block.getResultMetadata().isEmpty()) {
+                for (Map.Entry<String, OperatorStats> entry : block.getResultMetadata().entrySet()) {
+                  _opChainStats.getOperatorStatsMap().compute(entry.getKey(), (_key, _value) -> entry.getValue());

Review Comment:
   1. this is deserializing the metadata from received metadata from the previous stage and putting them into the current ones. if this is true. when there's no trace enabled, skip this step
   2. where is the opChain stats being encoded? or it is also in the map but when trace is enabled there will be more keys in the map?



##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -39,21 +35,18 @@ public abstract class MultiStageOperator implements Operator<TransferableBlock>,
 
   // TODO: Move to OperatorContext class.
   protected final OperatorStats _operatorStats;

Review Comment:
   do we still need this as final reference? can we also put this in OpChainStats and access via OpChainStats?



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