Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/21168 )
Change subject: IMPALA-12920: Support ai_generate_text built-in function for OpenAI's chat completion API ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/21168/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21168/6//COMMIT_MSG@38 PS6, Line 38: BuiltInFunctions (ai_generate_text and ai_generate_text_default) > Don't see test case for key_jceks_secret Added fe test. http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/exprs/ai-functions-ir.cc File be/src/exprs/ai-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/exprs/ai-functions-ir.cc@101 PS6, Line 101: // Check if the "choices" array exists and is not empty > Theoretically you could set the 'n' parameter, which would return multiple Makes sense. Added a check for n == 1. http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/exprs/ai-functions-ir.cc@128 PS6, Line 128: json.Accept(writer); > This makes an unnecessary copy; can we use string_view instead? https://en. Updated code to use string_view also templatized to minimize branches for the fast path. http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/exprs/scalar-expr-evaluator.cc File be/src/exprs/scalar-expr-evaluator.cc: http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/exprs/scalar-expr-evaluator.cc@453 PS6, Line 453: AiFunctions::AiGenerateTextInternal<true>(nullptr, StringVal::null(), StringVal::null(), > Presumably this results an in error because 'prompt' is a null string, but Done. http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/runtime/exec-env.cc@524 PS6, Line 524: nctions::set_api_key(api_key); > It's better to get jecks_secret per AiFunctions::AiGenerateTextInternal() i This ai_generate_text function is perf sensitive as it will be called for every row being evaluated so makes sense to minimize these JNI calls in the fast path. http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/runtime/exec-env.cc@528 PS6, Line 528: } > Is this safe to permanently cache? I guess this comes from a site file, so Yeah for the case where the secret-key changes there is an alternative to pass the secret-key using the other syntax. Adding support for dynamic config updates would be super cool feature. http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/runtime/exec-env.cc@535 PS6, Line 535: config_request.__set_name(DEFAULT_FS); > These don't cause anything immediately to fail. What's the rationale for no The rationale is that there is an alternative where user could pass the correct endpoint using alternative syntax. But, we could also fail initialization if that makes more sense. Added DEFINE_validator and also failing initialization on error. http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/udf/udf.h File be/src/udf/udf.h: http://gerrit.cloudera.org:8080/#/c/21168/6/be/src/udf/udf.h@a742 PS6, Line 742: > nit: unnecessary whitespace change, although I think the new form is a litt Removed unintentional whitespace change. http://gerrit.cloudera.org:8080/#/c/21168/6/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/21168/6/fe/src/main/java/org/apache/impala/service/JniFrontend.java@812 PS6, Line 812: from the con > nit: init as null Done. Also returning error for the case where password is empty. -- To view, visit http://gerrit.cloudera.org:8080/21168 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id4446957f6030bab1f985fdd69185c3da07d7c4b Gerrit-Change-Number: 21168 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Yida Wu <wydbaggio...@gmail.com> Gerrit-Comment-Date: Mon, 08 Apr 2024 03:16:13 +0000 Gerrit-HasComments: Yes
