github-actions[bot] commented on code in PR #67797:
URL: https://github.com/apache/doris/pull/67797#discussion_r4004007111
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/PlanContext.java:
##########
@@ -57,6 +58,10 @@ public SessionVariable getSessionVariable() {
return connectContext.getSessionVariable();
}
+ public CostWeight getCostWeight() {
+ return getStatementContext().getCostWeight();
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/common/cache/NereidsSqlCacheManager.java:
##########
@@ -262,6 +265,7 @@ public void tryAddBeCache(ConnectContext connectContext,
String sql, CacheAnalyz
return;
}
+ sqlCacheContext.setPhysicalPlan(physicalPlan.treeString());
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java:
##########
@@ -994,8 +994,7 @@ private void updateChildEnforceAndCost(GroupExpression
child, PhysicalProperties
ConnectContext connectContext =
jobContext.getCascadesContext().getConnectContext();
Cost enforceCost = CostCalculator.calculateCost(connectContext,
enforcer, Lists.newArrayList(childOutput));
enforcer.setCost(enforceCost);
- Cost totalCost = CostCalculator.addChildCost(
- connectContext, enforcer.getPlan(), enforceCost, currentCost,
0);
+ Cost totalCost = enforceCost.add(currentCost,
connectContext.getStatementContext().getCostWeight());
Review Comment:
[P1] Restore the regulator unit-test contract
This direct dereference bypasses the statically mocked cost helper used by
`ChildrenPropertiesRegulatorTest`; its fixture only stubs
`getCascadesContext()`, so `getConnectContext()` is null. On this exact head,
FE UT build 1044397 has five new unmuted NPE failures in the MUST_SHUFFLE
project/filter cases at this path. Please provide the test fixture with the
active statement/weight (or thread the active `CostWeight` into the regulator
without rediscovering it through connection state) so these existing assertions
execute and the FE suite is green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
