HappenLee commented on PR #67673: URL: https://github.com/apache/doris/pull/67673#issuecomment-5692935956
One remaining **[P2]** issue outside the five resource validation/consistency findings in my earlier comment: **validate `ai.effort` against the selected provider, rather than a union of all providers' values.** On the current head (`83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c`), `AIProperties.requiredAIProperties()` accepts `none|minimal|low|medium|high|xhigh|max` for every provider. For example, an otherwise valid ANTHROPIC resource with `ai.effort='none'` passes CREATE validation. `AnthropicAdapter` then forwards it unchanged as `output_config.effort`, although the [Anthropic API contract](https://platform.claude.com/docs/en/build-with-claude/effort) only supports `low|medium|high|xhigh|max`, with additional model-specific restrictions. References: - [FE validation](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/AIProperties.java#L66-L85) - [BE payload construction](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/be/src/exprs/function/ai/ai_adapter.h#L1623-L1627) This affects requests that explicitly configure an unsupported effort value; existing usage that omits effort is unaffected by this issue. The expected upstream rejection follows from the code and API contract; I did not reproduce it against a live provider endpoint. This was [previously reported by the automated reviewer](https://github.com/apache/doris/pull/67673#discussion_r3975780989), so it is not a newly discovered finding. The current diff of #68023 does not cover effort validation. Please include provider-specific effort validation and negative tests in the follow-up, or separate the effort feature from this PR. Apart from the previously discussed resource issues and this P2, this re-review did not identify an additional P1 blocker. If these known issues are accepted for follow-up, I have no additional code-level objection to merging this PR first. This is not a claim that the outstanding issues are fixed. Validation for this re-review: changed-file clang-format 16.0.6, BE build hygiene, and diff whitespace checks passed. Builds and runtime tests were not rerun. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
