Yida Wu 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 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.h File be/src/service/query-profile-parsing-tools.h: http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.h@33 PS5, Line 33: class QueryProfileToolAccessor { Could you please add a detailed comment at the top of this class or maybe to the public functions? It should explain: Purpose: What the class does and the main problem it solves. Usage: How to use it (for example, creating it directly vs. using CreateQueryProfileToolExecutorForProfile). Inputs: The expected input format. What does a valid JSON profile look like, and what different structures does it accept? A short simple example of the high-level input JSON structure would be good to have here. Outputs: The output rules. When does it return an error or an empty object? Are the outputs always JSON objects, or arrays? http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.h@64 PS5, Line 64: void GatherItems(const rapidjson::Value& data, : std::vector<std::pair<std::string, const rapidjson::Value*>>* out); This can be a const function http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.cc File be/src/service/query-profile-parsing-tools.cc: http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.cc@119 PS5, Line 119: if (!parent_obj.IsObject()) return; It looks we should add a DCHCK for this, but it seems fine we have this here with logging. " http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.cc@224 PS5, Line 224: const string key = it->name.GetString(); Can we use the string_view here? const string_view key(it->name.GetString(), it->name.GetStringLength()); http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.cc@255 PS5, Line 255: if (++used[unique_key] > 1) { : unique_key = key + " [" + to_string(used[key]) + "]"; : } Maybe we can optimize the code like this, reduce the map search cost and string allocation cost. " const int count = ++used[key]; const string unique_key = count > 1 ? key + " [" + to_string(count) + "]" : key;" http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.cc@412 PS5, Line 412: const string key = it->name.GetString(); Can we use the string_view here? const string_view key(it->name.GetString(), it->name.GetStringLength()); http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.cc@459 PS5, Line 459: const string key = it->name.GetString(); Can we use the string_view here? const string_view key(it->name.GetString(), it->name.GetStringLength()); http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.cc@752 PS5, Line 752: void QueryProfileToolAccessor::GatherItems( This function should be "const void QueryProfileToolAccessor::GatherItems" http://gerrit.cloudera.org:8080/#/c/24283/5/be/src/service/query-profile-parsing-tools.cc@756 PS5, Line 756: const string key = it->name.GetString(); Can we use the string_view here? const string_view key(it->name.GetString(), it->name.GetStringLength()); -- 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: 5 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: Sat, 16 May 2026 03:55:40 +0000 Gerrit-HasComments: Yes
