Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/24284 )
Change subject: IMPALA-14963: Query Profile AI Analysis Tool ...................................................................... Patch Set 14: (8 comments) 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@182 PS14, Line 182: "with " nit. seems able to combine with the upper line http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@184 PS14, Line 184: "instead " nit. seems able to combine with the upper line http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@187 PS14, Line 187: "tool " nit. seems able to combine with the upper line http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@190 PS14, Line 190: "is optimal for the given query. Ensure that the statistics are computed and " nit. seems able to combine with the upper line http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@194 PS14, Line 194: bottlneck nit. bottleneck By the way, it doesn't seem to need a new line here? http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@198 PS14, Line 198: "your final conclusion was true or false." nit. seems able to combine with the upper line http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@726 PS14, Line 726: if (ctx->has_error()) { : string err = : ctx->error_msg() == nullptr ? "Unknown AI function error" : ctx->error_msg(); : ctx->impl()->Close(); : delete ctx; : result_pool.FreeAll(); : udf_pool.FreeAll(); : tracker.Close(); : return Status(err); : } : if (result.is_null) { : ctx->impl()->Close(); : delete ctx; : result_pool.FreeAll(); : udf_pool.FreeAll(); : tracker.Close(); : return Status("AI function returned null response"); : } : response->assign(reinterpret_cast<char*>(result.ptr), result.len); : ctx->impl()->Close(); : delete ctx; : result_pool.FreeAll(); : udf_pool.FreeAll(); : tracker.Close(); : return Status::OK(); : } We have too many similar cleanup code here, probably we need a common one to call. Maybe add MakeScopeExitTrigger to handle the cleanup: auto cleanup = MakeScopeExitTrigger([&]() { if (ctx != nullptr) { ctx->impl()->Close(); delete ctx; } result_pool.FreeAll(); udf_pool.FreeAll(); tracker.Close(); }); http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@927 PS14, Line 927: RETURN_IF_ERROR(profile_tool_executor("get_summary", "", &summary_tool_output_json)); : Document summary_tool_output_doc; : summary_tool_output_doc.Parse(summary_tool_output_json.c_str()); : if (summary_tool_output_doc.HasParseError()) { : return Status("get_summary tool output must be valid JSON"); : } I noticed in a couple of places that profile_tool_executor returns a JSON string, but the output is immediately parsed back into a Document. Would it be better for profile_tool_executor to just accept a local Document/Value pointer as the output? -- 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: 14 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 00:06:21 +0000 Gerrit-HasComments: Yes
