github-actions[bot] commented on code in PR #65476:
URL: https://github.com/apache/doris/pull/65476#discussion_r3566353034
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/AbstractInsertExecutor.java:
##########
@@ -107,8 +108,18 @@ public AbstractInsertExecutor(ConnectContext ctx, TableIf
table, String labelNam
*/
public AbstractInsertExecutor(ConnectContext ctx, TableIf table, String
labelName, NereidsPlanner planner,
Optional<InsertCommandContext> insertCtx, boolean emptyInsert,
long jobId, boolean needRegister) {
+ this(ctx, table.getDatabase(), table, labelName, planner, insertCtx,
emptyInsert, jobId, needRegister);
Review Comment:
This overload still lets normal insert retry attempts recover the database
from the stale `Table` object before the new database/table ID recheck runs. In
`initPlan()`, `ExecutorFactory.build()` constructs the executor before the
second `getTarget()` comparison; the OLAP path still passes only the original
`targetTableIf`, so this constructor calls `table.getDatabase()` and, with
`needRegister`, immediately adds an `InsertLoadJob`. If the original
database/table was dropped and recreated while planning, `Table.getDatabase()`
resolves the old table's `qualifiedDbName` against the current catalog and can
register a load job under the replacement database even though the retry check
later sees the ID mismatch and continues. Please either pass the captured
`InsertTarget` database into the normal executor constructors too, or delay
executor construction/load-job registration until after the target identity
check has proven this planned target is still current.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/InsertIntoTableCommand.java:
##########
@@ -308,13 +333,17 @@ public void beforeComplete(AbstractInsertExecutor
insertExecutor, StmtExecutor e
}
// lock after plan and check does table's schema changed to ensure
we lock table order by id.
- TableIf newestTargetTableIf = getTargetTableIf(ctx,
qualifiedTargetTableName);
+ InsertTarget newestTarget = getTarget(ctx,
qualifiedTargetTableName);
+ DatabaseIf<?> newestTargetDatabase = newestTarget.getDatabase();
+ TableIf newestTargetTableIf = newestTarget.getTable();
newestTargetTableIf.readLock();
try {
- if (targetTableIf.getId() != newestTargetTableIf.getId()) {
- LOG.warn("insert plan failed {} times. query id is {}.
table id changed from {} to {}",
+ if (targetDatabase.getId() != newestTargetDatabase.getId()
Review Comment:
This retry branch needs to clear per-attempt state before `continue`.
`analyzeGroupCommit()` runs earlier in the same loop and can set
`ctx.groupCommit` from the target being planned; if that target is then
rejected here because the database/table ID changed, the next retry reuses the
same `ConnectContext`. Since `analyzeGroupCommit()` returns immediately when
the flag is already true, the replacement table is never rechecked for
schema-light-change, partition, MTMV, transaction, or plan-shape eligibility,
and the planner/executor can still take the group-commit path for a target that
would not qualify. Please reset/recompute the group-commit flag when discarding
a retry attempt, while preserving the HTTP stream-load case that intentionally
pre-sets it.
##########
fe/fe-core/src/main/java/org/apache/doris/dictionary/DictionaryManager.java:
##########
@@ -464,8 +491,7 @@ public void dataLoad(ConnectContext ctx, Dictionary
dictionary, boolean adaptive
lockRead();
boolean unlocked = false;
try {
- if (!dictionaryIds.containsKey(dictionary.getDbName())
- ||
!dictionaryIds.get(dictionary.getDbName()).containsKey(dictionary.getName())) {
+ if (!isCurrentDictionaryWithoutLock(dictionary)) {
Review Comment:
There is another cleanup mismatch in this same post-load section for partial
refreshes. When `statementContext.isPartialLoadDictionary()` is true, the sink
staged `dictionary.getVersion()` on the selected BEs, not `version + 1`;
`DictionarySink.toThrift()` only uses `version + 1` for full refreshes. But if
`commitNowVersion()` fails, this branch skips the full-load decrement and still
calls `abortSpecificVersion(..., dictionary.getVersion() + 1)`. BE abort only
erases the refreshing dictionary when the version matches exactly, so a failed
partial load can leave the staged current-version refresh in
`_refreshing_dict_map`. Please abort `dictionary.getVersion()` for partial
loads and `dictionary.getVersion() + 1` for full loads.
--
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]
