Gabriel39 commented on code in PR #66825:
URL: https://github.com/apache/doris/pull/66825#discussion_r3840683378
##########
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:
Thanks. This is a V1 compatibility issue in the position-delete/File Scanner
V1 path. Per the current PR scope, compatibility fixes and additional File
Scanner V1 issues are intentionally not being addressed here, so no code change
is planned for this thread.
##########
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:
Fixed. executeEmptyInsert now runs beforeExec inside the same cleanup scope
as completion, so a preflight failure reaches onFail and removes the external
transaction. Added a failing-begin empty-insert regression test that verifies
rollback and removal.
##########
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:
Fixed. Hive, Paimon, and MaxCompute now serialize report attachment with a
lifecycle closing fence and reject retained final-report references after
commit or rollback starts. Added barrier-ordering coverage for all three
ownership-bearing transaction types.
--
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]
