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 14: (20 comments) http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc File be/src/service/query-profile-parsing-tools.cc: http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@116 PS13, Line 116: } > This function duplicates ComputeDefaultProfileSizeLimitBytes in query-profi Added a universal method in a separate file that's called by both redaction and parser http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@366 PS13, Line 366: Document::AllocatorType& alloc) const { : const Value* scoped = GetScopedExecutionProfile(); > Use a std::set or std::multiset as the type for fragments_ to avoid a separ Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@455 PS13, Line 455: const pair<string, const Value*>* rhs) const { > Swap the order of lines 454 and 455. No need to declare the key string_vie Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@481 PS13, Line 481: ll > 5 is the highest value returned by FragmentMatchRank. Should this be == in Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@486 PS13, Line 486: if (!fragment.IsObject()) return; : for (auto it = fragment.MemberBegin(); it != fragment.MemberEnd(); ++it) { > Why is this sort needed? Can a std::set or std::multiset be used instead o Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@515 PS13, Line 515: Value QueryProfileToolAccessor::GetOperator(string_view node_id, string_view fragment_id, > Swap the order of lines 514 and 515. No need to declare the key string_vie Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@527 PS13, Line 527: er > 5 is the highest value returned by FragmentMatchRank. Should this be == in Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@554 PS13, Line 554: const Value* match = SearchOperator(*fragment.second, node_id, &node_key); > No need to declate matched_key, either set a default value of nullptr for m Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@648 PS13, Line 648: e QueryProfileToolAccessor::GetScanNodesSumm > Since the "all" is allocated by the same alloc, it seems we can simply move Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@650 PS13, Line 650: Value arr(rapidjson::kArrayType); : unordered_set<pair<string_view, string_view>, StringViewPairHash> seen; : auto add_scan_summary = [&](string_view fragment_key, string_view node_key, : const Value* node_value) { : if (!seen.emplace(fragment_key, node_key).second) return; : Value summary(rapidjson::kObjectType); : summary.AddMember("fragment", MakeJsonString(fragment_key, alloc), alloc); : summary.AddMember("node", MakeJsonString(node_key, alloc), alloc); : if (node_value != nullptr && node_value->IsObject()) { : > I think the "CloneValue(row, alloc)" can be "Value(move(row))" if we change Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@670 PS13, Line 670: : for (const auto& fragment : fragments_) { : vector<pair<string_view, const Value*>> nodes; : FindNodesByPattern(*fragment.second, "SCAN", &nodes); : for (const auto& node : nodes) { : > Declare this struct in query-profile-parsing-tools.h under the private sect Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@986 PS13, Line 986: const Value& data, vector<pair<string, const Value*>>* out) const { > clang tide error: Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@1129 PS13, Line 1129: bool match = false; : if (normalized_pattern == "SCAN") { > From the reference in https://en.cppreference.com/cpp/string/byte/toupper. Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@1147 PS13, Line 1147: total_ms += atof(string(match[1].first, match[1].second).c_str()) * 3600000. > From the reference in https://en.cppreference.com/cpp/string/byte/toupper. Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@1314 PS13, Line 1314: fragment_id = string_view( > There should be an else clause on this if statement to let the LLM know it Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@1324 PS13, Line 1324: string_view fragment_id; > There should be an else clause on this if statement to let the LLM know it Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@1328 PS13, Line 1328: args["node_id"].GetString(), args["node_id"].GetStringLength()); > There should be an else clause on this if statement to let the LLM know it Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@1339 PS13, Line 1339: if (args.HasMember("instance_id") && args["instance_id"].IsString()) { > There should be an else clause on this if statement to let the LLM know it Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@1343 PS13, Line 1343: return Status( > There should be an else clause on this if statement to let the LLM know it Done http://gerrit.cloudera.org:8080/#/c/24283/13/be/src/service/query-profile-parsing-tools.cc@1347 PS13, Line 1347: return Status::OK(); > There should be an else clause on this if statement to let the LLM know it 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: 14 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: Tue, 19 May 2026 04:03:00 +0000 Gerrit-HasComments: Yes
