Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/22130 )
Change subject: IMPALA-13565: Add general AI support to ai_generate_text ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/22130/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22130/3//COMMIT_MSG@7 PS3, Line 7: IMPALA-13565: Add general AI support to ai_generate_text Maybe reword this to "Add general AI platform support to ai_generate_text" http://gerrit.cloudera.org:8080/#/c/22130/3//COMMIT_MSG@12 PS3, Line 12: ai_api_additional_sites, allowing Impala to access additional Should we rename "ai_api_additional_sites" to "ai_api_additional_platforms"? If you go forward with changing 'sites' to 'platforms' you also need to update it in other places. http://gerrit.cloudera.org:8080/#/c/22130/3/be/src/exprs/ai-functions-ir.cc File be/src/exprs/ai-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/22130/3/be/src/exprs/ai-functions-ir.cc@90 PS3, Line 90: * The sties are loaded and parsed once to optimize the efficiency. typo: "sties" "optimize for efficiency" http://gerrit.cloudera.org:8080/#/c/22130/3/be/src/exprs/ai-functions.h File be/src/exprs/ai-functions.h: http://gerrit.cloudera.org:8080/#/c/22130/3/be/src/exprs/ai-functions.h@54 PS3, Line 54: UNSUPPORTED_STANDARD, Maybe skip the '_STANDARD' i.e enum API_STANDARD { /// Unsupported standard UNSUPPORTED, /// OpenAI standard OPEN_AI }; http://gerrit.cloudera.org:8080/#/c/22130/3/be/src/exprs/ai-functions.inline.h File be/src/exprs/ai-functions.inline.h: http://gerrit.cloudera.org:8080/#/c/22130/3/be/src/exprs/ai-functions.inline.h@91 PS3, Line 91: Document document; Instead of creating a local Document here we can pass Document reference from the caller. We can also avoid deep copying ai_platform_params and ai_custom_payload in AiFunctionParams and maybe make these string_views or string references? struct AiFunctionsParams { // AI platform parameters as a JSON string string ai_platform_params; // Default of api statndard is OPEN_AI AiFunctions::API_STANDARD api_standard = AiFunctions::API_STANDARD::OPEN_AI_STANDARD; // Only valid when a customized payload is included in the request. string ai_custom_payload; }; http://gerrit.cloudera.org:8080/#/c/22130/3/be/src/exprs/ai-functions.inline.h@111 PS3, Line 111: if (strcmp(api_standard_value, IMPALA_AI_API_STANDARD_OPENAI) == 0) { Should the comparison be case insensitive? http://gerrit.cloudera.org:8080/#/c/22130/3/be/src/exprs/ai-functions.inline.h@162 PS3, Line 162: // For the general platform, use the credential as a token by default. Technically speaking a 'token' is also a secret and we could also support retrieving it from a keystore? It probably also makes sense to support plain text secrets (maybe when jceks keystore is not configured?) for testing purpose as there is some overhead for setting up keystores. -- To view, visit http://gerrit.cloudera.org:8080/22130 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4ea2e1946089f262dda7ace73d5f7e37a5c98b14 Gerrit-Change-Number: 22130 Gerrit-PatchSet: 3 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Anonymous Coward (801) Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Mon, 06 Jan 2025 23:09:09 +0000 Gerrit-HasComments: Yes
