[Fix](ai_agg) isolate AI_AGG query_ctx per aggregate state#63080
Open
linrrzqqq wants to merge 1 commit intoapache:masterfrom
Open
[Fix](ai_agg) isolate AI_AGG query_ctx per aggregate state#63080linrrzqqq wants to merge 1 commit intoapache:masterfrom
linrrzqqq wants to merge 1 commit intoapache:masterfrom
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no blocking issues found in this PR.
Critical checkpoint conclusions:
- Goal and proof: The PR removes the process-wide static QueryContext from AI_AGG and binds the context through AggregateFunctionAIAgg into each AggregateFunctionAIAggData state on create, reset, and deserialize. The added unit test covers two aggregate function instances with different context-window settings and verifies isolation.
- Scope and clarity: The modification is small and focused on the AI_AGG context lifetime issue.
- Concurrency: The removed static pointer was the shared mutable state. The replacement stores context on each aggregate function instance and copies it into each aggregate state; no new shared mutable state or lock ordering concern was introduced.
- Lifecycle: Aggregate states are initialized through create, reset, and deserialize before use. QueryContext is an observer pointer consistent with the existing RuntimeState/query lifecycle used by AggFnEvaluator.
- Configuration: No new configuration items are added.
- Compatibility: No storage format, thrift protocol, or function signature compatibility change is introduced.
- Parallel paths: Nullable aggregate wrappers delegate set_query_context to the nested function, and the aggregate create/deserialize/reset paths reviewed all preserve context propagation.
- Conditional checks: Existing context checks remain unchanged; no new speculative defensive checks were added.
- Test coverage: The new BE unit test directly exercises the fixed cross-instance isolation behavior. I did not run tests in this review environment.
- Observability: No new observability is required for this narrowly scoped lifecycle fix.
- Transaction/persistence/data writes: Not applicable.
- FE/BE variable passing: No new transmitted variables are added.
- Performance: No meaningful overhead beyond assigning a pointer during aggregate state lifecycle operations.
User focus response: No additional user-provided focus points were supplied, and no extra issue was found while reviewing the whole PR.
Collaborator
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
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:
AI_AGGpreviously storedQueryContextin a process-level static pointer. Concurrent AI_AGG queries could overwrite that pointer, causing one query to read another query's AI resource metadata, query options, or timeout state.The aggregate function instance now receives the query context from
AggFnEvaluatorand binds it to each AggregateFunctionAIAggData state when the state is created, reset, or deserialized.