walterddr commented on code in PR #10299:
URL: https://github.com/apache/pinot/pull/10299#discussion_r1113260236
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxSendOperator.java:
##########
@@ -82,7 +82,7 @@ public MailboxSendOperator(MailboxService<TransferableBlock>
mailboxService,
RelDistribution.Type exchangeType, KeySelector<Object[], Object[]>
keySelector,
MailboxIdGenerator mailboxIdGenerator, BlockExchangeFactory
blockExchangeFactory, long jobId, int senderStageId,
int receiverStageId) {
- super(jobId, senderStageId);
+ super(jobId, senderStageId, null);
Review Comment:
why are we setting it to null?
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -111,6 +112,37 @@ protected List<Object[]> queryRunner(String sql) {
queryPlan.getQueryResultFields(),
queryPlan.getQueryStageMap().get(0).getDataSchema()).getRows();
}
+ protected List<Object[]> queryRunner(String sql, ExecutionStatsAggregator
executionStatsAggregator) {
Review Comment:
simply make the normal queryRunner API call this one with a null
executionStatsAggregator?
the only difference is the last step when reduceMailboxReceive yes?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -70,7 +74,12 @@ public TransferableBlock nextBlock() {
}
if (!_operatorStats.getExecutionStats().isEmpty()) {
- String operatorId = Joiner.on("_").join(toExplainString(),
_requestId, _stageId);
+ String operatorId;
+ if (_serverAddress != null) {
+ operatorId = Joiner.on("_").join(toExplainString(), _requestId,
_stageId, _serverAddress);
+ } else {
+ operatorId = Joiner.on("_").join(toExplainString(), _requestId,
_stageId);
+ }
Review Comment:
these can be moved to constructor right?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -70,7 +74,12 @@ public TransferableBlock nextBlock() {
}
if (!_operatorStats.getExecutionStats().isEmpty()) {
Review Comment:
the TODO above can be removed ^
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MailboxReceiveOperator.java:
##########
@@ -164,8 +164,16 @@ protected TransferableBlock getNextBlock() {
return block;
} else {
if (!block.getResultMetadata().isEmpty()) {
- _operatorStats.clearExecutionStats();
_operatorStatsMap.putAll(block.getResultMetadata());
+// for(String operatorId: block.getResultMetadata().keySet()) {
+// //TODO: Remove this hack!
+// if (!operatorId.contains("serverId")) {
+// _operatorStatsMap.put(operatorId + "_serverId" +
mailboxId.getFromHost(),
+// block.getResultMetadata().get(operatorId));
+// } else {
+// _operatorStatsMap.put(operatorId,
block.getResultMetadata().get(operatorId));
+// }
+// }
Review Comment:
remove
##########
pinot-query-runtime/src/test/resources/metadata_queries/BasicQuery.json:
##########
@@ -0,0 +1,68 @@
+{
Review Comment:
why are we duplicating the ResourceBasedTest?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
