github-actions[bot] commented on code in PR #66825:
URL: https://github.com/apache/doris/pull/66825#discussion_r3840599264
##########
be/src/format_v2/table/iceberg_position_delete_sys_table_reader.cpp:
##########
@@ -591,6 +594,9 @@ Status
IcebergPositionDeleteSysTableV2Reader::_build_delete_file_projected_colum
columns->push_back(*it);
columns->back().type = column.type;
set_iceberg_delete_field_id(&columns->back());
+ // The copied row tree bypasses
IcebergTableReader::annotate_projected_column, so prepare its
+ // typed nested defaults before the generic inner reader builds
the column mapper.
+
RETURN_IF_ERROR(prepare_iceberg_initial_default_exprs(&columns->back()));
Review Comment:
[P1] Preserve V1 defaults in the position-delete row path
This copied `row` tree also receives semantics-V1 descriptors from an old
FE, but omitting the second argument makes
`prepare_iceberg_initial_default_exprs()` parse every complex default as
semantics-V2 single-value JSON. V1 transports complex defaults in Iceberg's
human form (for example `{value=7}`, `[foo]`, or `{key=7}`), so after the BEs
are upgraded an old FE can assign this scan to a new BE and the inner reader
fails to open an evolved position-delete file. The ordinary reader now gates
this decoder on `supports_iceberg_scan_semantics_v2()`; apply the same V1/V2
boundary here and add old-FE-V1 to new-BE STRUCT/LIST/MAP coverage for
`position_deletes.row`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/insert/AbstractInsertExecutor.java:
##########
@@ -257,8 +261,9 @@ private void checkStrictModeAndFilterRatio() throws
Exception {
* execute insert txn for insert into select command.
*/
public void executeSingleInsert(StmtExecutor executor) throws Exception {
- beforeExec();
try {
+ // Pre-execution work may register external resources, so it must
share the transaction cleanup scope.
+ beforeExec();
Review Comment:
[P1] Put the empty-insert preflight in the same cleanup scope
This move protects `executeSingleInsert()`, but `executeEmptyInsert()` still
invokes the same `beforeExec()` before entering its `try`. That path is
reachable for an empty Iceberg static-partition overwrite after `initPlan()`
has already called `beginTransaction()`, which publishes the transaction in
both manager maps. If `IcebergTransaction.beginInsert()` then fails (for
example during branch validation, authentication, or SDK transaction creation),
it releases its fence but the exception bypasses `onFail()` and
`transactionManager.rollback()`, so both entries remain and the
coordinator/profile cleanup is skipped. Move the empty-path call inside its
cleanup scope too (or share a common wrapper), and add a failing-begin
empty-overwrite test that asserts the transaction is removed.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/QeProcessorImpl.java:
##########
@@ -277,22 +284,72 @@ public TReportExecStatusResult
reportExecStatus(TReportExecStatusParams params,
}
}
+ boolean hasExternalCommitData = hasExternalCommitData(params);
+ // Legacy BEs may attach the same vectors to periodic reports and
resend them at EOS. Only
+ // the final report transfers ownership, so rollout must not reject or
cache the preview.
+ boolean transfersExternalFileOwnership = hasExternalCommitData &&
params.isDone();
+ String reportKey = transfersExternalFileOwnership ?
externalFileReportKey(params) : null;
+ if (transfersExternalFileOwnership && reportKey == null) {
+ return rejectedExternalFileReport(result, "External-file report is
missing its identity fields");
+ }
+ if (transfersExternalFileOwnership &&
acceptedExternalFileReports.getIfPresent(reportKey) != null) {
+ // Keep acceptance available after coordinator removal so a lost
response is retry-safe.
+ result.setStatus(new TStatus(TStatusCode.OK));
+ result.setExternalFileCommitDataAccepted(true);
+ return result;
+ }
+
final QueryInfo info = coordinatorMap.get(params.query_id);
result.setStatus(new TStatus(TStatusCode.OK));
if (info == null) {
// Currently, the execution of query is splited from the exec
status process.
// So, it is very likely that when exec status arrived on FE
asynchronously, coordinator
// has been removed from coordinatorMap.
- return result;
+ return transfersExternalFileOwnership
+ ? rejectedExternalFileReport(result, "Coordinator no
longer owns this external-file report")
+ : result;
}
try {
- info.getCoord().updateFragmentExecStatus(params);
+ boolean accepted =
info.getCoord().updateFragmentExecStatus(params);
+ if (transfersExternalFileOwnership && !accepted) {
+ return rejectedExternalFileReport(result, "FE has not accepted
the external-file report");
+ }
} catch (Exception e) {
LOG.warn("Exception during handle report, response: {}, query: {},
instance: {}", result.toString(),
DebugUtil.printId(params.query_id),
DebugUtil.printId(params.fragment_instance_id), e);
- return result;
+ return transfersExternalFileOwnership
+ ? rejectedExternalFileReport(result, "FE did not accept
the external-file report")
+ : result;
}
result.setStatus(new TStatus(TStatusCode.OK));
+ if (transfersExternalFileOwnership) {
+ // Publish the retry token before replying; a transport loss
cannot revoke FE ownership.
+ acceptedExternalFileReports.put(reportKey, Boolean.TRUE);
+ result.setExternalFileCommitDataAccepted(true);
Review Comment:
[P1] Reject ownership reports after Hive rollback starts
A timeout can reach `executeSingleInsert()`'s `onFail()` while this
final-report handler already holds the `HMSTransaction`. Pause after
`getTxnById()`, let rollback inspect the still-empty update list and remove the
transaction, then resume `updateHivePartitionUpdates()`: unlike Iceberg, Hive
has no closing/accepting-data guard, so the append to the detached object
succeeds and this line publishes `external_file_commit_data_accepted=true`. BE
then relinquishes its MPU abort owner even though FE rollback already ran and
will never see those uploads. `PaimonTransaction` has the same post-rollback
append window. Please make report attachment atomic with closing for every
ownership-bearing transaction and cover this barrier ordering.
--
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]
