xiangfu0 commented on code in PR #19816:
URL: https://github.com/apache/pinot/pull/19816#discussion_r4278281207
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -422,6 +422,8 @@ public void mergeInto(BrokerResponseNativeV2 response,
StatMap<?> map) {
@SuppressWarnings("unchecked")
StatMap<LeafOperator.StatKey> stats = (StatMap<LeafOperator.StatKey>)
map;
response.mergeMaxRowsInOperator(stats.getLong(LeafOperator.StatKey.EMITTED_ROWS));
+ // The single-stage threads of the leaf are not covered by the stage
root's allocation
+
response.addTotalMemAllocatedBytes(stats.getLong(LeafOperator.StatKey.THREAD_MEM_ALLOCATED_BYTES));
Review Comment:
The comment here is helpful. Since the leaf and pipeline-breaker
contributions live in `mergeInto` while the stage-root part lives in
`mergeStageStats`, a cross-reference between the two would make all three
sources of the total discoverable from either side.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -1009,6 +1009,21 @@ private Collection<PlanNode>
requestPhysicalPlan(DispatchablePlanFragment fragme
return stagePlans;
}
+ @VisibleForTesting
+ static void mergeStageStats(BrokerResponseNativeV2 brokerResponse,
+ List<MultiStageQueryStats.StageStats.Closed> queryStats) {
+ for (MultiStageQueryStats.StageStats.Closed stageStats : queryStats) {
+ if (stageStats != null) { // for example pipeline breaker may not have
stats
+ stageStats.forEach((type, stats) -> type.mergeInto(brokerResponse,
stats));
+ if (!stageStats.isEmpty()) {
+ // The stage root's allocation covers all the operators of its op
chain
+ brokerResponse.addTotalMemAllocatedBytes(
+
stageStats.getLastOperatorStats().getUnsafe("ALLOCATED_MEMORY_BYTES", 0L));
Review Comment:
Using the string key with `getUnsafe(..., 0L)` is a reasonable way to avoid
the unsafe cast on the heterogeneous root type, and it safely yields 0 for any
root operator whose StatKey lacks the key. Good defensive default — just
flagging it's intentional since the string key loses compile-time checking.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -1009,6 +1009,21 @@ private Collection<PlanNode>
requestPhysicalPlan(DispatchablePlanFragment fragme
return stagePlans;
}
+ @VisibleForTesting
+ static void mergeStageStats(BrokerResponseNativeV2 brokerResponse,
+ List<MultiStageQueryStats.StageStats.Closed> queryStats) {
+ for (MultiStageQueryStats.StageStats.Closed stageStats : queryStats) {
+ if (stageStats != null) { // for example pipeline breaker may not have
stats
+ stageStats.forEach((type, stats) -> type.mergeInto(brokerResponse,
stats));
+ if (!stageStats.isEmpty()) {
+ // The stage root's allocation covers all the operators of its op
chain
Review Comment:
Nice that this reuses the existing stage-root value. Worth a short
cross-reference here: the total is assembled in two places — this stage-root
`allocatedMemoryBytes`, plus the leaf `threadMemAllocatedBytes` and
pipeline-breaker `allocatedMemoryBytes` added over in
`MultiStageOperator.mergeInto`. A one-line pointer would save the next reader
from thinking this method is the whole story.
--
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]
