924060929 commented on code in PR #68901:
URL: https://github.com/apache/doris/pull/68901#discussion_r4245296420
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/NereidsPlanner.java:
##########
@@ -170,6 +170,9 @@ public void plan(StatementBase queryStmt,
org.apache.doris.thrift.TQueryOptions
} finally {
statementContext.getStopwatch().stop();
NereidsPlanner.runningPlanNum.decrementAndGet();
+ // Connectors plan the scans of the statement only inside plan(),
so what they kept for planning, such as
+ // the ranges identical scans share, is not needed once it returns
or throws.
+ statementContext.endConnectorPlanning();
Review Comment:
Could we call `endConnectorPlanning()` from
`StatementContext.releasePlannerResources()` instead, and remove this separate
call from `NereidsPlanner.plan()`?
`releasePlannerResources()` is already the common cleanup point in
`planWithLock()`'s `finally`. In the normal `plan()` path,
`lockCallback.accept(resultPlan)` performs distribution, plan translation and
scan finalization before that `finally` runs, so all synchronous scan ranges
have already been converted into execution parameters when cleanup begins.
Moving the call there would preserve reuse within one plan and still drop the
memo before execution.
This also keeps planning cleanup in one place: callers that use
`planWithLock()` directly already reach it, and `StatementContext.close()`
calls it as a fallback. Those direct callers do not currently materialize these
scan memos, so this is a lifecycle/maintainability suggestion rather than a
demonstrated missed cleanup today. If a future planning path starts producing
them, using the existing cleanup point avoids requiring it to remember a second
cleanup call. Repeated cleanup is safe because `endPlanning()` removes the
registered keys and leaves the set empty.
Please put the call after the existing resource-release loop but before
rethrowing its recorded exception, so a resource-close failure does not skip
memo eviction. Metadata, table state and transactions would still retain their
statement lifetime; only planning values would be removed.
For clarity, the old implementation already removed all memo keys through
`ConnectorStatementScopeImpl.closeAll()`'s final `cache.clear()` (base commit
`dd456ab7a894f54c6553415223c6d5c9c7d4e035`, line 100). That happened at
statement teardown/query-finish cleanup. This PR's benefit is releasing the
planning-only entries earlier, rather than adding their first cleanup.
--
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]
