Jason Fehr 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 11: (21 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@44 PS9, Line 44: #include "runtime/exec-env.h" > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@121 PS9, Line 121: const int64_t process_mem_limit = exec_env->process_mem_tracker()->limit(); : if (process_mem_limit <= 0) return DEFAULT_PARSING_P > I'd like to keep the constant itself as an integer as this is what we alrea Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@127 PS9, Line 127: return min(DEFAULT_PARSING_PROFILE_SIZE_LIMIT_MAX_BYTES, percentage_limit_bytes); : } : : // Deep-copies a JSON value into a target allocator. : static Value CloneValue(const Value& src, Document::AllocatorType& alloc) { : return Value(src, alloc); : } : : static Value MakeJsonString(string_view text, Document::AllocatorType& alloc) { : return Value(text.data(), static_cast<rapidjson::SizeType>(text.size()), alloc); : } : : // Adds a key/value child to an object; duplicate keys are accumulated as arrays. : static void AddChild(Value& parent_obj, const char* key, Value&& value, : > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@143 PS9, Line 143: if (!parent_obj.IsObject()) return; : if (!parent_obj.HasMember(key)) { : parent_obj.AddMember(Value(key, alloc), std::move(value), alloc); : return; : } : : Value& existing = parent_obj[key]; : if (!existing.IsArray()) { : Value arr(rapidjson::kArrayType); : > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@154 PS9, Line 154: } : existing.PushBack(std::move(value), alloc); : } : : // Converts profile keys/children arrays into a normalized JSON object tree. : > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@178 PS9, Line 178: } > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@187 PS9, Line 187: } else { > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@189 PS9, Line 189: } > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@243 PS9, Line 243: : return Status("Query profile JSON must include either 'contents.profile_name' " : > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@338 PS9, Line 338: if (scoped != nullptr && scoped->IsObject()) { > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@345 PS9, Line 345: } > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@372 PS9, Line 372: for (auto it = frag_data->MemberBegin(); it != frag_data->MemberEnd(); ++it) { > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@422 PS9, Line 422: && score > prefix_score) { : prefix_score = score; > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@462 PS9, Line 462: Document::AllocatorType& alloc) const { > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@590 PS9, Line 590: || !(*summary)["Tables Queried"].IsString()) { > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@773 PS9, Line 773: const Value& sequence = sequences[sequence_idx]; > Done Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@1022 PS9, Line 1022: ol call n > Done Done http://gerrit.cloudera.org:8080/#/c/24283/11/be/src/service/query-profile-parsing-tools.cc File be/src/service/query-profile-parsing-tools.cc: http://gerrit.cloudera.org:8080/#/c/24283/11/be/src/service/query-profile-parsing-tools.cc@320 PS11, Line 320: DCHECK(false) << "profile is missing summary section for query " << query_id_; Nit: This DCHECK is not needed since the DCHECK in GetSummaryPtr() covers this case. http://gerrit.cloudera.org:8080/#/c/24283/11/be/src/service/query-profile-parsing-tools.cc@571 PS11, Line 571: FindMember Calling .FindMember() is different from all the other places where .HasMember() is called. Why the difference approach? http://gerrit.cloudera.org:8080/#/c/24283/11/be/src/service/query-profile-parsing-tools.cc@1026 PS11, Line 1026: if (out_val == nullptr) return Status("tool output value pointer cannot be null"); Add a DCHECK(out_val != nullptr) above this line. http://gerrit.cloudera.org:8080/#/c/24283/11/be/src/util/string-util.h File be/src/util/string-util.h: http://gerrit.cloudera.org:8080/#/c/24283/11/be/src/util/string-util.h@50 PS11, Line 50: /// Splits text into lines using '\n' delimiters. Nit: This function differs from other split functions because it creates std::string_view instances against the original std::string_view instead of creating new std::string instances. Update the doc explaining that difference. -- 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: 11 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 20:44:56 +0000 Gerrit-HasComments: Yes
