Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/24284 )
Change subject: IMPALA-14963: Query Profile AI Analysis Tool ...................................................................... Patch Set 14: (5 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@172 PS14, Line 172: percent_limit_bytes = std::max<int64_t>(1L, percent_limit_bytes); seems unnecessary since we have a check for `percent_limit_bytes <= 0` http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@194 PS14, Line 194: bottlneck typo http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@527 PS14, Line 527: boost::algorithm::erase_tail(*tool_json, replace with simpler non boost alternatives: ``` tool_json->resize(MAX_TOOL_RESULT_CHARS); ``` http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@729 PS14, Line 729: ctx->impl()->Close(); : delete ctx; : result_pool.FreeAll(); : udf_pool.FreeAll(); : tracker.Close(); Since the cleanup is called multiple times define scoped exit functions like: ``` const auto cleanup = MakeScopeExitTrigger([&]() { result_pool.FreeAll(); udf_pool.FreeAll(); tracker.Close(); }); ........... FunctionContext* ctx = FunctionContextImpl::CreateContext( nullptr, &udf_pool, &result_pool, str_type, arg_types); if (ctx == nullptr) { return Status("Failed to create function context for AI analysis"); } const auto ctx_cleanup = MakeScopeExitTrigger([&]() { ctx->impl()->Close(); delete ctx; }); ``` http://gerrit.cloudera.org:8080/#/c/24284/14/be/src/service/query-profile-ai-analysis.cc@913 PS14, Line 913: ScopedAiAnalysisSlotRelease scoped_ai_analysis_slot_release; Replace ScopedAiAnalysisSlotRelease call here with a MakeScopeExitTrigger also remove ScopedAiAnalysisSlotRelease class's definition ``` const auto slot_release = MakeScopeExitTrigger([]() { ReleaseAiAnalysisSlot(); }); ``` The issue is that ScopedAiAnalysisSlotRelease does nothing in the constructor but its destructor decrements the count and so it can cause issues if it's called without a call to TryAcquireAiAnalysisSlot. -- 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:28:39 +0000 Gerrit-HasComments: Yes
