Gokul Kolady 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 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc File be/src/service/query-profile-parsing-tools.cc: http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@54 PS1, Line 54: vector<string> lines; : std::stringstream ss(text); > declarations seems unnecessary here? Done http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@57 PS1, Line 57: while (std::getline(ss, line)) lines.emplace_back(std::move(line)); : return lines; : } : : // Deep-copies a JSON value into a target allocator. : Value CloneValue(const Value& src, Document::AllocatorType& alloc) { : > This should probably be a util function in be/src/util/json-util.h/cc Done http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@66 PS1, Line 66: } // namespace > We should use existing StripWhiteSpace Done http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@75 PS1, Line 75: } : : Document::AllocatorType& alloc = out->parsed.GetAllocator(); : const Value* root_profile = nullptr; : if (source_json.HasMember("contents") && source_json["contents"].IsObject()) { : root_profile = &source_json["contents"]; : > There are lots of local copies here. Let's avoid all unnecessary local copi Done http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@84 PS1, Line 84: } > be/src/util/pretty-printer.h has some utility functions - PrintTimeNs & Pri Done http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@115 PS1, Line 115: AddChild(out, key, value, alloc); > Style guideline for this and other classes. The declarations should probabl Done http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@863 PS1, Line 863: if (tool_name == "get_fragment") { > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@962 PS1, Line 962: > line too long (98 > 90) Done -- 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: 3 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: Wed, 13 May 2026 20:15:27 +0000 Gerrit-HasComments: Yes
