Repository navigation
Conversation
### Background A backend retains an extra query-context reference while it coordinates merged runtime filters. Query-context destruction triggers the final profile report for that backend. ### What problem does this PR solve? Issue Number: None Related PR: None PR Type: Bugfix Problem Summary: The merge owner remained until cancellation or timeout after successful queries, delaying final fragment profiles. Release only the extra owner after a non-recursive query's initial fragment batch is prepared and all remote filter dispatches and callbacks are complete. Legacy and recursive queries retain the existing lifetime behavior. ### Details - **Change map:** About 200 lines of production changes in BE lifecycle, FE planning, and an optional Thrift field; the remainder is unit and regression coverage. - **Frequency:** Successful queries using a runtime-filter merge controller could retain the extra owner until timeout. - **Root cause:** Normal completion had no cleanup of the delayed merge owner; the existing merge-done flag precedes RPC dispatch and cannot establish callback completion. - **Why existing tests missed it:** Existing coverage did not assert the merge owner's lifetime after successful filter completion or a timely complete profile. - **Fix:** Add a query-wide single-use contract, track remote size/publish dispatch and callback completion, and erase the delayed owner outside map locks. Keep local-only filters registered but exclude them from remote completion checks. - **Other paths and branches:** Missing or false contract values preserve the previous behavior. Recursive plans do not opt in, so reset/rebuild remains protected. The optional field is backward compatible. ### Testing - Local ASAN BE and FE build; focused BE tests repeated with randomized order; focused FE unit tests. - Local query-profile and recursive-CTE regression suites, including a shuffle remote-filter path. - clang-format, format check, build hygiene, clang-tidy, and diff check. ### Notes for Reviewers The profile regression ran on a single-BE cluster. Multi-BE and mixed-version behavior remains for broader validation. ### Release note Final profiles for successful non-recursive queries with runtime filters are reported promptly instead of being held until query timeout. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Partially verified: multi-BE and mixed-version runs were not performed locally. ### AI Disclosure - Tool: OpenAI Codex - Generated: Code, tests, and this description. - Human review: Pending independent review.
### Background A backend retains a query context while coordinating merged runtime filters. Completion of the merge dispatch allows a single-use query to release this extra owner and report its final profile. ### What problem does this PR solve? Issue Number: None Related PR: None PR Type: Bugfix Problem Summary: A recursive reset could replace the completion object while an older publish call still used it, causing a use-after-free. A normal end-of-file response to a delayed size-sync RPC was also classified as a dispatch failure, preventing timely cleanup. Keep a strong reference to the publishing round's completion and treat that end-of-file response as normal. ### Details - **Frequency:** The reset race requires a recursive filter with a concurrent old-round publish tail; delayed size-sync responses can occur after a producer finishes. - **Root cause:** The publisher repeatedly read a replaceable shared-pointer member rather than retaining its own round's completion. The new callback failure bit did not match the existing end-of-file handling. - **Why existing tests missed it:** The recursive regression contained no runtime filter, and synthetic ownership tests raced with a live cleanup worker. No test called the merge size callback with an end-of-file status. - **Fix:** Snapshot the completion under the merge-context lock, use it throughout direct and tree dispatch, align callback failure classification with the existing callback, isolate synthetic ownership tests from the worker, and make the recursive regression generate a partitioned remote runtime filter. - **Other paths and branches:** The optional single-use contract remains conservative for recursive and older plans. Transport and non-EOF application failures still prevent automatic release. No release-branch behavior is changed. ### Testing - Local ASAN BE and FE build with FE Checkstyle; focused BE tests, including 20 randomized repetitions of 48 tests; focused FE tests (26 passed). - Local recursive runtime-filter regression generated through the runner and passed five further repetitions; existing parallel recursive and query-profile regressions passed. - clang-format, format check, build hygiene, clang-tidy, and diff check passed. ### Release note Final profiles for successful single-use queries with runtime filters can be reported promptly, including when a completed producer acknowledges a delayed size-sync with end-of-file. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Partially verified: local single-BE regression passed; multi-BE and mixed-version execution were not run. ### AI Disclosure - Tool: OpenAI Codex - Generated: Code, tests, and this description. - Human review: Pending independent review.
### Background FE groups pipeline fragments by worker identity and sends one phase-two start RPC for each worker. BE may release the extra RF merge QueryContext owner when an opted-in query has finished its RF work. ### What problem does this PR solve? Issue Number: None Related PR: None PR Type: Bugfix Problem Summary: Workers from different catalogs can have distinct identities but share one BE BRPC endpoint. The first start can complete the query before a second start reaches that BE; releasing the merge context then makes the late start fail. Only opt in to prompt RF merge context release when each physical endpoint has one worker identity. ### Details - **Frequency:** Queries that use distinct catalog workers mapped to one BE endpoint and whose phase-two starts are processed far enough apart. - **Root cause:** The single-use eligibility check excluded recursive plans but did not exclude duplicate starts to one physical endpoint. - **Why existing tests missed it:** They covered one worker and distinct endpoints, not different catalog identities at the same endpoint. - **Fix:** Compare every assigned worker's BRPC endpoint and worker identity across the complete plan. Fall back to the existing delayed-owner lifetime if one endpoint has multiple identities. - **Other paths and branches:** Repeated jobs for the same worker remain eligible. Distinct endpoints remain eligible. This commit changes no wire field or release branch. ### Testing - `./run-fe-ut.sh --run org.apache.doris.qe.runtime.ThriftPlansBuilderTest,org.apache.doris.qe.runtime.RuntimeFiltersThriftBuilderTest,org.apache.doris.common.profile.ProfileTest,org.apache.doris.common.profile.ExecutionProfileTest` (27 passed). - `./run-be-ut.sh --run --filter='FragmentMgrCrossClusterCancelTest.LocalOnlyRfMergeOwnerReleasedAfterLastFragmentOwner:FragmentMgrCrossClusterCancelTest.MissingSingleUseContractKeepsMergeOwnerForReset:FragmentMgrCrossClusterCancelTest.RemoteRfRequiresSizeDispatchAndCallbackCompletion:FragmentMgrDelayDeleteMapTest.*:RuntimeFilterMgrTest.MergeSizeDispatchCallbackAcceptsEofButRejectsOtherFailures' -j 20` (6 passed). - `BUILD_TYPE=ASAN ./build.sh --be --fe -j 50` (passed, including FE Checkstyle). - Isolated single-BE `rf_merge_context_profile`, `rf_merge_context_reuse`, and `rec_cte_parallel_targets` regression suites (passed). - `build-support/clang-format.sh`, `build-support/check-format.sh`, `build-support/check-build-hygiene.sh`, and `build-support/run-clang-tidy.sh` (passed). ### Release note Avoid query failures when distinct catalog workers share a BE endpoint, while preserving prompt final profiles for eligible queries. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Partially verified: isolated single-BE tests passed; delayed duplicate-start and multi-BE mixed-version cases were not run. ### AI Disclosure - Tool: OpenAI Codex - Generated: Eligibility guard, focused unit test, and this commit description. - Human review: Not yet performed.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
1 similar comment
Member
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review recovery stopped: Could not verify exactly one final review from this invocation; first recorded API failure: GitHub API POST repos/apache/doris/pulls/70305/reviews failed (CalledProcessError, exit=1, timeout=None); stderr="unexpected end of JSON input\nGitHub request failed: POST /repos/apache/doris/pulls/70305/reviews; HTTP=500; request_id=2885:342262:1713C19:4ABC321:6AECAB14; CalledProcessError; exit=1; timeout=None"; stdout="" (see review-api-errors.jsonl) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
There was a problem hiding this comment.
Reviewed all 14 changed files at commit 25d0414. No new actionable finding or independently confirmed existing P0/P1 finding. The review converged after two rounds with complete assigned static coverage and no unresolved candidates.
Critical checkpoint conclusions:
- Goal and proof: the change releases the extra RF merge QueryContext owner after a complete eligible deployment and successful required dispatch/callback completion, enabling final profile reporting. Added unit and regression tests address eligibility, ownership gates, callback failures, timely profiles, and recursive reuse; their execution was not part of this review.
- Scope and reuse: changes are focused on lifetime eligibility and completion accounting; existing cancellation, BRPC callback, profile, and map machinery is reused.
- Concurrency and locks: service preparation, pipeline completion, BRPC callbacks, and the cancel worker are the concurrent actors. Batch/dispatch publication uses release/acquire atomics; pending callbacks use atomic accounting. Cleaner work and last-reference destruction occur outside map shard locks. No new lock-order inversion was substantiated.
- Lifecycle and static initialization: live fragment contexts retain their own QueryContext references; the delayed map supplies only an extra owner. Callbacks retain per-round Completion and weak query ownership. Recursive reset preserves captured old Completion lifetime. No new strong ownership cycle or cross-translation-unit initializer dependency was found.
- Configuration: no configuration item is added. Cleanup uses the existing cancel-worker interval.
- Interface compatibility: missing/false eligibility preserves conservative lifetime; an old BE ignores the optional new field. No storage-format or exported function-symbol change was identified. Mixed-version execution was not run.
- Thrift IDs: the complete TRuntimeFilterParams definition was compared on HEAD and live-verified branch-3.1, branch-4.0, branch-4.1, and branch-4.2. New optional bool field 6 is unused on those release branches; existing field meanings are preserved. This PR targets master.
- Parallel paths: local-only, remote direct/tree, broadcast, dynamic-size, disabled/early-terminated, recursive, legacy, and one/two-phase deployment paths were traced. Distinct workers sharing an endpoint disable early release. Stream-load extraction omits the opt-in and retains conservative behavior.
- Conditional checks: the batch marker, independent size gate, failure ordering, and recursive/endpoint exclusions have concrete failure-scenario comments matching the inspected control flow.
- Test coverage: FE tests cover empty/recursive/duplicate-endpoint eligibility; BE tests cover ownership gates, missing/false opt-in, reset retention, callback EOF/errors, and reentrant erase/clear. Controlled concurrent service-boundary and multi-BE delayed-delivery execution remain validation limits.
- Expected results: join rows (2,20) and (4,40), and recursive rows 1 through 4, match the supplied SQL and deterministic ordering. ProfileAction waits for COMPLETE and the suites use per-query tokens. Output-generation provenance was not independently reproduced.
- Observability: existing query destruction logs, RPC error logs, and profile completion reporting expose the changed outcome; no additional metric requirement was substantiated.
- Transactions and persistence: no EditLog, replay, transaction, or failover behavior is changed.
- Writes and core data invariants: no write atomicity, partition visibility, or delete-bitmap behavior is changed. No introduced data-correctness defect was substantiated.
- FE-to-BE propagation: the opt-in is emitted in shared parent RF parameters only after whole-query eligibility checks; BE requires both presence and true. Alternate send paths remain conservative.
- Performance and memory: completion state is bounded per filter/round; the existing background worker scans retained contexts and eligible filters. Large filter payload ownership and cancellation teardown retain existing mechanisms. No new significant untracked allocation or hot-path regression was substantiated by source inspection.
- Other risks and user focus: no additional user focus was supplied. Every initial risk was dismissed with source evidence; no unreviewed changed path or unresolved candidate remains.
Validation was static only. No builds, tests, benchmarks, formatting tools, or runtime checks were executed, and repository source was not modified. Runtime stress, multi-BE scheduling, mixed-version behavior, and author-reported test execution are not independently verified by this review.
Review converged after 2 round(s).
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28429 ms |
Contributor
TPC-DS: Total hot run time: 153724 ms |
Contributor
ClickBench: Total hot run time: 24.99 s |
### Background The recursive runtime-filter regression checks that a repeated recursive join publishes a filter to the probe scan across an exchange. ### What problem does this PR solve? Issue Number: None Related PR: None PR Type: Test Problem Summary: The Cloud P0 planner does not render the recursive join with the exact PARTITIONED label required by the new test. Keep the join, runtime-filter target, exchange, and result checks while allowing a different join distribution label. ### Details - **Failure:** Cloud P0 fails at the exact join-distribution assertion before checking runtime-filter behavior. - **Cause:** The test assumes a physical distribution label that is not stable across deployment modes. - **Stability evidence:** The isolated shared-nothing regression suite passes before and after this assertion change; Cloud validation remains pending. ### Testing - `doris-local-regression run -d recursive_cte -s rf_merge_context_reuse` passed on the isolated local cluster. - `git diff --check` passed. ### Release note None. ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Partially verified: Cloud P0 must validate the revised assertion. ### AI Disclosure - Tool: OpenAI Codex - Generated: Test assertion and commit description. - Human review: Not applicable.
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 14 changed files at db176cf. No new actionable finding and no existing inline P0/P1 finding to carry forward. The static review converged in one round: both complete-review agents and the separate concurrency/lifetime reviewer returned NO_NEW_VALUABLE_FINDINGS, and every initial risk was dismissed with source evidence.
Critical checkpoint conclusions:
- Goal and tests: the change permits final profile reporting by releasing the extra RF merge QueryContext owner after complete eligible deployment and required remote completion. Added ownership, callback, eligibility, profile-deadline and recursive tests address that goal; they were inspected, not executed.
- Scope and reuse: changes are focused on eligibility and completion accounting, reusing the existing cancel worker, BRPC error handling and profile reporting.
- Concurrency and locks: service preparation, pipeline tasks, RPC workers, BRPC callbacks and the cleaner are the concurrent actors. Release/acquire dispatch gates follow all send-loop increments; failure is published before the last callback decrement. Controller checks and final owner destruction occur outside map shard locks. No introduced lock inversion or blocking wait was substantiated.
- Lifecycle and static initialization: live fragment and incoming RPC references retain QueryContext after its extra owner is removed. Callbacks hold weak query ownership and a captured per-round Completion; AutoReleaseClosure retains the callback during invocation. Relay RPC state has independent ownership. No new strong cycle or static-initialization dependency was found.
- Configuration: none added; cleanup uses the existing cancel-worker interval.
- Interface compatibility: missing/false opt-in keeps conservative lifetime, and old BE ignores the optional field. No disk-format or exported ABI change was identified; mixed-version execution was not run.
- Thrift IDs: complete TRuntimeFilterParams definitions were compared on HEAD and live-verified branch-3.1, branch-4.0, branch-4.1 and branch-4.2. New optional bool field 6 is unused on each release branch; existing meanings are preserved. This PR targets master.
- Parallel paths: local-only, remote direct/tree, broadcast, runtime-size, disabled/early-terminated, recursive, legacy and one/two-phase deployment paths were checked. Duplicate worker identities at one endpoint disable release. Alternate stream-load serialization does not carry the opt-in and remains conservative; cloud and shared-nothing use the same relevant lifecycle.
- Special conditions: comments explain the complete-batch barrier, separate size phase, Bloom/nonbroadcast requirement, local-only bypass and query-wide recursive/endpoint exclusions. These match the inspected call chains.
- Test coverage: BE tests cover ownership gates, missing/false opt-in, old Completion lifetime, EOF/error/transport outcomes and reentrant erase/clear; FE tests cover empty/recursive plans and endpoint identity. Real RPC timing and multi-BE interleavings remain execution-validation limits.
- Expected outputs: ordered join rows (2,20),(4,40) and recursive rows 1 through 4 agree with supplied SQL. Profile tests use unique SQL tokens and require COMPLETE within ten seconds. Output generation was not independently reproduced.
- Observability: existing RPC failure logs, query destruction logs and final profile reporting expose relevant outcomes; no additional necessary metric was identified.
- Transactions and persistence: no EditLog, replay, failover or transaction behavior changes.
- Data writes and core invariants: no write atomicity, visible-version or delete-bitmap behavior changes; no introduced data-correctness defect was substantiated.
- FE-to-BE propagation: full-query eligibility is serialized in parent RF parameters; BE requires presence and true. Other send paths stay conservative.
- Performance and memory: completion state is constant-sized per filter/round, with accounting per dispatched RPC and a linear scan on the existing worker. No significant new untracked allocation, payload copy or hot-path regression was substantiated by source inspection.
- Other risks and user focus: no additional user focus was supplied. Recursive mutable dispatch/reset interactions were checked against merge-base source and predate this PR; the new cleanup excludes recursive plans. No unresolved introduced-code candidate or unreviewed changed path remains.
Validation was static only. No builds, tests, benchmarks, formatting tools or runtime checks were run, and repository source was not modified. Multi-BE delayed filters, controlled duplicate-start/concurrent-reset scheduling, mixed-version execution and author-reported validation were not independently reproduced.
Review converged after 1 round(s).
Member
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28282 ms |
Contributor
TPC-DS: Total hot run time: 153734 ms |
Contributor
ClickBench: Total hot run time: 25 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### Background The RF merge profile regression checks that local and shuffled joins produce complete query profiles. ### What problem does this PR solve? Issue Number: None Related PR: apache#70305 (updates its regression coverage) PR Type: Test Problem Summary: The profile suite assumed every shuffled join uses a PARTITIONED join label. Cluster planning can select another distribution while still retaining the join, Exchange, and runtime filter behavior under test. Check those relevant plan properties without requiring one distribution label. ### Details - **Failure:** The profile suite failed on its exact PARTITIONED join assertion. - **Cause:** The assertion depended on a physical distribution choice rather than the behavior this suite verifies. - **Stability evidence:** The focused suite passed three consecutive runs on a local ASAN cluster after the change; CI must validate the alternate cluster plan. ### Testing - `doris-local-regression run -d query_profile -s rf_merge_context_profile` (three consecutive local ASAN runs, passed). - `git diff --check` (passed). ### Release note None. ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Partially verified: the local focused suite passed; CI must verify the alternate cluster plan. ### AI Disclosure - Tool: Codex - Generated: Regression assertion update and this description. - Human review: Not applicable.
Member
Author
|
/review |
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.
Background
A backend retains an extra QueryContext reference while coordinating merged runtime filters. Destruction of that context triggers the backend's final fragment-profile report.
What problem does this PR solve?
Issue Number: None
Related PR: None
PR Type: Bugfix
Problem Summary: A successful query could retain the extra merge owner until cancellation or timeout, delaying its final profiles beyond the frontend's normal collection window. This change releases that owner after an eligible query has completed initial fragment preparation and all required remote filter dispatches and callbacks. Recursive, legacy, and duplicate-endpoint-start plans retain the conservative lifetime behavior.
Details
Testing
BUILD_TYPE=ASAN ./build.sh --be --fe -j 50passed locally, including FE Checkstyle../run-fe-ut.sh --runsuites passed (27/27); focused ASAN./run-be-ut.sh --runtests passed (6/6).rf_merge_context_profile,rf_merge_context_reuse, andrec_cte_parallel_targetsregression suites each passed (1 suite, 0 failed).build-support/clang-format.sh,build-support/check-format.sh,build-support/check-build-hygiene.sh,build-support/run-clang-tidy.sh, andgit diff --checkpassed locally.Notes for Reviewers
The optional FE-to-BE field is fail-closed. The BE requires complete initial deployment, successful remote phases, and zero pending callbacks before releasing the delayed owner; live fragment contexts still retain their own QueryContext references.
Release note
Final profiles for eligible successful queries using runtime filters can be reported promptly rather than waiting for query timeout.
Check List (For Author)
AI Disclosure