[Enhancement](ai_func) Skip Null inputs in AI functions - #66242
Open
linrrzqqq wants to merge 2 commits into
Open
[Enhancement](ai_func) Skip Null inputs in AI functions#66242linrrzqqq wants to merge 2 commits into
linrrzqqq wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes. The compact prompt/result mapping is functionally sound for nullable scalar arguments, and the reverse embedding-offset expansion preserves row order without copying nested vectors. Two issues remain: a legal NULL resource literal can now reach a BE DORIS_CHECK instead of propagating NULL (MAIN-002), and nullable scalar blocks with an all-zero null map unnecessarily duplicate every result (MAIN-001).
Critical checkpoint conclusions: The change is focused and covers scalar, text-embedding, and multimodal-embedding paths. There is no production concurrency, special lifecycle, configuration, persistence, transaction, storage-format, or protocol change. Return-type and overload dispatch are otherwise consistent, and provider result-count validation protects compact-row reconstruction. Tests cover partial NULLs, merged argument maps, nullable array prompts, all-NULL constants, multimodal inputs, and batch boundaries, but they miss the NULL resource and all-zero-map cases above. No extra user review focus was supplied. Per the runner instruction, this was a static review and no local build or test command was run.
linrrzqqq
force-pushed
the
ai-func-skip-null
branch
from
July 29, 2026 18:19
e02787a to
0d75fc4
Compare
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Follow-up review of 0d75fc43b7c12316bb6345091562fe9d64aa4c4b: I found no remaining blocking issue. The two previously raised concerns are addressed at this head: an all-NULL resource now returns before resource lookup, and a nullable input with an all-zero null map wraps the existing result directly instead of scattering and copying every result.
Critical checkpoint conclusions:
- Goal and correctness: The change skips NULL prompt rows for all scalar AI functions and for both text and multimodal
EMBED. Provider response counts are checked before reconstruction; scalar results are scattered in original row order, and embedding offsets are expanded in place without copying the nested Float32 vectors. - Scope and parallel paths: The change is focused on NULL propagation and compact execution. The common scalar path, String/Bool/Float result types, text embedding, multimodal embedding, constant inputs, multiple nullable arguments, all-NULL blocks, and all-zero null maps are covered.
- Nullable and compatibility contracts:
ColumnConst(ColumnNullable)handling follows the function framework's constant path, BE return nullability matches FEPropagateNullable, andEMBEDpreserves itsArray<Nullable<Float32>>element type. There is no storage, protocol, persistence, rolling-upgrade, or function-symbol compatibility change. - Concurrency, lifecycle, configuration, and data writes: There is no new production concurrency, shared mutable lifecycle, configuration item, transaction/persistence behavior, or data-write path. The test-only adapter capture is thread-local.
- Performance: NULL rows no longer consume provider requests or tokens; the scalar all-zero fast path avoids a full result copy, and the embedding path rewrites only offsets while retaining the nested vector buffer.
- Testing: BE unit coverage was added for partial NULLs, merged argument null maps, String/Bool/Float and nullable-array inputs, all-NULL constants, NULL resources, all-zero maps, text and multimodal embeddings, and batch boundaries. Per the review-runner instruction, I did not run builds or tests locally.
No additional user-provided review focus was supplied. Review completion is confirmed for the current head.
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29434 ms |
Contributor
TPC-DS: Total hot run time: 177519 ms |
Contributor
ClickBench: Total hot run time: 24.92 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Mryange
previously approved these changes
Jul 30, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
只处理了顶层 nullable（argument.type->is_nullable()）。例如 ai_classify 的 labels 为 |
github-actions
Bot
removed
the
approved
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28953 ms |
Contributor
TPC-DS: Total hot run time: 170158 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Static review of d7a1ef26978facb3e582be9181db89843ddb10c2: I found no remaining blocking issue, so my approval opinion is positive. The two existing concerns are addressed at this head: a NULL resource returns before lookup, and a nullable input with an all-zero null map wraps the completed result without scattering and copying it.
Critical checkpoint conclusions:
- Goal and correctness: The change skips NULL prompt rows for all scalar AI functions and for both text and multimodal
EMBED. Per-batch provider result counts are validated before reconstruction; scalar results are scattered back in original row order, and embedding offsets are expanded backward in place without copying nested Float32 vectors. - Scope and clarity: The change is focused on manual NULL propagation and compact provider execution. The common base owns the shared contract, while only specialized prompt/result handling remains in derived functions.
- Concurrency: There is no new production thread, shared mutable state, lock, atomic, or lock-order concern. The test-only embedding-input capture is thread-local.
- Lifecycle and initialization: There is no new special production lifecycle or cross-translation-unit static dependency. The test-only vector uses function-local thread-local initialization.
- Configuration: No configuration item is added or changed, so dynamic-update handling is not applicable.
- Compatibility: There is no storage-format, protocol, persistence, function-name, or rolling-upgrade change. FE
PropagateNullableand BE outer-nullable return construction agree for String, Bool, Float32, andArray<Nullable<Float32>>results. - Parallel paths and special conditions: All eleven registered AI scalars use the new contract. String/Bool/Float scalar outputs, merged nullable arguments, const nullable columns, nullable array rows/elements, text embedding, and multimodal embedding were checked. Resource exclusion from the prompt map is safe because FE restricts it to a literal, with the NULL-constant case handled before resource lookup.
- Test coverage and expected results: Factory-based BE unit tests exercise the real prepared-function path for partial NULLs, multiple nullable arguments, all-NULL constants, no-actual-NULL maps, NULL resources, nullable label elements, specialized result types, exact text request compaction, multimodal skipping, and batch-boundary offset restoration. The result oracles match the compact/scatter and array-offset invariants. Current PR checks report BE UT, compile, formatter, and regression suites passing; per runner instruction, I did not run builds or tests locally.
- Observability: Existing status propagation and explicit provider result-count errors are sufficient for the changed path; no new durable or distributed state requires additional logs or metrics.
- Transactions, persistence, data writes, and FE-BE variables: None are introduced or modified, so atomicity, failover, EditLog, and scattered variable-passing checks are not applicable.
- Performance: NULL rows no longer consume external requests or tokens; the all-zero scalar fast path avoids a full result copy, and
EMBEDrestores row layout by rewriting offsets while retaining nested vector storage. - Other issues and user focus: No additional substantiated correctness, error-handling, memory-safety, compatibility, lifecycle, or performance issue remained after the final sweep. No additional user-provided review focus was supplied.
Review completion: complete for this head. Two normal full-review agents and a separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS against the same frozen diff, thread set, and ledger; the main review independently verified every risk conclusion, and the final 16-file sweep found zero unresolved candidates. No inline comments are proposed.
wzz6423
added a commit
to wzz6423/doris
that referenced
this pull request
Aug 3, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66242 Problem Summary: The automated review prompt submits a COMMENT review when it finds no critical blocking issue. A comment does not submit GitHub approval, so a prior automated REQUEST_CHANGES review can keep a PR blocked even after the code-review check succeeds. Submit an APPROVE review for the clean-review path. ### Release note None ### Check List (For Author) - Test: Manual test - Parsed the updated workflow YAML and asserted the clean-review prompt uses --approve. - Behavior changed: Yes (clean automated reviews now submit GitHub approval) - Does this need documentation: No
wzz6423
added a commit
to wzz6423/doris
that referenced
this pull request
Aug 3, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66242 Problem Summary: The workflow previously treated any review submitted for the current pull request head as successful. A COMMENT review could therefore satisfy the workflow even though it does not approve the pull request. Require the GitHub Actions bot to submit either APPROVED or CHANGES_REQUESTED before the review workflow succeeds. ### Release note None ### Check List (For Author) - Test: Manual test - Parsed the updated workflow YAML. - Verified COMMENTED and human APPROVED reviews are rejected, while the bot APPROVED review is accepted. - Behavior changed: Yes (invalid automated review states now fail the workflow) - Does this need documentation: No
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Summary:
The framework's default NULL implementation unwraps Nullable arguments and executes AI functions for every input row. For partially NULL inputs, the nested placeholder values of NULL rows are still included in prompts and sent to external AI providers.
This causes unnecessary remote requests and token consumption. It also requires special handling for embedding results to preserve the original row order without duplicating large embedding vectors.
This PR:
AIFunctionsubclasses.AIFunctionbase class.EMBED.Release note
Fix AI scalar functions to skip NULL input rows instead of sending their placeholder values to external AI providers.