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 11: (17 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" > Nit: includes should be alphabetically ordered within their group, thus thi 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 > This calculation confused me at first. The function doc in query-profile-p I'd like to keep the constant itself as an integer as this is what we already do in the Redaction module and we're planning on providing users with an option to overwrite this limit with a flag in the AI Analysis patch ie. "5%". I rearranged the calculation so it's more clear that we're treating the constant as a percentage. 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, : > Move this function into be/src/util/string-util.h,.cc 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); : > Move this function into be/src/util/string-util.h,.cc 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. : > Move this function into be/src/util/string-util.h,.cc. Also, should not ne Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@178 PS9, Line 178: } > Nit: insert newline to separate this line from the one above it. Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@187 PS9, Line 187: } else { > Add a comment explaining the purpose of this function. Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@189 PS9, Line 189: } > Add a DCHECK(out != nullptr) above this as any situation where out is nullp 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' " : > Separate this into two separate if statements so the returned error can be 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()) { > This function duplicates a lot of functionality in GetSummaryPtr(). Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@345 PS9, Line 345: } > If query profiles should always have a summary section, then add a DCHECK(f 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) { > Is this case expected to happen? If not, add a DCHECK(false) above the ret 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; > Replace with call to IsAllDigits() function. 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 { > Is this case expected to happen? If not, add a DCHECK(false) above the ret 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()) { > The creation of this string isn't necessary. 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]; > If query profiles should always have a summary section, then add a DCHECK(f Done http://gerrit.cloudera.org:8080/#/c/24283/9/be/src/service/query-profile-parsing-tools.cc@1022 PS9, Line 1022: ol call n > Extract "node_id" into a constant. 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: 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:21:22 +0000 Gerrit-HasComments: Yes
