Abhishek Rawat 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 2: (6 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: string Trim(const string& s); : vector<string> SplitLines(const string& text); declarations seems unnecessary here? http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@57 PS1, Line 57: // Serializes a RapidJSON value to compact JSON. : string JsonToString(const Value& value) { : StringBuffer buffer; : Writer<StringBuffer> writer(buffer); : value.Accept(writer); : return string(buffer.GetString(), buffer.GetSize()); : } This should probably be a util function in be/src/util/json-util.h/cc http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@66 PS1, Line 66: string Trim(const string& s) { We should use existing StripWhiteSpace http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@75 PS1, Line 75: vector<string> SplitLines(const string& text) { : vector<string> lines; : std::stringstream ss(text); : string line; : while (std::getline(ss, line)) lines.push_back(line); : return lines; : } There are lots of local copies here. Let's avoid all unnecessary local copies in this function. http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@84 PS1, Line 84: string FormatDurationNs(int64_t nanos) { be/src/util/pretty-printer.h has some utility functions - PrintTimeNs & PrintTimeMs, see if you can use them instead? http://gerrit.cloudera.org:8080/#/c/24283/1/be/src/service/query-profile-parsing-tools.cc@115 PS1, Line 115: class ImpalaProfileParser { Style guideline for this and other classes. The declarations should probably be in header file and we can put definitions in cc. -- 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: 2 Gerrit-Owner: Gokul Kolady <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[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 17:29:22 +0000 Gerrit-HasComments: Yes
