yashmayya commented on code in PR #18736:
URL: https://github.com/apache/pinot/pull/18736#discussion_r3397913142
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/MultiStageStatsTreeBuilder.java:
##########
@@ -63,4 +96,85 @@ public ObjectNode jsonStatsByStage(int stage) {
InStageStatsTreeBuilder treeBuilder = new
InStageStatsTreeBuilder(stageStats, this::jsonStatsByStage);
return planNode.visit(treeBuilder, new InStageStatsTreeBuilder.Context(1));
}
+
+ /**
+ * Renders a stage's explicit stats tree. The shape (and any plugin operator
types) come straight from the decoded
+ * tree; the stage's PlanNode tree is consulted only to resolve which plan
nodes are {@link MailboxReceiveNode}s so
+ * the sender stage's tree can be nested under them, mirroring the legacy
renderer's cross-stage nesting.
+ */
+ private ObjectNode jsonFromStatsTree(StageStatsTreeNode statsTree, int
stage) {
+ Map<Integer, PlanNode> planNodesById = new HashMap<>();
+ PlanNode fragmentRoot = _planNodes.get(stage);
+ if (fragmentRoot != null) {
+ assignPlanNodeIds(fragmentRoot, planNodesById, new int[]{0});
+ }
+ // The stage root is the send operator: its parallelism (when reported,
e.g. by the built-in MAILBOX_SEND stats)
+ // scales cpu-time into wall-clock time for the whole stage. Plugin send
types may not report it; default to 1.
+ int parallelism = 1;
+ JsonNode rootParallelism =
statsTree.getStatMap().asJson().get("parallelism");
+ if (rootParallelism != null) {
+ parallelism = Math.max(1, rootParallelism.asInt(1));
+ }
+ return jsonFromStatsTreeNode(statsTree, planNodesById, parallelism);
+ }
+
+ private ObjectNode jsonFromStatsTreeNode(StageStatsTreeNode node,
Map<Integer, PlanNode> planNodesById,
+ int parallelism) {
+ ObjectNode json = JsonUtils.newObjectNode();
+ json.put("type", node.getType().name());
+ for (Map.Entry<String, JsonNode> entry :
node.getStatMap().asJson().properties()) {
+ json.set(entry.getKey(), entry.getValue());
+ }
+ if (json.get("parallelism") == null) {
+ json.put("parallelism", parallelism);
+ }
+ JsonNode executionTimeMs = json.get("executionTimeMs");
+ if (executionTimeMs != null) {
+ json.put("clockTimeMs", executionTimeMs.asLong(0) / parallelism);
Review Comment:
The explicit-tree renderer drops the `self*` stat fields that the legacy
renderer emits, so turning on stream stats changes the per-stage JSON — which
the PR description says this mode must not do ("bit-compatible … external
tooling parses this format").
`InStageStatsTreeBuilder.selfNode` adds
`selfExecutionTimeMs`/`selfClockTimeMs` (`addClockTimeMs`), `selfAllocatedMB`
(`addSelfAllocatedBytes`) and `selfGcTimeMs` (`addSelfGcTime`) —
parent-minus-children, when non-zero. `jsonFromStatsTreeNode` emits
`clockTimeMs` here but none of the `self*` fields.
It's visible side-by-side in a single stream-mode response (semi-join,
stream stats on): the root stage 0 still falls through to the legacy renderer
(no `StageStatsTreeNode` for the broker-local stage) and carries
`selfExecutionTimeMs`/`selfClockTimeMs`, but every server-reported stage
rendered here does not:
```
MAILBOX_RECEIVE (stage 0, legacy-rendered) selfExecutionTimeMs:223,
selfClockTimeMs:223 (no planNodeIds)
MAILBOX_SEND (stage 1) planNodeIds:[0]
(no self*)
LEAF mytable planNodeIds:[1,2,3,4]
(no self*)
MAILBOX_RECEIVE ...
MAILBOX_SEND (stage 2) planNodeIds:[0]
(no self*)
LEAF daysOfWeek planNodeIds:[1,2]
(no self*)
```
So within one response the broker-local root has the self-time breakdown and
the server stages don't, and vs legacy mode every node would have it. These
feed the controller stage-stats / flamegraph self-time view, so it's a real (if
non-fatal) parity regression — and one the tests don't catch
(`MultiStageStatsTreeBuilderTest` / `StreamStatsReportingIntegrationTest`
assert types/`planNodeIds`/nesting shape, never the `self*` fields).
`jsonFromStatsTreeNode` already builds the child JSON above this point, so
the parent-minus-children `self*` (and unconditional `clockTimeMs`) can be
computed the same way `addClockTimeMs`/`addSelfAllocatedBytes`/`addSelfGcTime`
do. (If dropping them is intentional, worth softening the "bit-compatible"
wording instead.) Either way a test asserting a server stage's node carries
`self*` would lock the parity down.
FWIW I also checked the cross-stage nesting for the dynamic-broadcast build
side (whether the sender stage gets dropped from the tree) — that one's fine:
the `LEAF`'s own `planNodeIds` include the pipeline-breaker
`MailboxReceiveNode`, so the sender stage (`daysOfWeek` above) nests correctly.
Only the `self*` fields diverge.
--
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]
