[improvement](planner) Reduce planner overhead - #67797
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nereids created ProcessState and maintained rewrite-path state even when plan-process tracing was disabled, rendered the final physical plan for every SQL-cache candidate before cache admission, and repeated cost calculations and weighted-cost object construction in the Cascades hot path.
Create ProcessState only while plan-process tracing is active, render the SQL-cache plan body only after FE or BE cache-admission checks succeed, and simplify cost accumulation while retaining the final property-aware node-cost recalculation. The removed CostWeight allocation keeps its non-negative-weight validation in Cost.
The independently measured ProcessState and cost-cleanup budgets are 0.303/0.335/1.712 ms CPU and 200.1/526.3/2618.5 KiB allocation for TPCH Q5, TPCDS Q72, and forced-Cascades TPCDS Q64. The deferred SQL-plan rendering cost pool is 0.408/0.588/1.348 ms CPU and 242.8/459.8/1012.5 KiB allocation; realized total CPU savings scale with the non-admission ratio.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.nereids.jobs.RewriteTopDownJobTest,org.apache.doris.nereids.cost.CostModelV1Test,org.apache.doris.nereids.properties.ChildrenPropertiesRegulatorTest,org.apache.doris.qe.SqlCacheTest (14 tests passed)
- Final Cost rerun: CostModelV1Test and ChildrenPropertiesRegulatorTest (11 tests passed)
- mvn clean install -DskipTests -Dskip.doc=true -T 1C -Dmaven.build.cache.enabled=false (81/81 modules passed)
- mvn checkstyle:check -pl fe-core (0 violations)
- sh generated-source.sh
- Behavior changed: No
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: CostWeight is a semantic snapshot of the session cost weights and should not be removed to avoid its allocation. Keep one lazily initialized CostWeight in StatementContext and reuse it for every Cost created by the statement. Lazy initialization preserves SET_VAR hint semantics, while rebinding the ConnectContext refreshes the snapshot for reused prepared statements.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- CostModelV1Test and ChildrenPropertiesRegulatorTest: 12 tests passed
- Final CostModelV1Test rerun: 4 tests passed
- FE Checkstyle: 0 violations
- Behavior changed: No
- Does this need documentation: No
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#67797 Problem Summary: PR apache#67797 changed cost accumulation from weighting the summed CPU, memory, and network components to adding already-weighted double values. Floating-point non-associativity introduced one-ULP total-cost differences, which could flip strict memo cost comparisons and unexpectedly change plan shapes. Pass the statement-scoped CostWeight to Cost.add and recompute the weighted value from the accumulated components, preserving the original plan-selection semantics without restoring per-CostWeight allocations. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.cost.CostModelV1Test: 4 tests passed - FE Checkstyle: 0 violations - Behavior changed: No (restores the pre-PR floating-point evaluation order) - Does this need documentation: No
|
run buildall |
TPC-H: Total hot run time: 16870 ms |
TPC-DS: Total hot run time: 82185 ms |
ClickBench: Total hot run time: 14.85 s |
|
/review |
There was a problem hiding this comment.
Review summary
Requesting changes for three blocking issues:
- The new cost-weight lookup ignores the
StatementContextowned by the active planner and can null-dereference in the supported minidump/direct-planner path. - The replacement child-cost path leaves five existing
ChildrenPropertiesRegulatorTestcases failing on this exact head. - BE SQL-cache admission now renders a catalog-backed plan after table locks are released and after rows are sent, creating a concurrent-DDL exception/snapshot race.
Critical checkpoints
- Goal and tests: The allocation/redundant-work reductions are mechanically aligned with the stated performance goal, but correctness is not established because of the three issues above. The new tests cover isolated cost addition/reset and FE-computed cache bodies; they do not cover active direct/minidump planning, BE-cache rendering under DDL, or tracing parity.
- Scope and clarity: All 24 changed files belong to the four stated planner-overhead reductions. The interface propagation is complete, though combining four independent optimizations broadens the review surface.
- Concurrency and lifecycle: Cost/rewrite work remains statement-serialized and nullable
ProcessStateaccess is consistently guarded. The material concurrency regression is late catalog-backed rendering in the BE cache path. Prepared execution resets the weight snapshot, but direct planners do not guarantee that their active statement is installed on the ambient connection. - Configuration and compatibility: No configuration, persisted format, storage metadata, public symbol, RPC/Thrift, or rolling-upgrade contract changes were found.
ComputeResultSetis internal and all five implementations plus both callers were updated. - Parallel/special paths: FE cache miss/hit, BE miss, cloud preflight, result/blackhole wrappers, empty/one-row leaves, command exclusions, unsupported-cache paths, Cascades regulation/enforcement, memo ranking, and cost recomputation were checked. No distinct issue remains beyond the three inline comments.
- Conditionals and error handling: Cache eligibility/hit exclusions and every nullable tracing-state use retain coherent guards. The BE admission path must not let optional cache rendering fail an already-produced query.
- Transactions, persistence, data writes, and FE-BE propagation: These areas are not changed.
- Observability and performance: No new metric/log is required. The intended allocation and redundant-calculation savings are plausible; they do not justify the context-ownership and post-lock rendering failures.
- User focus: No additional user-provided review focus was supplied.
Verification status
This was a complete static review of the authoritative diff after two convergence rounds; all round-2 reviewers returned NO_NEW_VALUABLE_FINDINGS. Local builds/tests/source edits were prohibited by the review contract. Exact-head CI was independently triaged: FE UT build 1044397 has the five PR-relevant regulator NPEs; the external-regression failure occurred after all 661 tests passed and was caused by a Maven download EOF, while FE coverage failed on its Node/GLIBC toolchain. Compile, checkstyle, performance, BE UT, and the remaining regression checks passed.
| } | ||
|
|
||
| public CostWeight getCostWeight() { | ||
| return getStatementContext().getCostWeight(); |
There was a problem hiding this comment.
[P1] Bind the weight to the planner's active statement
This new lookup assumes the StatementContext owned by NereidsPlanner is also installed on its ConnectContext, but the supported minidump replay path violates that invariant. MinidumpUtils.setConnectContext() creates a connection with no statement, and executeSql() constructs a statement only for NereidsPlanner; its first CostCalculator.calculateCost() therefore reaches this line with getStatementContext() == null and throws instead of producing the replay plan. Please pass the active planner/Cascades statement (or its snapshotted CostWeight) into cost calculation, or establish the ownership invariant for every direct-planner entry point, and cover minidump replay with an active test.
There was a problem hiding this comment.
Fixed in 9c7aa04. Cost calculation now receives the CostWeight snapshot from the active CascadesContext/StatementContext and threads the same snapshot through memo ranking and enforcer accumulation, so it no longer rediscovers the weight through ConnectContext. I also restored the minidump replay context contract, enabled the previously disabled replay test, and updated its fixture to the current format. The targeted FE run passed all 28 tests across MinidumpUtTest, ChildrenPropertiesRegulatorTest, CostModelV1Test, SqlCacheTest, and OlapQueryCacheTest.
| return; | ||
| } | ||
|
|
||
| sqlCacheContext.setPhysicalPlan(physicalPlan.treeString()); |
There was a problem hiding this comment.
[P1] Do not render the plan after releasing table locks
On the BE-cache path this call runs only after the planner has released its table read locks and after StmtExecutor has sent all result rows, but before it sets EOF. treeString() calls PhysicalOlapScan.toString(), which iterates the live OlapTable.indexNameToId HashMap and copies the nameToPartition TreeMap without a lock. A concurrent index/partition DDL can therefore throw ConcurrentModificationException here, converting an otherwise successful query into a post-row failure; a non-throwing interleaving can also cache a body that does not describe the executed snapshot. Previously the plan string was captured inside planWithLock. Please capture an immutable plan body while the catalog is protected, or otherwise make late rendering snapshot-safe and unable to fail the completed query.
There was a problem hiding this comment.
Fixed in 9c7aa04. NereidsPlanner now renders and stores the SQL-cache physical plan inside the lock-protected planning callback. FE/BE cache insertion consumes that already captured string and no longer receives or renders a live PhysicalPlan after result delivery and table-lock release. The targeted FE run passed all 28 tests, including SqlCacheTest and OlapQueryCacheTest.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#67797 Problem Summary: ChildrenPropertiesRegulatorTest mocked CascadesContext without a ConnectContext. After cost accumulation began using the statement-scoped CostWeight directly, five property-regulation cases dereferenced that missing context and failed with NullPointerException. Build the test fixture with a real ConnectContext and StatementContext and expose it through the mocked CascadesContext so the unit test matches the production invariant. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.properties.ChildrenPropertiesRegulatorTest (8 tests) - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review summary
Requesting changes on current head d4aa2f6ce704ff042368c85de7a8ee1d83fddfcd. The latest commit statically repairs the regulator test-fixture failure, but two previously reported P1 correctness issues remain in the code:
- Bind the weight to the planner's active statement: minidump replay installs no
StatementContexton itsConnectContext, so the first cost calculation still null-dereferences throughPlanContext.getCostWeight(). The same ownership invariant must cover sibling connection-based lookups in enforcer and nth-plan paths. - Do not render the plan after releasing table locks: BE SQL-cache admission still calls
physicalPlan.treeString()after planner locks are released and after rows may be sent but before EOF; OLAP scan rendering traverses mutable catalog maps, so concurrent DDL can fail an otherwise completed query or cache a non-snapshot plan body.
No new inline comments are included because both defects are fully covered by those existing threads; reposting them would violate the duplicate fence.
Critical checkpoints
- Goal and tests: Conditional
ProcessStateallocation, deferred cache rendering, removal of redundant pre-regulation cost work, and statement-scoped weight reuse are aligned with the stated overhead goal. The arithmetic/reset tests and FE-cache plan-body assertion are statically consistent, and the current fixture now supplies the regulator's required connection/statement. However, there is still no test covering active minidump/direct planning, snapshot-safe BE cache rendering under concurrent DDL, or explicit trace-on parity. The author reports the targeted tests and full FE build passed; this review did not independently run builds or tests because the review contract prohibits them. - Scope and clarity: All 24 changed files map to the four stated planner optimizations. The changes are mechanically focused within each mechanism, though combining the mechanisms broadens the review surface.
- Concurrency and lifecycle: Cascades jobs are statement-serialized; the lazy cost snapshot has no planner-job race, prepared execution resets it, and every nullable
ProcessStateuse is guarded. The remaining lifecycle violations are the direct-planner/connection statement mismatch and the query-versus-DDL race during late catalog-backed rendering. - Configuration and compatibility: No configuration, persisted format, RPC/Thrift, storage metadata, or rolling-upgrade contract changes were found. The internal
ComputeResultSetsignature is propagated to all five implementations and both callers. - Parallel and special paths: FE/BE cache admission and replay, cloud preflight, MySQL/Arrow/prepared exclusions, result/blackhole wrappers, one-row/empty/cache leaves, Cascades property alternatives and enforcers, nth-plan ranking, and cost recomputation were checked. No distinct issue remains beyond the two fenced P1s.
- Conditionals and error handling: Cache eligibility, SQL-cache replay exclusion, and trace-state guards are coherent. Optional cache-plan rendering must not be allowed to throw after BE rows have been produced.
- Transactions, persistence, data writes, and FE-BE propagation: Not changed by this PR.
- Observability and performance: No new metric or log appears necessary. The intended allocation and redundant-calculation savings are plausible, but they do not justify the two correctness regressions.
- User focus: No additional user-provided review focus was supplied.
Verification status
This was a complete static review of the authoritative 24-file diff. One full convergence round used two complete-review agents plus a separate five-risk audit; all returned NO_NEW_VALUABLE_FINDINGS after missed-case rechecks, and every risk was accepted as duplicate-fenced or dismissed with code evidence. No new inline comment was accepted. The live base/head were reverified immediately before submission.
### What problem does this PR solve? Issue Number: None Related PR: apache#67797 Problem Summary: Cost calculation rediscovered weights through the connection's current statement instead of using the statement owned by the active planner, which broke direct planner entry points such as minidump replay. SQL cache plan text was also rendered after catalog locks were released. Pass the active statement's cost-weight snapshot through memo costing and enforcer accumulation, restore the minidump replay context contract, and capture the cache plan while table locks are held. ### Release note None ### Check List (For Author) - Test: Unit Test - MinidumpUtTest - ChildrenPropertiesRegulatorTest - CostModelV1Test - SqlCacheTest - OlapQueryCacheTest - Behavior changed: No - Does this need documentation: No
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16848 ms |
TPC-DS: Total hot run time: 81240 ms |
ClickBench: Total hot run time: 14.77 s |
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nereids created ProcessState and maintained rewrite-path state even when plan-process tracing was disabled, rendered the final physical plan for every SQL-cache candidate before cache admission, and repeated cost calculations and CostWeight construction in the Cascades hot path.
This PR:
The independently measured ProcessState and cost-cleanup budgets are:
These are arithmetic budgets from independently measured constituents, not a combined-patch ABBA result. Deferred SQL-plan rendering exposes an additional CPU/allocation cost pool of 0.408 ms/242.8 KiB, 0.588 ms/459.8 KiB, and 1.348 ms/1012.5 KiB respectively. Realized total CPU savings for that part scale with the non-admission ratio.
Release note
None
Check List (For Author)