Repository navigation
[refactor](coordinator) Unify query execution in Coordinator - #69830
Merged
924060929 merged 11 commits intoNov 4, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
924060929
requested review from
CalvinKirs,
englefly and
starocean999
as code owners
October 29, 2026 08:12
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 28820 ms |
Contributor
TPC-DS: Total hot run time: 150950 ms |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
ClickBench: Total hot run time: 24.46 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of PR #69830 at head e9837f9 converged after round 1 with no verified blocking findings and no inline comments to publish.
Reviewed checkpoints: the EnvFactory planner cast and shared coordinator/load context wiring have repository callers that construct Nereids planners and establish the required load context; cloud coordinator removal preserves compute-group authorization, empty-group rejection, and backend selection; coordinator queue, cancellation, report, join, and cleanup paths retain terminal status and release resources across the examined orderings; unconditional Nereids distribution and the property/regulator changes cover the affected join, set-operation, recursive-CTE, and remote-Doris paths without a concrete changed-line mismatch; FILE_STREAM handling remains limited to the HTTP stream TVF path with one scan location and one instance, while group commit uses its separate scan node; and cache/adaptive FE RPCs retain coordinator null/lifetime guards and use the same planned instance data used for dispatch. The removed legacy fixtures and REMOVED session variable were checked as coverage/compatibility concerns, but no specific production failure was demonstrated. A late logical SQL-cache concern was rechecked and dismissed because the cache branch returns before the planner callback invokes distribution.
No user-provided review focus was supplied. No existing inline P0/P1 finding remains applicable, so existing_blocking_comment_ids is empty. This was a static-only review: no builds or tests were run, and inspected tests are cited only as source evidence. Convergence is true after round 1; all risk items and candidate findings are explicitly dismissed with evidence in the review ledger.
Review converged after 1 round(s).
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28619 ms |
Contributor
TPC-DS: Total hot run time: 150694 ms |
Contributor
ClickBench: Total hot run time: 24.24 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 28877 ms |
Contributor
TPC-DS: Total hot run time: 152053 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
ClickBench: Total hot run time: 24.14 s |
924060929
force-pushed
the
codex/remove-legacy-coordinator
branch
from
October 30, 2026 06:53
16a78e4 to
8b305ab
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
morrySnow
changed the title
morrySnow
previously approved these changes
Nov 4, 2026
### What problem does this PR solve? Issue Number: N/A Problem Summary: FE still carries two execution coordinators: the legacy `Coordinator` assigns scan ranges and fragment instances itself, while `NereidsCoordinator` executes Nereids distributed plans but inherits the legacy coordinator's state and initialization. `CloudCoordinator` adds another legacy-only factory branch. This duplicates scheduling and lifecycle ownership and retains a fallback that newer execution features do not support. Replace the legacy implementation with the standalone Nereids implementation, renamed to `Coordinator`, and remove `NereidsCoordinator` and `CloudCoordinator`. SQL, INSERT and background load creation now use the same factory. Cloud compute-group selection, privilege validation and backend membership remain in `BackendDistributedPlanWorkerManager` during distributed planning. Preserve the remaining shared capabilities explicitly: HTTP Stream Load generates parameters with `ThriftPlansBuilder`; Arrow Flight retains scans that must outlive dispatch; streaming INSERT retains scan backend IDs; cancellation uses the runtime task state, without treating remote-catalog workers as local backends. Group-commit backend selection remains in distributed planning. Retire `enable_nereids_distribute_planner` using the existing removed-variable handling and remove its legacy-only optimizer branches. Migrate relevant unit coverage and remove regression assertions that specifically required the deleted legacy scheduling path; keep their result checks. ### Release note Use Nereids distributed planning for all queries and loads. `enable_nereids_distribute_planner` no longer selects a planner; existing SET statements for this removed variable remain accepted. ### Check List (For Author) - Test: `./build.sh --fe --exclude-obs-dependencies --exclude-cos-dependencies`; FE Checkstyle (0 violations); 205 focused FE unit cases across 19 classes, including ordinary and group-commit HTTP stream planning. Corrected test fixtures were rerun separately. OBS/COS plugin packaging and full BE-backed regression were not run locally. - Behavior changed: Yes. Remove the legacy scheduling fallback and always use Nereids distributed planning. - Does this need documentation: No; the retired variable is hidden by the existing removed-variable mechanism.
### What problem does this PR solve? Problem Summary: HTTP Stream Load consumes one BE-owned input stream and completes one StreamLoadContext. Nereids local shuffle could expand its plan to eight instances, invoking the BE completion callback repeatedly and satisfying the same promise more than once. Keep FILE_STREAM TVF scans on one instance during distribution and assert that contract before returning the HTTP plan. Strengthen ordinary and group-commit planning tests with parallel_pipeline_task_num=8. Always-on distribution also exposed missing query IDs in shared FE unit-test contexts and missing worker setup in the cloud table-stream metadata fixture. Initialize identity centrally, retain real distribution on fixture backends, apply the test heartbeat interval to an Env already initialized by static fixtures, and update the UNION regression's obsolete legacy-plan expectations. ### Release note None ### Check List (For Author) - Test: 529 FE unit cases passed across 39 classes (3 existing skipped cases); FE Checkstyle; FE build with OBS/COS packaging excluded. Full BE-backed regression was not run locally. - Behavior changed: Yes. Preserve HTTP Stream Load's single-instance completion contract with Nereids distribution. - Does this need documentation: No
### What problem does this PR solve? Problem Summary: Always-on Nereids distribution inserts local exchanges into translated plans and requires valid query identity and available workers during planning. Aggregate translator tests assumed direct scan/aggregate adjacency, and SQL-cache tests suppressed query-id writes and supplied no backend. Ignore only intra-fragment local-exchange wrappers when asserting the aggregate structure, and initialize the cache fixture with a real query ID and an available backend. Regenerate 114 EXPLAIN-shape golden files for the Nereids distribution/property path using the original regression SQL and statistics. Keep existing query-result golden sections unchanged. ### Release note None ### Check List (For Author) - Test: 25 focused FE unit cases passed; 113 shape suites and the alias-through-join EXPLAIN suite passed comparison runs using real FE planning with mock backends and the original injected statistics. FE build and Checkstyle passed with OBS/COS packaging excluded. Full BE-backed query-result regression was not run locally. - Behavior changed: No production behavior changes in this follow-up. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Problem Summary: Preserve the master branch's split-generation stop and per-scan cleanup behavior when renaming NereidsCoordinator to Coordinator. Verify that the coordinator context reaches the active scan nodes and that a failing scan cleanup does not prevent remaining scans from closing. ### Release note None ### Check List (For Author) - Test: FE build and Checkstyle passed; 78 focused FE unit cases across six classes passed. - Behavior changed: No; retain master behavior while unifying coordinators. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Problem Summary: The unified distributed planner permits bucket shuffle for set operations when local shuffle is disabled. The translator still gated the set-operation bucket-shuffle marker on the FE local-shuffle planner flag. A pruned basic scan could therefore omit receivers for buckets targeted by another branch. Derive the marker from the selected child distribution, leaving legality and alignment in the property regulator. Cover UNION and EXCEPT with pruned scans, all four local-shuffle flag combinations, and complete receiver bucket allocation. ### Release note Preserve set-operation bucket receivers when local shuffle is disabled. ### Check List (For Author) - Test: 36 focused FE unit cases passed (set-operation translation, bucketed aggregation, bucket scheduling, Coordinator). Full FE build and Checkstyle passed. Full BE-backed result regression was not run locally. - Behavior changed: Yes; retain missing-bucket receivers for allowed bucket-shuffle set operations with local shuffle disabled. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Problem Summary: Coordinator cancellation delegates to the runtime fragment health check, which required both captured and current backend epochs to be positive before detecting a restart. Queries captured at an initial -1 or replayed 0 epoch could miss a subsequent positive epoch and wait until execution timeout. Match the legacy restart comparison in the shared health method: ignore a current zero epoch for replay/upgrade compatibility, but reject all other changed epochs. Add unit and Coordinator integration coverage for both initial values and the current-zero exception. ### Release note Cancel queries promptly when a backend epoch changes after an uninitialized epoch was captured. ### Check List (For Author) - Test: Four new assertions reproduced the failure before the fix; 31 focused FE unit cases passed after the fix. Full FE build and Checkstyle passed. - Behavior changed: Yes; restore cancellation on backend restart after a nonpositive captured epoch. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Problem Summary: The eliminate_outer_join suite still expects the legacy shuffle label for a nested full outer join. Regenerate the EXPLAIN expectations with the unified Nereids distribution path. The generated output changes exactly the shuffle label reported by P0 CI; all other plan output is unchanged. ### Release note None ### Check List (For Author) - Test: run-regression-test.sh generated the output and a separate comparison run passed all 29 EXPLAIN checks in the suite. Used real FE planning and three mock backends; no BE query-result execution was needed for these EXPLAIN-only checks. - Behavior changed: No - Does this need documentation: No
…lism ### What problem does this PR solve? Issue Number: N/A Problem Summary: The nested full-outer-join EXPLAIN expectation generated with low pipeline parallelism fails when Cloud P0 fuzzes parallel_pipeline_task_num to 7. Bucket-shuffle downgrade uses the actual backend count and pipeline parallelism, which be_number_for_test does not pin. Scope bucket_shuffle_downgrade_ratio=0 to this statement so the test continues checking outer-join elimination with a stable distribution choice. Leave production heuristics, other statements and golden output unchanged. ### Release note None ### Check List (For Author) - Test: Reproduced the original Cloud P0 mismatch with three mock backends and pipeline parallelism 7. All 29 EXPLAIN checks passed through run-regression-test.sh in four combinations of one/three mock backends and pipeline parallelism one/seven. Regenerated output is byte-identical to the committed golden. Real FE planning was used; no BE query-result execution or new FE build was needed for this SQL-only test change. - Behavior changed: No - Does this need documentation: No
…citly ### What problem does this PR solve? Issue Number: N/A Problem Summary: The removed enable_nereids_distribute_planner switch no longer forces partitioned joins. test_outer_join still assumes that behavior, while earlier coordinator migration removed partitioned exchange assertions from two distribution suites. Select supported distributions with effective statement-scoped join settings instead: retain both partitioned and bucket-shuffle checks and make the pruning suite exercise its intended bucket path without an early skip. Keep query-result assertions, production heuristics and golden files unchanged. ### Release note None ### Check List (For Author) - Test: Audited all 1225 files with false settings. All 1204 shape scripts compared successfully using real FE planning, original statistics and mock backends across the tested backend/parallelism configurations. Reproduced the stale outer-join assertion in a one-backend fixture; its six replacement paths passed with one/three backends and parallelism one/seven. The two distribution plan probes passed with one backend and parallelism one/seven/sixteen. Full BE-backed result execution and a new production FE build were not performed for this Groovy-only change. - Behavior changed: No - Does this need documentation: No
…ation ### What problem does this PR solve? Related PR: apache#69830 Problem Summary: The unified coordinator permits shuffling the left intermediate result onto the right table's storage buckets. For the TopN-through-distinct-left-join shape test, this alternative can select a one-phase aggregate instead of the expected two-phase aggregate. Disable bucket shuffle only for that EXPLAIN so the test keeps checking its original TopN rewrite shape. Preserve the result queries and all existing goldens. ### Release note None ### Check List (For Author) - Test: Reproduced the CI shape mismatch with real FE planning and mock backends using the original DDL and statistics matching the eight input rows. Both shape checks pass through run-regression-test.sh with 1/3 mock backends and parallel_pipeline_task_num=1/7/16. The modified Groovy source parses successfully; git diff --check passes. Real BE result execution remains for CI. - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Related PR: apache#69830 Problem Summary: Master added a pre-lock partition-view warmup decision that still calls the legacy distribute-planner switch. After coordinator unification that method no longer exists, and every executable or distributed EXPLAIN plan uses Nereids distribution. Remove the obsolete switch and dictionary exceptions, and share one finalization predicate between warmup selection and actual distribution. Update the existing planner tests to cover all EXPLAIN levels. ### Release note None ### Check List (For Author) - Test: 181 focused FE unit tests passed via run-fe-ut.sh, covering Coordinator, cloud workers, transport, INSERT, dictionary drop races, Nereids planning, partition preloading, CTE pruning, statement execution, and session variables. build.sh --fe --exclude-obs-dependencies --exclude-cos-dependencies passed with zero Checkstyle violations. Local output deployment used GNU cp to preserve existing configuration files without the macOS cp -n exit-status issue. - Behavior changed: No additional behavior beyond the unified-distribution refactor - Does this need documentation: No
924060929
force-pushed
the
codex/remove-legacy-coordinator
branch
from
November 4, 2026 02:11
61aded4 to
43e43e1
Compare
Contributor
Author
|
run buildall |
Contributor
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69830
commit: 43e43e104ca8d21386138a5ff2be378bb2bec17e
base: 11bfb7388944340181e7e4bef55c7629ea44e902
reviewed_at: 2026-11-04T10:22+08:00
reviewer: 924060929
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
4 of 16 tasks
Contributor
TPC-H: Total hot run time: 28257 ms |
Contributor
TPC-DS: Total hot run time: 151275 ms |
Contributor
ClickBench: Total hot run time: 24.28 s |
morrySnow
approved these changes
Nov 4, 2026
gavinchou
approved these changes
Nov 4, 2026
CalvinKirs
approved these changes
Nov 4, 2026
morningman
added a commit
that referenced
this pull request
Nov 5, 2026
…0163) ### What problem does this PR solve? Issue Number: None Related PR: #68206, #68636, #68368 Problem Summary: **In short.** Three branch-4.2 backports of master fixes each added a test: #68206 (of #61342), #68636 (of #57006) and #68368 (of #66354). Master has the three fixes but none of the tests. This PR ports them. Two guard a fix that has no test on master; the third keeps the ClickBench `query1` shape case from failing whenever the exact row count of `hits` is cached. The PR changes tests only. **Background, and what each test covers.** - *Old primary key metadata (#61342, #68206).* A `PrimaryKeyConstraint` written before its foreign table collections existed has no `ft`, `ftn` or `ftni` in its JSON. Gson builds the object without its field initializers, so these stay null unless `gsonPostProcess` fills them in, and an FE that replays the journal from the start (without an image, or with `metadata_failure_recovery`) failed with a NullPointerException. #61342 added the null checks without a test; #68206 added one on branch-4.2 for `ft`, the only collection there. The new `PrimaryKeyConstraintTest` removes all three from a serialized constraint, loads it, and adds, renames and removes a foreign table on it. - *Shadow index columns (#57006, #68636).* During a schema change the new schema names a changed column `__doris_shadow_<name>`. `Index.getColumnUniqueIds` compares the index's column names with the schema's after `Column.removeNamePrefix`, so the new tablet metadata keeps the index column's unique id. #57006 has no test; #68636 added one to `IndexTest`, ported here to JUnit 5. - *ClickBench query1 shape (#66354, #68368).* `shape_check/clickbench/query1` checks the plan of `SELECT COUNT(*) FROM hits`. Once `SimpleAggCacheMgr` has cached the exact row count of `hits`, `REWRITE_SIMPLE_AGG_TO_CONSTANT` turns the count into a constant and the shape into `PhysicalOneRowRelation`, so the case fails whenever that cache is warm when it runs. It did on P0 for #69830 (build 1087320: expected `--hashAgg[GLOBAL]`, real `--PhysicalOneRowRelation`), a PR that does not touch the planner. On branch-4.2, #68368 disables the rule in this case together with the cost model change it backports, which master has from #66354. This PR disables it the same way; the expected output does not change. **Results.** | Change | Test | Result | |---|---|---| | none | `PrimaryKeyConstraintTest` (new), `IndexTest` (one new case), `ConstraintManagerTest`, `ConstraintPersistTest` | pass | | `gsonPostProcess` without its null checks | `PrimaryKeyConstraintTest` | fails: NullPointerException while loading | | `getColumnUniqueIds` without `removeNamePrefix` | `IndexTest#testGetColumnUniqueIdsForShadowColumn` | fails: `expected [107] but was []` | | `query1` as on master, row count of `hits` cached (local cluster) | `shape_check/clickbench/query1` | fails: `PhysicalOneRowRelation` | | `query1` with this PR, same cache | `shape_check/clickbench/query1` | passes | On the local cluster, `explain shape plan SELECT COUNT(*) FROM hits` gives `hashAgg[GLOBAL]` the first time and `PhysicalOneRowRelation` three seconds later, once the asynchronous row count load has finished; with the rule disabled it stays `hashAgg[GLOBAL]`. Co-authored-by: minghong <[email protected]> Co-authored-by: Jack <[email protected]> Co-authored-by: feiniaofeiafei <[email protected]>
englefly
added a commit
that referenced
this pull request
Nov 6, 2026
### What problem does this PR solve? Issue Number: None Related PR: #69830 Problem Summary: Coordinator unification removed the legacy distributed planner and SessionVariable.isEnableNereidsDistributePlanner(), but GroupJoin translation and its conjunct-alignment postprocessor still called that getter. FE compilation failed with cannot-find-symbol errors in both paths. Remove the obsolete gates while preserving the fusion switch and backend capability checks. Update the bucket-shuffle regression to require fusion when the removed planner switch is set to false, since that switch no longer selects a legacy planner. ### Release note None ### Check List (For Author) - Test: - Manual test: ./build.sh --fe -j8 passed, including Checkstyle. - Manual test: ./build.sh --be -j12 passed with BUILD_TYPE=ASAN. - Unit Test: ./run-fe-ut.sh --run with GroupJoinFusionUtilsTest, PlanFragmentBucketShuffleTest and CoordinatorTest#testRemovedSwitchCannotSelectLegacyScheduling passed (9 tests, 0 failures, 0 errors). - Updated the bucket-shuffle regression; the regression suite was not run. - Behavior changed: No. The removed planner switch is already ignored on master. - 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
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.
What problem does this PR solve?
Problem Summary:
FE still carries two execution coordinators: the legacy
Coordinatorassigns scan ranges and fragment instances itself, whileNereidsCoordinatorexecutes Nereids distributed plans but inherits the legacy coordinator's state and initialization.CloudCoordinatoradds another legacy-only factory branch. This duplicates scheduling and lifecycle ownership and retains a fallback that newer execution features do not support.Replace the legacy implementation with the standalone Nereids implementation, renamed to
Coordinator, and removeNereidsCoordinatorandCloudCoordinator. SQL, INSERT and background load creation now use the same factory. Cloud compute-group selection, privilege validation and backend membership remain inBackendDistributedPlanWorkerManagerduring distributed planning.Preserve the remaining shared capabilities explicitly: HTTP Stream Load generates parameters with
ThriftPlansBuilderand retains one fragment instance on the receiving BE, matching its single completion callback; Arrow Flight retains scans that must outlive dispatch; streaming INSERT retains scan backend IDs; cancellation uses the runtime task state, without treating remote-catalog workers as local backends. Group-commit backend selection remains in distributed planning.Keep set-operation bucket-shuffle scheduling markers aligned with the selected child distribution, including when local shuffle is disabled, so pruned scans retain all required bucket receivers.
Preserve backend-restart cancellation when a fragment captured an uninitialized epoch (-1 or 0), while retaining the current-zero replay/upgrade exception. Stabilize the nested full-outer-join EXPLAIN with a statement-scoped bucket-shuffle downgrade setting, keeping the outer-join elimination checks independent of actual backend count and pipeline parallelism.
Migrate join-distribution assertions to effective statement-scoped settings: cover both partitioned and bucket-shuffle plans, retain exchange-count checks, and make pruning checks exercise the bucket path without an early skip. Keep the removed setting where it still provides compatibility coverage; do not mechanically rewrite unrelated test expectations.
Retire
enable_nereids_distribute_plannerusing the existing removed-variable handling and remove its legacy-only optimizer branches. Migrate unit fixtures to the distributed planner's query identity, worker and local-exchange contracts. Regenerate EXPLAIN-shape expectations with the Nereids distribution/property path while preserving query-result checks.Release note
Use Nereids distributed planning for all queries and loads.
enable_nereids_distribute_plannerno longer selects a planner; existing SET statements for this removed variable remain accepted.Check List (For Author)