github-actions[bot] commented on code in PR #62282:
URL: https://github.com/apache/doris/pull/62282#discussion_r3070722645
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertIntoTableCommandTest.java:
##########
@@ -170,4 +174,23 @@ void
testSelectInsertExecutorFactoryForRemoteTableWithGroupCommitException() {
command.selectInsertExecutorFactory(planner, ctx, stmtExecutor,
remoteDorisExternalTable);
Review Comment:
This new assertion calls `command.containsInternalGroupCommitTvf()`, but
that helper is declared `private` in `InsertIntoTableCommand`, so this test
will not compile. The check needs to go through a public/package-visible
surface, or the helper visibility needs to be adjusted intentionally.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertIntoTableCommand.java:
##########
@@ -447,21 +447,30 @@ ExecutorFactory selectInsertExecutorFactory(
if (getLogicalQuery().containsType(InlineTable.class)) {
jobId = -1;
}
+ // Do not register internal group commit loads to
LoadManager.
+ // Internal group commit uses 'SELECT * FROM
group_commit(...)' as source,
+ // and the actual commit is managed by BE group commit
mechanism.
+ // These jobs will never transition to a completed state
through FE,
+ // causing a memory leak if registered.
+ if (containsInternalGroupCommitTvf()) {
+ jobId = -1;
+ }
if (targetTableIf instanceof RemoteDorisExternalTable) {
executorFactory = ExecutorFactory.from(
planner,
dataSink,
physicalSink,
() -> new RemoteOlapInsertExecutor(
- ctx, (RemoteOlapTable) olapTable,
label, planner, insertCtx, emptyInsert, jobId)
+ ctx, (RemoteOlapTable) olapTable,
label, planner, insertCtx, emptyInsert,
Review Comment:
This call site still uses `RemoteOlapInsertExecutor(..., jobId)` even though
`AbstractInsertExecutor` now defaults `needRegister` to `false`. Before this
patch, remote OLAP inserts with `jobId != -1` were registered in the local
`LoadManager`; after this change they are not. That is a behavior regression
for normal remote inserts, and it also means `applyInsertPlanStatistic()` can
no longer find the local load job by id. If the intent is to preserve the
previous remote OLAP behavior, this path needs the same explicit registration
decision as the local `OlapInsertExecutor` path.
--
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]
