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 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/24283/6/be/src/service/query-profile-parsing-tools.h File be/src/service/query-profile-parsing-tools.h: http://gerrit.cloudera.org:8080/#/c/24283/6/be/src/service/query-profile-parsing-tools.h@109 PS6, Line 109: ParsedProfile parsed_; : std::vector<std::pair<std::string, const rapidjson::Value*>> fragments_; : std::string query_key_; : std::string query_id_; : int query_type_query_scope_idx_ = 0; explain each with examples where required. For instance is fragments_ all fragments related sections or aggregated ones? Also, sample key/value from profiles will be useful to understand. http://gerrit.cloudera.org:8080/#/c/24283/6/be/src/service/query-profile-parsing-tools.cc File be/src/service/query-profile-parsing-tools.cc: http://gerrit.cloudera.org:8080/#/c/24283/6/be/src/service/query-profile-parsing-tools.cc@50 PS6, Line 50: static const string CONTENTS_KEY = "contents"; : static const string PROFILE_NAME_KEY = "profile_name"; : static const string CHILD_PROFILES_KEY = "child_profiles"; : static const string INFO_STRINGS_KEY = "info_strings"; : static const string NUM_CHILDREN_KEY = "num_children"; : static const string VALUE_KEY = "_value"; : static const string SUMMARY_KEY = "Summary"; : static const string QUERY_TYPE_KEY = "Query Type"; : static const string QUERY_TYPE_QUERY = PrintValue(TStmtType::QUERY); : static const string EVENT_SEQUENCES_KEY = "event_sequences"; : static const string EVENTS_KEY = "events"; : static const string EXECUTION_PROFILE_PREFIX = "Execution Profile"; : static const string PER_NODE_PROFILES_KEY = "Per Node Profiles"; : static const string ERROR_KEY = "error"; : static const string INSTANCE_TOKEN = "Instance"; : static const string FRAGMENT_ID_KEY = "fragment_id"; : static const string TOOL_UNKNOWN_PREFIX = "Unknown tool: "; These should be const char*. You're anyways converting these to cstring using .c_str(). http://gerrit.cloudera.org:8080/#/c/24283/6/be/src/service/query-profile-parsing-tools.cc@410 PS6, Line 410: const int score = DictComplexity(*value); Explain what we use scoring for? http://gerrit.cloudera.org:8080/#/c/24283/6/be/src/service/query-profile-parsing-tools.cc@834 PS6, Line 834: if (key.find(node_id) != string::npos) return &it->value; If you search for node "1", this matches "10", "11", "21", etc. — returning the first substring hit encountered during traversal. The exact-match check on line 833 is good, but the immediate fallback to substring match with no scoring/prioritization will produce wrong results for short node IDs. http://gerrit.cloudera.org:8080/#/c/24283/6/be/src/service/query-profile-parsing-tools.cc@855 PS6, Line 855: transform(upper_key.begin(), upper_key.end(), upper_key.begin(), ::toupper); This allocates + transforms on every recursion level. The pattern should be normalized once and passed as a const string& through the recursion. -- 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: 6 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: Sun, 17 May 2026 06:17:10 +0000 Gerrit-HasComments: Yes
