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 12: (6 comments) http://gerrit.cloudera.org:8080/#/c/24284/11/be/src/service/query-profile-ai-analysis.cc File be/src/service/query-profile-ai-analysis.cc: http://gerrit.cloudera.org:8080/#/c/24284/11/be/src/service/query-profile-ai-analysis.cc@47 PS11, Line 47: > Done Done http://gerrit.cloudera.org:8080/#/c/24284/12/be/src/service/query-profile-ai-analysis.cc File be/src/service/query-profile-ai-analysis.cc: http://gerrit.cloudera.org:8080/#/c/24284/12/be/src/service/query-profile-ai-analysis.cc@480 PS12, Line 480: if (response_doc == nullptr) return false; Let's add DCHECK(response_doc != nullptr); http://gerrit.cloudera.org:8080/#/c/24284/12/be/src/service/query-profile-ai-analysis.cc@490 PS12, Line 490: const string candidate( : response.substr(first_brace, last_brace - first_brace + 1)); : response_doc->Parse(candidate.c_str()); Instead of creating a new string, get a string_view from the substring and parse it: const string_view candidate = response.substr(first_brace, last_brace - first_brace + 1); response_doc->Parse(candidate.data(), candidate.size()); http://gerrit.cloudera.org:8080/#/c/24284/12/be/src/service/query-profile-ai-analysis.cc@519 PS12, Line 519: const Value& assistant_message Since this is an internal-only function and all calls to this function have a Value* for the assistant_message, it would be better for this parameter to have type const Value* http://gerrit.cloudera.org:8080/#/c/24284/12/be/src/service/query-profile-ai-analysis.cc@520 PS12, Line 520: if (assistant_content == nullptr) return false; Let's add DCHECK(assistant_content != nullptr); http://gerrit.cloudera.org:8080/#/c/24284/12/be/src/service/query-profile-ai-analysis.cc@762 PS12, Line 762: string(tool_name) Does a new string need to be initialized here or can the tool_name string_view be passed directly? -- 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: 12 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: Tue, 19 May 2026 21:08:51 +0000 Gerrit-HasComments: Yes
