Abhishek Rawat has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21493 )

Change subject: IMPALA-13131: Azure OpenAI API expects 'api-key' instead of 
'Authorization' in the request header
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21493/4/be/src/exprs/ai-functions-ir.cc
File be/src/exprs/ai-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/21493/4/be/src/exprs/ai-functions-ir.cc@146
PS4, Line 146: StringVal AiFunctions::AiGenerateText(FunctionContext* ctx, 
const StringVal& endpoint,
             :     const StringVal& prompt, const StringVal& model,
             :     const StringVal& api_key_jceks_secret, const StringVal& 
params) {
             :   std::string_view endpoint_sv(FLAGS_ai_endpoint);
             :   // endpoint validation
             :   if (endpoint.ptr != nullptr && endpoint.len != 0) {
             :     endpoint_sv = 
std::string_view(reinterpret_cast<char*>(endpoint.ptr), endpoint.len);
             :     // Simple validation for endpoint. It should start with 
https://
             :     if (!is_api_endpoint_valid(endpoint_sv)) {
             :       LOG(ERROR) << "AI Generate Text: \ninvalid protocol: " << 
endpoint_sv;
             :       return 
StringVal(AI_GENERATE_TXT_INVALID_PROTOCOL_ERROR.c_str());
             :     }
             :   }
             :   AI_PLATFORM platform = GetAiPlatformFromEndpoint(endpoint_sv);
             :   switch(platform) {
             :     case AI_PLATFORM::OPEN_AI:
             :       return AiGenerateTextInternal<false, AI_PLATFORM::OPEN_AI>(
             :           ctx, endpoint_sv, prompt, model, api_key_jceks_secret, 
params, false);
             :     case AI_PLATFORM::AZURE_OPEN_AI:
             :       return AiGenerateTextInternal<false, 
AI_PLATFORM::AZURE_OPEN_AI>(
             :           ctx, endpoint_sv, prompt, model, api_key_jceks_secret, 
params, false);
             :     default:
             :       LOG(ERROR) << "AI Generate Text: \nunsupported endpoint: " 
<< endpoint_sv;
             :       return 
StringVal(AI_GENERATE_TXT_UNSUPPORTED_ENDPOINT_ERROR.c_str());
             :   }
             : }
             :
             : StringVal AiFunctions::AiGenerateTextDefault(
             :   FunctionContext* ctx, const StringVal& prompt) {
             :   AI_PLATFORM platform = 
GetAiPlatformFromEndpoint(FLAGS_ai_endpoint);
             :   switch(platform) {
             :     case AI_PLATFORM::OPEN_AI:
             :       return AiGenerateTextInternal<true, 
AI_PLATFORM::OPEN_AI>(ctx, FLAGS_ai_endpoint,
             :           prompt, NULL_STRINGVAL, NULL_STRINGVAL, 
NULL_STRINGVAL, false);
             :     case AI_PLATFORM::AZURE_OPEN_AI:
             :       return AiGenerateTextInternal<true, 
AI_PLATFORM::AZURE_OPEN_AI>(ctx,
             :           FLAGS_ai_endpoint, prompt, NULL_STRINGVAL, 
NULL_STRINGVAL, NULL_STRINGVAL,
             :           false);
             :     default:
             :       DCHECK(false) << "Default endpoint " << FLAGS_ai_endpoint 
<< "must be supported";
             :       LOG(ERROR) << "AI Generate Text: \nunsupported endpoint: " 
<< FLAGS_ai_endpoint;
             :       return 
StringVal(AI_GENERATE_TXT_UNSUPPORTED_ENDPOINT_ERROR.c_str());
             :   }
             : }
> Can we remove the duplicated code by using a new function, such as HandleAi
We can't. Note that there is template specialization and so you can't move this 
into a common code block:
```
StringVal AiFunctions::AiGenerateText(.....)
.......
  AI_PLATFORM platform = GetAiPlatformFromEndpoint(endpoint_sv);
  switch(platform) {
    case AI_PLATFORM::OPEN_AI:
      return AiGenerateTextInternal<false, AI_PLATFORM::OPEN_AI>(......);
    case AI_PLATFORM::AZURE_OPEN_AI:
      return AiGenerateTextInternal<false, AI_PLATFORM::AZURE_OPEN_AI>(.....);
    default:
      .......
}
```

```
StringVal AiFunctions::AiGenerateTextDefault(....)
...
  AI_PLATFORM platform = GetAiPlatformFromEndpoint(FLAGS_ai_endpoint);
  switch(platform) {
    case AI_PLATFORM::OPEN_AI:
      return AiGenerateTextInternal<true, AI_PLATFORM::OPEN_AI>(.....);
    case AI_PLATFORM::AZURE_OPEN_AI:
      return AiGenerateTextInternal<true, AI_PLATFORM::AZURE_OPEN_AI>(.....);
    default:
      .........
}
```


http://gerrit.cloudera.org:8080/#/c/21493/4/be/src/exprs/ai-functions.inline.h
File be/src/exprs/ai-functions.inline.h:

http://gerrit.cloudera.org:8080/#/c/21493/4/be/src/exprs/ai-functions.inline.h@79
PS4, Line 79:     Status status = getAuthorizationHeader<platform>(authHeader, 
ai_api_key_);
            :     if (!status.ok()) return copyErrorMessage(ctx, 
status.msg().msg());
> Is it okay if we pass the function pointer?
I think I had a missing parenthesis somewhere so the original macro seems to 
work now.



--
To view, visit http://gerrit.cloudera.org:8080/21493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9cc07940ce355d511bcf0ee615ff31042d13eb5
Gerrit-Change-Number: 21493
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Rawat <[email protected]>
Gerrit-Reviewer: Abhishek Rawat <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Wenzhe Zhou <[email protected]>
Gerrit-Reviewer: Yida Wu <[email protected]>
Gerrit-Comment-Date: Tue, 11 Jun 2024 20:28:39 +0000
Gerrit-HasComments: Yes

Reply via email to