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