Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24284 )
Change subject: IMPALA-14963: Query Profile AI Analysis Tool ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc File be/src/service/query-profile-ai-analysis.cc: http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@53 PS4, Line 53: namespace { Declare functions with static keyword instead of using anonymous namespace. http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@55 PS4, Line 55: constexpr int MAX_AGENT_ITERATIONS = 12; Add the "static" keyword to all these constexpr and const definitions. http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@94 PS4, Line 94: const char* PROFILE_TOOLS_JSON = Are the newline characters required? http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@294 PS4, Line 294: struct ChatMessage { Nit: need blank line. http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@301 PS4, Line 301: // Serializes a RapidJSON value into compact JSON text. : string JsonToString(const Value& value) { : StringBuffer buffer; : Writer<StringBuffer> writer(buffer); : value.Accept(writer); : return string(buffer.GetString(), buffer.GetSize()); : } Replace with JsonToString function from json-util.h. http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@308 PS4, Line 308: : // Truncates oversized tool output so model payloads stay bounded. : string TruncateIfNeeded(const string& s) { : if (s.size() <= MAX_TOOL_RESULT_CHARS) return s; : return s.substr(0, MAX_TOOL_RESULT_CHARS) + "\n... [TRUNCATED]"; : } Replace this function with boost::algorithm::erase_tail to do an in-place truncate of tools_json (if needed). That will avoid the unnecessary string copy. http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@387 PS4, Line 387: MemTracker tracker(-1); Initializing this MemTracker with a value of -1 means it can consume unlimited amounts of memory. Instead, construct this MemTracker with ExecEnv::GetInstance()->process_mem_tracker() as its parent. http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@512 PS4, Line 512: if (tool_call_id.empty()) { : LOG(WARNING) << "Skipping tool call '" << tool_name : << "' because model response is missing tool_call id."; : continue; : } Should the model be notified that it hallucinated a tool? http://gerrit.cloudera.org:8080/#/c/24284/4/be/src/service/query-profile-ai-analysis.cc@570 PS4, Line 570: [profile_tool_executor] This code is creating a copy of profile_tool_executor. If that is not the desired behavior, pass by reference: [&profile_tool_executor] -- To view, visit http://gerrit.cloudera.org:8080/24284 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9376b8c56df75032f51abbb76b3ac6927f9107e Gerrit-Change-Number: 24284 Gerrit-PatchSet: 4 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: Wed, 13 May 2026 23:51:26 +0000 Gerrit-HasComments: Yes
