HappenLee commented on PR #67673:
URL: https://github.com/apache/doris/pull/67673#issuecomment-5651329512
Reviewed commit `83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c`. The dedicated
embedding selection is straightforward, but I think the following resource
validation and consistency issues need to be addressed.
1. **[P1] Keep the resource validated during analysis consistent with the
resource sent to BE.**
The new general-group check validates one resource object, but the
statement records only its name. A possible interleaving is: an `AI_AGG` query
passes analysis against a resource with a complete general group; another
session drops that resource and creates a valid embed-only resource with the
same name; `Coordinator.getNeededAiResources()` or
`ThriftPlansBuilder.collectAiResources()` then looks up the replacement and
checks only its type. BE receives an empty general provider, and
`AggregateFunctionAIAggData::prepare()` dereferences the null adapter returned
by the factory. This is a less common concurrency scenario, but it is a direct
null dereference, including in Release builds.
Please retain the validated resource snapshot in the statement, or
revalidate the required capabilities and obtain the same snapshot at transport
time. Add a DROP/CREATE interleaving test. Taking a read lock only inside
`toThrift()` would not address the identity change.
References: [analysis
check](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/AIAgg.java#L90-L99),
[transport
lookup](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/fe/fe-core/src/main/java/org/apache/doris/qe/runtime/ThriftPlansBuilder.java#L222-L240),
[BE
initialization](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/be/src/exprs/aggregate/aggregate_function_ai_agg.h#L131-L156).
2. **[P2] Persist the normalized provider produced by ALTER validation.**
`requiredAIProperties(changedProperties)` normalizes the provider in a
temporary merged map, but `modifyProperties()` subsequently writes the original
ALTER map. Consequently, lowercase `openai` passes validation but is persisted
and forwarded unchanged. The BE adapter factory is case-sensitive and cannot
resolve it.
Concrete input:
```sql
CREATE RESOURCE 'review_embed_case' PROPERTIES (
'type'='ai',
'ai.embed.provider_type'='OPENAI',
'ai.embed.endpoint'='https://example.com/v1/embeddings',
'ai.embed.model_name'='embedding-model',
'ai.embed.api_key'='dummy-key',
'ai.dimensions'='8'
);
ALTER RESOURCE 'review_embed_case' PROPERTIES (
'ai.embed.provider_type'='openai'
);
SELECT EMBED('review_embed_case', 'hello');
```
Positive dimensions intentionally avoid the pre-existing
default-dimensions validation issue. Please install the validated and
normalized merged snapshot, and cover ALTER plus persistence/replay and
`toThrift()` in tests.
References:
[normalization](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/AIProperties.java#L112-L128),
[ALTER
implementation](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/fe/fe-core/src/main/java/org/apache/doris/catalog/AIResource.java#L128-L148).
3. **[P2] Validate group completeness on ALTER regardless of
`ai.validity_check` or LOCAL.**
When the general provider is LOCAL, or `ai.validity_check=false`, ALTER
skips `requiredAIProperties()`. It can therefore persist only
`ai.embed.endpoint`. The new selector treats any dedicated field as sufficient
to select the entire dedicated group, resulting in an empty provider and a
failed adapter invariant.
```sql
CREATE RESOURCE 'review_embed_partial' PROPERTIES (
'type'='ai',
'ai.provider_type'='LOCAL',
'ai.endpoint'='http://localhost:8000/v1/embeddings',
'ai.model_name'='local-model'
);
ALTER RESOURCE 'review_embed_partial' PROPERTIES (
'ai.embed.endpoint'='http://localhost:8001/v1/embeddings'
);
SELECT EMBED('review_embed_partial', 'hello');
```
This fails before an HTTP request is made. Please separate mandatory
structural validation from optional validity checking and reject partial groups
during ALTER. Silently falling back to another model would hide the invalid
configuration. The ALTER framework predates this PR, but the new
group-selection logic relies on an invariant that this framework does not
guarantee.
References: [conditional
validation](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/fe/fe-core/src/main/java/org/apache/doris/catalog/AIResource.java#L107-L132),
[dedicated-group
selector](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/be/src/exprs/function/ai/embed.h#L47-L51).
4. **[P2] Serialize a consistent resource snapshot under the resource read
lock.**
ALTER mutates `properties` under `writeLock()`, but `toThrift()` takes no
read lock, and both transport callers invoke it directly. Even a completely
valid ALTER can produce this interleaving:
```text
Query: check embed_provider_type -> absent; skip it
ALTER: install a complete dedicated group and finish
Query: read embed_endpoint, embed_api_key, embed_model_name -> present
BE: select the dedicated group, but its provider is empty
```
Please copy the properties under `readLock()` and serialize that
snapshot. The merged ALTER state should also be validated and installed within
one write-locked transition: two requests validating the same old OPENAI/key
state can otherwise commit LOCAL/empty-key followed by GEMINI, leaving GEMINI
without a key. Add deterministic interleaving tests for both cases.
Reference: [unlocked
serialization](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/fe/fe-core/src/main/java/org/apache/doris/catalog/AIResource.java#L173-L201).
5. **[P2] Validate dedicated providers against embedding capabilities.**
The dedicated group uses the general provider allowlist, so an embed-only
resource with DEEPSEEK or MOONSHOT passes CREATE and expression analysis.
However, both corresponding BE adapters unconditionally return `NotSupported`
for embedding construction and parsing. Such a resource cannot execute EMBED
even with an otherwise usable endpoint.
Please reject explicitly unsupported providers for the dedicated group
and add negative tests. A small shared capability description would be
sufficient.
References: [group
validation](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/AIProperties.java#L63-L80),
[unsupported
adapters](https://github.com/apache/doris/blob/83a2ac73e5e5469ea3f1c83cafc31dbbb61dd89c/be/src/exprs/function/ai/ai_adapter.h#L1016-L1040).
For findings 2–4, the EMBED path reaches `DORIS_CHECK(adapter)`: Release
builds convert this to a query error, while Debug/ASAN builds terminate through
`Status::FatalError()`. These should not all be described as production BE
crashes. Finding 1's `AI_AGG` null dereference is a separate failure path.
From a reuse and scope perspective, I would keep the single
configuration-selection hook and make CREATE/ALTER share the validated snapshot
logic. I would also split `ai.effort` into a separate PR: it is independent of
embedding routing and additionally changes provider payloads, aggregate-state
serialization, and the execution version. The current effort version gate and
empty-value clearing fix are present; I am not reporting those as unresolved
issues. The PR description and release note should reflect the final scope.
Validation: the existing `AIResourceTest` (20 tests) and
`RepositoryAuditEncryptionTest` (5 tests) passed locally, as did changed-file
clang-format 16.0.6, build hygiene, and diff checks. The failure scenarios
above are based on source tracing and concrete inputs/interleavings; I did not
run them against a live FE/BE cluster. The existing tests do not cover these
ALTER/concurrency cases. I did not identify an obvious new per-row performance
regression; no benchmark was run.
--
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]
