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 4: (31 comments) http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools-test.cc File be/src/service/query-profile-parsing-tools-test.cc: http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools-test.cc@57 PS3, Line 57: > Use the std::filesystem functions/classes instead of directly creating path Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.h File be/src/service/query-profile-parsing-tools.h: http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.h@31 PS3, Line 31: typedef rapidjson::Document ParsedProfile; > What is the purpose of this struct? If it is for clarity, use a typedef. Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.h@34 PS3, Line 34: public: : explicit QueryProfileToolAccessor(ParsedProfile parsed); : rapidjson::Value GetSummary(rapidjson::Document::AllocatorType& alloc) const; : rapidjson::Value GetTimeline(rapidjson::Document::AllocatorType& alloc) const; : rapidjson::Value GetCompilation(rapidjson::Document::AllocatorType& alloc) const; : rapidjson::Value GetExecutionProfile(rapidjson::Document::AllocatorType& alloc) const; : rapidjson::Value GetFragmentsOverview(rapidjson::Document::AllocatorType& alloc) const; : rapidjson::Value GetFragment( : const std::string& fragment_id, rapidjson::Document::AllocatorType& alloc) const; : rapidjson::Value GetFragmentInstances( : > Since this class consists of a single public function and no class members, Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.h@68 PS3, Line 68: void F > The static keyword means something different when used in the context of a Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc File be/src/service/query-profile-parsing-tools.cc: http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@50 PS3, Line 50: static const string PROFILE_NAME_KEY = "profile_name"; > No need for anonymous namespace, use static keyword on function declaration Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@55 PS3, Line 55: atic > Add using namespace std; above line 41 and remove all the "std::" prefixes. Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@79 PS3, Line 79: :\b[A-Z_]* > Eliminate magic strings with a static const string declaration. For exampl Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@100 PS3, Line 100: cons > Nit: use const auto& if possible. Same comment for all other places where Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@109 PS3, Line 109: > Does there need to be an else clause that returns an error if info_strings Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@111 PS3, Line 111: // Deep-copies a JSON value into a target allocator. > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@113 PS3, Line 113: return Value(src, alloc); > This potentially performs an unnecessary object instantiation. Instead, ad Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@119 PS3, Line 119: > Does there need to be an else clause that returns an error if child_profile Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@133 PS3, Line 133: static Status ConvertJsonProfile( > Nit: a comment explaining the purpose of this function would be nice to hav Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@159 PS3, Line 159: ey, Clo > Use the TStmtType.QUERY constant instead. Same comment on line 163. Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@168 PS3, Line 168: return Status("'child_profiles' must be an array"); : } : for (const auto& child : profile[CHILD_PROFILES_KEY.c_str()].GetArray()) { : if (!child.IsObject() || !child.HasMember(PROFILE_NAME_KEY.c_str()) : || !child[PROFILE_NAME_KEY.c_str()].IsString()) { : continue; : } : const char* child_name = child[PROFILE_NAME_KEY.c_str()].GetString(); : Value child_value(rapidjson::kObjectType); : > What is the purpose of this second time looping through parsed_.parsed? Removed redundant loop http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@189 PS3, Line 189: if (source_json.HasParseError() || !source_json.IsObject()) { : return Status > Directly return the object instantiation: return Value(rapidjson::kObjectT Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@204 PS3, Line 204: const string profile_name = (*root_profile)[PROFILE_NAME_KEY.c_str()].GetString(); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@205 PS3, Line 205: Value converted(rapidjson::kObjectType); : RETURN_IF_ERROR(ConvertJsonProfile(*root_profile, &converted, alloc)); : out->AddMember(Value(profile_name.c_str(), alloc), converted, alloc); : return Status::OK(); : } : : return Status("Query profile JSON must include either 'contents.profile_name' " : > Declaring a separate function variable is unnecessary. Just move the code Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@219 PS3, Line 219: : int query_type_ > Directly return the object instantiation: return Value(rapidjson::kObjectT Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@234 PS3, Line 234: if (key == query_key_) query_type_query_scope_idx_ = query_type_query_index; > Move the declaration of FID_RE to the top of this file and add flag to make Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@266 PS3, Line 266: string bucket = item.first; > This function loops through fragments three times. Is it possible to reduc Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@317 PS3, Line 317: // Returns a condensed list of all fragments with summary metadata. : Value QueryProfileT > Directly return the object instantiation: return Value(rapidjson::kObjectT Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@409 PS3, Line 409: for (auto it = fragment.MemberBegin(); it != fragment.MemberEnd(); ++it) { : const string > Directly return the object instantiation: return Value(rapidjson::kObjectT Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@418 PS3, Line 418: // Returns a matching execution node, optionally scoped to a fragment. : Value QueryProfileToolAccessor::GetNode(const string& node_id, const string& fragment_id, : Document::AllocatorType& alloc) const { : const Value* search_data = &parsed_; : Document tmp; : tmp.SetObject(); : Value fragment_holder; : if (!fragment_id.empty()) { : > No need for a separate function variable here. Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@540 PS3, Line 540: while (getline(ss, table, ',')) { : string trimmed = table; : StripWhiteSpace(&trimmed); : if (!trimmed.empty()) arr.PushBack(Value(trimmed.c_str(), alloc), alloc); : } : > Once the regex definition has been moved to the top, this function can be e Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@547 PS3, Line 547: : // Returns reservation and estimate fields from summary/plan text. : Value QueryProfileToolAccessor::GetResourceEstimates( : Document::AllocatorType& alloc) const { : Value obj(rapidjson::kObjectType); : const Value* summary = GetSummaryPtr(); : > Once the regex definition has been moved to the top, this function can be e Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@683 PS3, Line 683: > The outer parentheses create an unnecessary capture group. Define the rege Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@705 PS3, Line 705: return out; > Since this function is called multiple times, would it be better to get all Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@765 PS3, Line 765: // Recursively searches for a node by id/key match. > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@853 PS3, Line 853: all > Don't use an anonymous namespace. Declare the function static instead. Done http://gerrit.cloudera.org:8080/#/c/24283/3/be/src/service/query-profile-parsing-tools.cc@906 PS3, Line 906: if (tool_name == "get_resource_estimates") return profile.GetResourceEstimates(alloc); > Nit: This function is also part of the public API. Add a comment stating t 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: 4 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: Thu, 14 May 2026 21:53:27 +0000 Gerrit-HasComments: Yes
