Gokul Kolady has posted comments on this change. ( http://gerrit.cloudera.org:8080/24284 )
Change subject: IMPALA-14963: Query Profile AI Analysis Tool ...................................................................... Patch Set 15: (13 comments) Added unit tests where possible http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc File be/src/service/query-profile-ai-analysis.cc: http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@172 PS14, Line 172: "Profile until you are left with the only answer that is plausible. Never make " > seems unnecessary since we have a check for `percent_limit_bytes <= 0` Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@182 PS14, Line 182: "\n\n" > nit. seems able to combine with the upper line Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@184 PS14, Line 184: "evidence, > nit. seems able to combine with the upper line Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@187 PS14, Line 187: "analys > nit. seems able to combine with the upper line Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@190 PS14, Line 190: "performance bottlenecks, and identify " > nit. seems able to combine with the upper line Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@194 PS14, Line 194: c constex > typo Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@194 PS14, Line 194: c constex > nit. bottleneck Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@198 PS14, Line 198: "name": "get_summary", > nit. seems able to combine with the upper line Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@527 PS14, Line 527: return parsed_tools_doc; > replace with simpler non boost alternatives: Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@729 PS14, Line 729: ringVal result = AiFunctions::AiGenerateText( : ctx, null_arg, null_arg, null_arg, null_arg, null_arg, impala_options_val); : if (ctx->has_error()) { : string err = : ctx->error_m > Since the cleanup is called multiple times define scoped exit functions lik Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@726 PS14, Line 726: StringVal impala_options_val(reinterpret_cast<uint8_t*>( : const_cast<char*>(impala_options_json.data())), : impala_options_json.size()); : StringVal result = AiFunctions::AiGenerateText( : ctx, null_arg, null_arg, null_arg, null_arg, null_arg, impala_options_val); : if (ctx->has_error()) { : string err = : ctx->error_msg() == nullptr ? "Unknown AI function error" : ctx->error_msg(); : return Status(err); : } : if (result.is_null) { : return Status("AI function returned null response"); : } : response->assign(reinterpret_cast<char*>(result.ptr), result.len); : return Status::OK(); : } : : // Runs an iterative tool-calling loop until a final analysis response is produced. : Status RunAiAgentWithToolExecutor(string_view initial_summary_json, : const AiToolExecutor& tool_executor, string* analysis) { : if (analysis == nullptr) return Status("analysis output pointer cannot be null"); : if (!tool_executor) return Status("tool executor cannot be empty"); : : vector<ChatMessage> conversation; : conversation.reserve(MAX_AGENT_ITERATIONS * 2); : > We have too many similar cleanup code here, probably we need a common one t Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@913 PS14, Line 913: redactor.redacted_profile_text(), &profile_tool_executor, > Replace ScopedAiAnalysisSlotRelease call here with a MakeScopeExitTrigger a Done http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@927 PS14, Line 927: [&profile_tool_executor]( : string_view tool_name, string_view tool_args_json, string* tool_json) { : if (tool_json == nullptr) { : return Status("tool output pointer cannot be null"); : } : > I noticed in a couple of places that profile_tool_executor returns a JSON s Done -- 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: 15 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: Kurt Deschler <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 20 May 2026 02:03:33 +0000 Gerrit-HasComments: Yes
