github-actions[bot] commented on code in PR #67673:
URL: https://github.com/apache/doris/pull/67673#discussion_r3968880655
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/AIProperties.java:
##########
@@ -53,29 +57,24 @@ public class AIProperties extends BaseProperties {
public static final String VALIDITY_CHECK = "ai.validity_check";
public static final List<String> REQUIRED_FIELDS = Arrays.asList(ENDPOINT,
PROVIDER_TYPE, MODEL_NAME);
+ public static final List<String> EMBED_REQUIRED_FIELDS =
+ Arrays.asList(EMBED_ENDPOINT, EMBED_PROVIDER_TYPE,
EMBED_MODEL_NAME);
public static final List<String> PROVIDERS
= Arrays.asList("OPENAI", "LOCAL", "GEMINI", "DEEPSEEK",
"ANTHROPIC",
"MOONSHOT", "QWEN", "MINIMAX", "ZHIPU", "BAICHUAN", "VOYAGEAI",
"JINA");
public static void requiredAIProperties(Map<String, String> properties)
throws DdlException {
- // Check required field
- for (String field : REQUIRED_FIELDS) {
- if (Strings.isNullOrEmpty(properties.get(field))) {
- throw new DdlException("Missing [" + field + "] in
properties.");
- }
+ boolean hasGeneralProperties = hasAnyProperty(properties,
REQUIRED_FIELDS, API_KEY);
+ boolean hasEmbedProperties = hasAnyProperty(properties,
EMBED_REQUIRED_FIELDS, EMBED_API_KEY);
+ if (!hasGeneralProperties && !hasEmbedProperties) {
Review Comment:
[P1] Reject embed-only resources for consumers that require the general
group. This condition now makes an embed-only resource valid, but the FE checks
for the other scalar AI functions and `AI_AGG` only verify the resource type.
They transport it with the general fields unset; scalar execution then reaches
`DORIS_CHECK(adapter)` for an empty provider, and `AI_AGG` dereferences the
null factory result. Please add a resource-capability check during FE legality
analysis, with negative scalar and aggregate tests, so these valid SQL
statements return an analysis error instead of aborting a BE.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/AIProperties.java:
##########
@@ -32,9 +32,13 @@ public class AIProperties extends BaseProperties {
public static final String ENDPOINT = "ai.endpoint";
public static final String PROVIDER_TYPE = "ai.provider_type";
public static final String MODEL_NAME = "ai.model_name";
+ public static final String EMBED_ENDPOINT = "ai.embed.endpoint";
+ public static final String EMBED_PROVIDER_TYPE = "ai.embed.provider_type";
+ public static final String EMBED_MODEL_NAME = "ai.embed.model_name";
// optional
public static final String API_KEY = "ai.api_key";
+ public static final String EMBED_API_KEY = "ai.embed.api_key";
Review Comment:
[P1] Register the embed API key with statement/audit masking.
`NeedAuditEncryption` rewrites CREATE/ALTER RESOURCE properties through
`DatasourcePrintableMap`, but its sensitive-key set contains only `ai.api_key`.
As a result, `ai.embed.api_key` remains verbatim in the encrypted SQL used by
statement and audit logging; the new test covers only SHOW RESOURCES. Please
add this key to the generic masker and cover both CREATE and ALTER
encryption/logging paths.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/AIProperties.java:
##########
@@ -97,6 +96,30 @@ public static void requiredAIProperties(Map<String, String>
properties) throws D
}
}
+ private static boolean hasAnyProperty(Map<String, String> properties,
List<String> requiredFields,
+ String apiKeyField) {
+ return properties.containsKey(apiKeyField) ||
requiredFields.stream().anyMatch(properties::containsKey);
+ }
+
+ private static void validatePropertyGroup(Map<String, String> properties,
List<String> requiredFields,
+ String providerTypeField, String apiKeyField) throws DdlException {
+ for (String field : requiredFields) {
+ if (Strings.isNullOrEmpty(properties.get(field))) {
+ throw new DdlException("Missing [" + field + "] in
properties.");
+ }
+ }
+
+ String providerType = properties.get(providerTypeField).toUpperCase();
+ properties.put(providerTypeField, providerType);
Review Comment:
[P1] Persist the normalized provider validated here. During ALTER,
`requiredAIProperties` mutates only `changedProperties`, while
`modifyProperties` subsequently writes the original ALTER map. For a resource
with positive dimensions, setting `ai.embed.provider_type` to lowercase
`openai` therefore validates successfully but persists lowercase; the BE
factory lookup is case-sensitive and reaches the null-adapter invariant. Please
install the validated/normalized merged snapshot, and test ALTER plus
serialization/replay, instead of discarding its normalization.
##########
be/src/exprs/function/ai/embed.h:
##########
@@ -44,8 +44,15 @@ class FunctionEmbed : public AIFunction<FunctionEmbed> {
using PreparedFunctionImpl::execute;
+ AIResource select_ai_resource(const TAIResource& resource) const {
+ bool has_embed_properties = resource.__isset.embed_endpoint ||
Review Comment:
[P1] Do not treat a single embed field as a complete dedicated group. ALTER
skips `requiredAIProperties` when `ai.validity_check=false` or the general
provider is `LOCAL`, so adding only `ai.embed.endpoint` is persisted and sets
this bit. EMBED then selects an empty provider/model/key and reaches the fatal
adapter check. Please make group-shape validation unconditional, separate it
from remote validity checking, reject incomplete wire groups normally, and add
partial-group ALTER tests.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/AIResource.java:
##########
@@ -149,10 +151,30 @@ protected void getProcNodeData(BaseProcResult result) {
public TAIResource toThrift() throws NumberFormatException {
TAIResource tAIResource = new TAIResource();
-
tAIResource.setProviderType(properties.get(AIProperties.PROVIDER_TYPE));
- tAIResource.setEndpoint(properties.get(AIProperties.ENDPOINT));
- tAIResource.setApiKey(properties.get(AIProperties.API_KEY));
- tAIResource.setModelName(properties.get(AIProperties.MODEL_NAME));
+ if (properties.containsKey(AIProperties.PROVIDER_TYPE)) {
+
tAIResource.setProviderType(properties.get(AIProperties.PROVIDER_TYPE));
+ }
+ if (properties.containsKey(AIProperties.ENDPOINT)) {
+ tAIResource.setEndpoint(properties.get(AIProperties.ENDPOINT));
+ }
+ if (properties.containsKey(AIProperties.API_KEY)) {
+ tAIResource.setApiKey(properties.get(AIProperties.API_KEY));
+ }
+ if (properties.containsKey(AIProperties.MODEL_NAME)) {
+ tAIResource.setModelName(properties.get(AIProperties.MODEL_NAME));
+ }
+ if (properties.containsKey(AIProperties.EMBED_PROVIDER_TYPE)) {
Review Comment:
[P1] Serialize one read-locked resource snapshot. ALTER mutates the shared
`HashMap` entry by entry under `writeLock`, but both query-planning transport
paths call this lock-free `toThrift` method. A concurrent plan can therefore
emit only part of the new dedicated group, or mix old and new
endpoint/provider/key values; any observed field makes the BE choose that
group. Please copy under `readLock` or atomically replace an immutable map, and
add a concurrency test proving serialization sees only the complete old or new
group.
--
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]
