Uh oh!
There was an error while loading. Please reload this page.
[Opt](ai-func) Improving AI function performance - #62494
Conversation
hello-stephen
commented
Apr 14, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
linrrzqqq
commented
Apr 14, 2026
run buildall |
hello-stephen
commented
Apr 14, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Apr 14, 2026
FE Regression Coverage ReportIncrement line coverage |
linrrzqqq
commented
Apr 15, 2026
/review |
There was a problem hiding this comment.
Found 1 blocking issue.
be/src/exprs/function/ai/embed.h: text embedding batching breaks Gemini resources._execute_text_embed()now batches multiple prompts into onebuild_embedding_request(inputs, ...)call, butGeminiAdapter::build_embedding_request()still serializes them into a singlecontentobject andparse_embedding_response()still returns a single embedding for the text path. Runningembed()on multiple rows with a GEMINI AI resource will now fail the cardinality check (expected N got 1) or effectively only embed the last input. Please either keep Gemini on the old per-row path or implement Gemini's true batch text embedding protocol before enabling batching here.
Critical checkpoint conclusions:
- Goal and correctness: The PR aims to improve AI-function performance via batching. That is only partially achieved because multi-row text
embed()is no longer correct for all supported providers. Existing tests do not cover the failing Gemini text-embedding path. - Scope/minimality: The change is focused, but the generic text-embedding batching applies to providers with different protocol semantics.
- Concurrency: No new thread-safety or locking issue identified; the path remains synchronous.
- Lifecycle/static init: No special lifecycle or static initialization issue found.
- Configuration:
multimodal_embed_max_batch_file_countis added and forwarded to BE correctly throughTQueryOptions. - Compatibility: No storage-format or persistence compatibility issue found; FE/BE query-option propagation looks complete for the new variable.
- Parallel paths: Multimodal embedding and string AI functions were updated, but the provider-specific Gemini text embedding path was not handled consistently.
- Special conditions/checks: The new multimodal input validation is reasonable.
- Test coverage: Unit coverage improved, but there is no test for multi-row
embed()with a GEMINI resource, which is the broken path here. - Test result files: Not applicable.
- Observability: Existing observability is sufficient for this review; no blocker here.
- Transaction/persistence/data writes/FE-BE variable passing: Not applicable beyond query-option forwarding, which is covered.
- Performance: Batching should help supported providers, but this regression must be fixed first.
- Other issues: No additional blocking issue confirmed beyond the Gemini regression.
Uh oh!
There was an error while loading. Please reload this page.
831a82e to
75d0015Comparelinrrzqqq
commented
Apr 15, 2026
run buildall |
hello-stephen
commented
Apr 15, 2026
FE Regression Coverage ReportIncrement line coverage |
hello-stephen
commented
Apr 15, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Apr 15, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
linrrzqqq
commented
Apr 15, 2026
/review |
There was a problem hiding this comment.
I found one blocking issue.
Critical checkpoint: Goal / correctness / tests
Conclusion: The PR goal is to batch AI function requests for better performance, and the BE unit tests cover several batch-splitting and parsing cases. However, one provider-specific compatibility path is broken, so the implementation does not fully accomplish the goal safely.Critical checkpoint: Scope and focus
Conclusion: The change is reasonably focused on AI batching plus the needed session/thrift plumbing.Critical checkpoint: Concurrency
Conclusion: No new concurrency or locking risks were introduced in the modified paths. The execution remains row-batch local inside function evaluation.Critical checkpoint: Lifecycle / static initialization
Conclusion: No applicable lifecycle or static initialization issues found in the changed code.Critical checkpoint: Configuration changes
Conclusion:embed_max_batch_sizeis added as a forwarded session variable and wired intoTQueryOptions. FE forwarding looks complete for the new option.Critical checkpoint: Compatibility / protocol changes
Conclusion: FE/BE protocol compatibility forembed_max_batch_sizeis handled, but provider API compatibility is not fully preserved: Gemini embedding now emits a batchedrequestspayload without any corresponding endpoint normalization or compatibility handling.Critical checkpoint: Parallel code paths
Conclusion: Text AI functions and EMBED were both updated for batching. No missing sibling path stood out beyond the Gemini provider-specific compatibility issue below.Critical checkpoint: Special condition checks
Conclusion: Input-size validation added for multimodal requests is appropriate and clear.Critical checkpoint: Test coverage
Conclusion: BE unit tests were expanded well for batch parsing/splitting and multimodal request shapes. But there is no test proving Gemini works end-to-end against the configured endpoint shape after this request-format change.Critical checkpoint: Test result files
Conclusion: Not applicable; no regression.outfiles changed.Critical checkpoint: Observability
Conclusion: No new observability additions were needed for this scope.Critical checkpoint: Transaction / persistence
Conclusion: Not applicable.Critical checkpoint: Data writes / atomicity
Conclusion: Not applicable.Critical checkpoint: FE-BE variable passing
Conclusion: The newembed_max_batch_sizevariable is forwarded to BE correctly.Critical checkpoint: Performance
Conclusion: The batching approach should significantly improve throughput, but the Gemini compatibility regression blocks safe acceptance.Critical checkpoint: Other issues
Conclusion: No other blocking issue confirmed beyond the inline finding.
Because of the Gemini compatibility regression, I’m requesting changes.
Uh oh!
There was an error while loading. Please reload this page.
linrrzqqq
commented
Apr 15, 2026
run buildall |
hello-stephen
commented
Apr 15, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Apr 15, 2026
FE Regression Coverage ReportIncrement line coverage |
hello-stephen
commented
Apr 15, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Apr 15, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
linrrzqqq
commented
Apr 16, 2026
/review |
There was a problem hiding this comment.
I found two blocking regressions in the new batching work.
append_parsed_text_result()now treats any scalar text wrapped in[]as a batch array and errors out if it is not valid JSON. That breaks legitimate scalar outputs for existing functions, for exampleai_maskexplicitly asks the model to emit[MSKED], andai_aggstill routes arbitrary scalar text throughadapter->parse_response().- Gemini embed batching now always builds a
batchEmbedContents-style request body (requestsarray), butnormalize_endpoint()only rewrites bare.../v1/.../v1betaroots. Existing AI resources configured with the documented explicit...:embedContentendpoint will now send the wrong payload to the old method and fail.
Critical checkpoints:
- Goal: batch AI requests for better performance. The PR largely achieves that for the happy path, but the two regressions above break existing valid inputs/configurations.
- Scope/focus: mostly focused, though the shared response parser changed behavior for non-batched paths too (
ai_agg). - Concurrency: no new shared-state concurrency issue found in the reviewed paths.
- Lifecycle/static init: no new lifecycle or SIOF issue found.
- Config:
embed_max_batch_sizeis added and forwarded to BE correctly, but I did not see FE-side range validation. - Compatibility: Gemini embed endpoint compatibility is regressed for users with explicit
:embedContentresources. - Parallel paths: the shared parser change also affects non-batched callers like
ai_agg. - Tests: the new BE tests cover happy-path batching well, but they miss bracketed scalar outputs and explicit-Gemini-endpoint compatibility.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // provider response -> choices[0].message.content = "[\"1\",\"0\",\"1\"]" | ||
| // this helper -> appends "1", "0", "1" into `results` | ||
| static Status append_parsed_text_result(std::string_view text, | ||
| std::vector<std::string>& results) { |
There was a problem hiding this comment.
consider maybe we can also use std::vector<string_view> to reduce str copy
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
Apr 17, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Apr 17, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Apr 17, 2026
FE Regression Coverage ReportIncrement line coverage |
hello-stephen
commented
Apr 17, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
hello-stephen
commented
Apr 18, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Apr 18, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Apr 18, 2026
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
hello-stephen
commented
Apr 18, 2026
FE Regression Coverage ReportIncrement line coverage |
linrrzqqq
commented
Apr 20, 2026
/review |
There was a problem hiding this comment.
I found blocking correctness issues in the new batching/parsing paths.
append_parsed_text_result()now mixes batch parsing with scalar parsing. This breaks validai_aggoutputs that are themselves JSON array strings, and it can also let malformed bracketed output slip through for single-row batched string functions.- The new
ai_context_window_sizeguard for scalar AI functions is only based on raw input length.build_batch_prompt()escapes quotes, backslashes, newlines, etc., so the actual JSON prompt can exceed the configured window and still be sent as one batch. EMBEDtext batching has the same serialized-size accounting bug: it only sums raw input bytes beforebuild_embedding_request()JSON-escapes the payload.
Critical checkpoints
- Goal: The PR does implement batch sending and the new session variables, and the FE->BE plumbing/tests cover the happy path. However, the current code does not fully accomplish the goal safely because the response parser and window enforcement are incorrect in edge cases.
- Scope/focus: The AI batching work is mostly focused, although the patch also includes an unrelated
predicate_column.hfix. - Concurrency: I did not find new lock-order or thread-safety bugs in the modified scalar/embed paths.
ai_aggstill relies on its existing static query-context pattern; this PR does not introduce new synchronization around it. - Lifecycle/static init: No new static-initialization-order issue found. Existing
ai_aggstatic context lifetime behavior remains. - Config changes:
embed_max_batch_sizeandai_context_window_sizeare added, validated, forwarded to BE, and regression-tested. The blocking issue is that BE enforcement is only approximate and can undercount escaped payload size. - Compatibility: The new thrift fields are optional with defaults, and the FE/BE forwarding path is updated. I did not find a rolling-upgrade serialization blocker in this patch.
- Parallel code paths: Gemini/openai normalization and multimodal embedding paths were updated consistently where needed.
- Special-condition checks: The new behavior depends on heuristic text-shape detection in
append_parsed_text_result(), which is not safe across both batch and scalar callers. - Test coverage: Good positive coverage for batch splitting, session-variable validation, Gemini endpoint normalization, and multimodal MIME propagation. Missing negative coverage for JSON-array literal outputs in
ai_aggand for escaped-input batches that exceed the serialized window. - Observability: The new warning for batch result size mismatch is useful. No additional observability blocker found.
- Transaction/persistence/data writes: Not applicable for this change.
- FE/BE variable passing: Updated correctly for the new session variables.
- Performance: The batching work should improve performance, but the serialized-size undercount can still trigger provider-side failures under quote/backslash-heavy inputs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
PR approved by at least one committer and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
### Release note
Improving the performance of AI functions through batch sending, embed
controls the number of (text/file) items sent in a single batch through
the variable `embed_max_batch_size`, and the remaining functions
internally maintain a conservative context window.
The current sending format is similar to:
```json
"input": [
{"role": "system", "content": "system_prompt here"},
{"role": "user", "content": [
{"idx": 1, "text": "xxx"},
{"idx": 2, "text": "xxx"},
]
}
]
```
performance:
```sql
-- AI_CLASSIFY
SELECT COUNT(*) AS total_rows,
SUM(IF(res = 'science', 1, 0)) AS excepte_eq_res
FROM (
SELECT AI_CLASSIFY('deepseek-chat', str, ['science', 'sport']) AS res FROM test_str
) t;
-- before +------------+----------------+
| total_rows | excepte_eq_res |
+------------+----------------+
| 100 | 100 |
+------------+----------------+
1 row in set (2 min 11.579 sec)
-- now
+------------+----------------+
| total_rows | excepte_eq_res |
+------------+----------------+
| 100 | 100 |
+------------+----------------+
1 row in set (10.487 sec)
-- AI_FILTER
SELECT COUNT(*) AS total_rows,
SUM(IF(res = 1, 1, 0)) AS zero_res_rows
FROM (
SELECT AI_FILTER('deepseek-chat', str) AS res FROM test_str
) t;
-- before
+------------+---------------+
| total_rows | zero_res_rows |
+------------+---------------+
| 100 | 0 |
+------------+---------------+
1 row in set (2 min 2.979 sec)
-- now
+------------+---------------+
| total_rows | zero_res_rows |
+------------+---------------+
| 100 | 0 |
+------------+---------------+
1 row in set (5.007 sec)
-- EMBED
select count(embed('qwen-embed', str)) FROM test_str;
-- before
+---------------------------------+
| count(embed('qwen-embed', str)) |
+---------------------------------+
| 100 |
+---------------------------------+
1 row in set (4 min 4.888 sec)
-- now
set embed_max_batch_size = 10;
+---------------------------------+
| count(embed('qwen-embed', str)) |
+---------------------------------+
| 100 |
+---------------------------------+
1 row in set (23.424 sec)
-- Multimodal_Embed
SELECT COUNT(EMBED('qwen_mul_embed', to_json(file))) FROM test_jpg2;
-- before: can't get results for a long time(over 20 mins).
-- now
set embed_max_batch_size = 20;
+----------------------------------------------------+
| .... |
| 1152 |
+----------------------------------------------------+
1142 rows in set (1 min 13.577 sec)
```
Release note
Improving the performance of AI functions through batch sending, embed controls the number of (text/file) items sent in a single batch through the variable
embed_max_batch_size, and the remaining functions internally maintain a conservative context window.The current sending format is similar to:
performance:
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)