Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/24283 )
Change subject: IMPALA-14962: Query Profile Parser and Section Retrieval Interface ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc File be/src/service/query-profile-parsing-tools.cc: http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@303 PS9, Line 303: GatherItems(*scoped_exec, &all_items); : for (const auto& item : all_items) { : const string& key = item.first; : if (!IsFragmentKey(key)) continue; : const int count = ++used[key]; : string unique_key = key; : if (count > 1) { : unique_key.append(" [").append(std::to_string(count)).append("]"); : } : fragments_.emplace_back(move(unique_key), item.second); : } It seems that we only use GatherItems to collect fragment nodes only, how about changing to GatherFragments? So collecting fragment nodes only could help some performance. And we can remove the "if (!IsFragmentKey(key)) continue;" here http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@839 PS9, Line 839: out->emplace_back(key, &it->value); If changing to GatherFragments, we can change here: "if (IsFragmentKey(key)) { out->emplace_back(string(key), &it->value); }" http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@871 PS9, Line 871: if (numeric_node_id && node_id.size() <= 2) return 0; Do we really need to limit the "node_id.size() <= 2", it seems fine to do "if (numeric_node_id) return 0;" http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@1088 PS9, Line 1088: Value err(rapidjson::kObjectType); : const string unknown_tool = TOOL_UNKNOWN_PREFIX + string(tool_name); : err.AddMember(Value(ERROR_KEY, alloc), : MakeJsonString(unknown_tool, alloc), alloc); : return err; It seems better to return Status to handle errors, and otherwise the caller would return a Status::OK with an error JSON. How about we change the function to: "static Status ExecuteTool(..., Value* out_val)" And change here to: "return Status(TOOL_UNKNOWN_PREFIX + string(tool_name));" http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@1138 PS9, Line 1138: Value out = ExecuteTool(tool_name, args_doc, *profile, out_doc.GetAllocator()); If we change ExecuteTool to return Status, we can change here: " Value out; RETURN_IF_ERROR(ExecuteTool(tool_name, args_doc, *profile, out_doc.GetAllocator(), &out)); " -- To view, visit http://gerrit.cloudera.org:8080/24283 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d9c9e735aa089bb243b07af421553002a465a88 Gerrit-Change-Number: 24283 Gerrit-PatchSet: 9 Gerrit-Owner: Gokul Kolady <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Gokul Kolady <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Mon, 18 May 2026 05:35:41 +0000 Gerrit-HasComments: Yes
