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 19: (10 comments) http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/exprs/expr-test.cc@11625 PS16, Line 11625: > I think we can add a test for the tool_calls error path here: Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/impala-http-handler.cc@204 PS16, Line 204: : if (FLAGS_query_profile_ai_analysis_max_concurrent_runs > 0) { : webserver->RegisterUrlCallback("/query_ai_analysis", "query_ai_analysis.tmpl", : MakeCallback(this, &ImpalaHttpHandler::QueryAiAnalysisHandler), false); : > Don't register these URLs if the query_profile_ai_analysis_max_concurrent_r Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis-test.cc File be/src/service/query-profile-ai-analysis-test.cc: http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis-test.cc@38 PS16, Line 38: > Thanks for adding the tests. Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.h File be/src/service/query-profile-ai-analysis.h: http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.h@30 PS16, Line 30: // Calls the configured AI endpoint with Impala options JSON and returns : // raw response text. : Status CallAiGenerateText(std::string_view impala_options_json, std::string* response); : : // Callback used by the AI loop to execute a named tool call and return tool output JSON. : using AiToolExecutor = std::function<Status( : std::string_view tool_name, std::string_view tool_args_json, std::string* tool_json)>; : : // Runs iterative AI analysis, allowing the model to issue tool calls until it returns : // final analysis text. > Please add some comments describing the functions Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.h@42 PS16, Line 42: > The convention in other files for these types of functions is to use a name Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc File be/src/service/query-profile-ai-analysis.cc: http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@72 PS16, Line 72: "Maximum number of concurrent query-profile AI analysis runs."); > Typically, for these types of new features, we provide a way of turning the Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@118 PS16, Line 118: > Since this is an internal-only function, there is no need to pass in max_co Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@119 PS16, Line 119: // Attempts to reserve one slot for a profile AI analysis run. > No need for this check as the flag validator ensures the number is greater Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@120 PS16, Line 120: static bool TryAcquireAiAnalysisSlot() { : std::lock_guard<std::mutex> l(num_profiles_being_analyzed_lock); : if (num_profiles_being_analyzed : >= FLAGS_query_profile_ai_analysis_max_concurrent_runs) { : return false; : } : ++num_profiles_being_analyzed; : return true; > Since multiple operations need to happen to determine if the max number of Done http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@131 PS16, Line 131: std::lock_guard<std::mutex> l(num_profiles_being_analyzed_lock); > Will need to hold the num_profiles_being_analyzed_lock mutex here. 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: 19 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 04:30:38 +0000 Gerrit-HasComments: Yes
