[fix](iceberg) Backport external write hardening to branch-4.1 - #66825
[fix](iceberg) Backport external write hardening to branch-4.1#66825Gabriel39 wants to merge 10 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: this review found 11 actionable correctness and rolling-compatibility defects.
The blocking paths group into four areas: external-file report ownership can delete committed Iceberg files or strand Hive multipart uploads; mixed-version Hive/Iceberg writes can fail after staging or silently use the wrong partition source; Scanner V2 loses or misinterprets Iceberg requiredness and initial defaults; and current nested non-identity merge routing constructs transforms from the wrong type.
Critical-checkpoint summary: reader lifecycle and collection parent-null handling were cleared; async reservation transfer, spill accounting, and dependency wakeups were cleared; overwrite OCC/branch behavior and the orphan-file action remained fail-closed; but schema mapping/default materialization, external compatibility, nested partition routing, and report ownership have the inline blockers below. No additional user-provided focus was supplied. The review converged in Round 2 after both full-scope passes and the separate risk challenge returned no new valuable findings. Per the review workflow, no builds or source modifications were performed.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: this review found seven additional actionable defects.
Critical-checkpoint summary:
- Correctness and data safety: top-level non-finite Iceberg defaults fail in FE before the new BE parser is reachable; orphan deletion can race a concurrent commit; and rejected final reports can leave prepared Paimon files without an abort owner.
- Resource lifetime and admission: cold partition fan-out and the final spill merger can both allocate far beyond their successful reservations.
- External compatibility and performance: a positive equality-delete summary still causes unnecessary remote manifest traversal during rolling-upgrade checks.
- Tests: the expanded complex-evolution suite has stale and missing golden results, so its checked-in expectations cannot pass.
The final-report state machine for the other inspected Iceberg/Hive/MaxCompute paths, nested partition routing, object-store publication, branch overwrite/OCC, reader lifecycle, and collection null handling were cleared. All 115 changed paths were reviewed, all 11 existing inline threads were duplicate-fenced, and two additional concerns were dismissed as pre-existing after authoritative-preimage checks. No additional user-provided focus was supplied. Three full rounds converged unanimously with no new valuable findings in the final round. Per the review instructions, no builds or source modifications were performed.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: seven additional, distinct defects remain (five P1, two P2).
Findings:
- [P1] Failed rewrite actions can permanently retain the new Iceberg commit fence.
- [P1] Target-file rollover can invoke a full sort without reserving destination/permutation workspace.
- [P1] Nontrivial ARRAY/MAP rematerialization still allocates an entry-sized parent mask when no descendant consumes it.
- [P1] Paimon backend close failures are discarded after FE has accepted commit payloads.
- [P1] Cold-writer reserve estimation copies a potentially huge first value before memory admission.
- [P2] The V2 gate rejects selected-snapshot/ref scans based on schemas that cannot contribute reachable files.
- [P2] The V2 gate expands a nested-pruned root to unrequested descendants, so an unused sibling can reject a V1-safe scan.
Review checkpoints:
- Architecture and layer boundaries: traced Scanner/TableReader/mapper/file-reader responsibilities and FE-to-Thrift-to-BE ownership. No separate boundary inversion survived review; the schema-gate and materialization defects are called out inline.
- Reader and transaction lifecycle/contracts: checked split/EOF/error cleanup, async queue tokens, report ACK/retry/finalizers, Iceberg fence/OCC paths, and writer close/rollback. The rewrite-fence leak and post-ACK Paimon close regression remain blocking.
- Schema mapping/materialization: checked stable field IDs, recursive defaults/optionality/binary markers, nested projection, ARRAY/MAP offsets/null maps, selected schemas, and partition paths. Three projection/materialization defects remain inline.
- Filtering, deletes, and pushdown: traced predicates plus equality/position-delete paths and row/schema alignment; no additional nonduplicate issue survived beyond prior live threads and the V2 projection gates above.
- Format boundaries and external compatibility: checked Iceberg V1/V2 rolling upgrade, time travel/refs, external schema evolution, Hive/Paimon reporting, nested partitions, and Azure/S3 multipart identity. The two mixed-version false rejections are inline; other reproduced compatibility concerns are already threaded.
- Performance and observability: reviewed admission, sorter rollover/spill/final merge, hot-path collection masks, report-size handling, counters, and cleanup visibility. Three unreserved/unbounded allocation paths remain inline.
- Tests: reviewed all changed BE/FE/regression tests. Coverage does not exercise the seven reported witnesses; no builds or tests were run, as required by the review prompt.
- User focus: no additional user-provided focus was supplied.
Status: capped/incomplete after the maximum three rounds because the final normal and final risk passes both produced new distinct findings; this is not a convergence claim.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
FE UT Coverage ReportIncrement line coverage |
|
/review |
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Request changes: five additional, distinct P1 defects remain.
Findings:
- Iceberg admission still misses the pre-append per-column capacity rollover sort.
- A new BE can mistake a legacy FE's transport OK for durable Hive/Paimon/MaxCompute ownership acceptance.
- Expiring an ancestor snapshot can truncate the schema-provenance walk while files written under that schema remain live.
- Rewrite cache invalidation can report failure after the remote Iceberg commit is already durable.
- A swallowed rewrite-group failure can be counted as success, allowing source files to be deleted without replacements.
Critical-checkpoint summary:
- Architecture and lifecycle: external-report ownership and both rewrite transaction boundaries have the three blocking defects above; new-FE retry identity/publication and the other writer-finalizer paths were cleared.
- Schema mapping and mixed-version scanning: recursive defaults, requiredness transport, collection null propagation, projected IDs, and backend selection were cleared, but expired-parent provenance can under-gate V2 semantics.
- Memory and performance: cold-writer fanout, target-size/EOS/final-merge accounting, async reservation transfer, and prior allocation issues were rechecked; the separate pre-append capacity rollover remains unreserved.
- Filtering, deletes, and destructive/object-store paths: equality/position-delete handling, local maintenance fencing, and exact Azure block identity yielded no new nonduplicate issue beyond existing live threads.
- Tests: the changed tests do not exercise the five reported witnesses; no builds or tests were run, as required.
- User focus: no additional user-provided focus was supplied.
Status: converged in Round 3 after both normal full-scope passes and the final risk challenge returned NO_NEW_VALUABLE_FINDINGS.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: four additional, distinct P1 defects remain.
Findings:
- A cancellation report can terminalize external-file ownership before the async Hive writer registers its S3 MPU cleanup owner.
- Cross-partition revocation can sort one Iceberg writer concurrently with async dispatch on another while admission reserves only one transient workspace.
- The new insert retry reset erases MTMV's captured base-table snapshots before the first planning attempt.
- Nested complex default reconstruction can bind a renamed field to a reused sibling name through alias-first element ordering.
Critical-checkpoint summary:
- External-file ownership and provider lifecycle: ACK/rejection/ambiguity, size guards, FE retry caching, Iceberg/Paimon/MaxCompute ownership, and exact Azure/S3 identities were traced; the late Hive cancellation owner remains blocking.
- Memory and concurrency: reservation-token transfer, queue admission, cold fan-out, append/target/EOS/final-merge paths, and revocation were traced; cross-writer revocation still violates the single-workspace bound.
- Format V2 schema mapping and external compatibility: recursive defaults, requiredness, binary/non-finite values, projection, equality/position deletes, schema history, and mixed-version gating were traced; the nested renamed/reused complex-type binding remains blocking.
- Iceberg transaction lifecycle, nested partition routing, and orphan cleanup yielded no additional nonduplicate issue beyond existing live threads; MTMV snapshot pinning has the separate first-attempt regression above.
- Tests: the changed coverage does not exercise the cancellation barrier, cross-writer overlap, MTMV injected-snapshot fence, or reordered complex alias collision.
No additional user-provided review focus was supplied. The review converged in Round 2 after both normal full-scope domain passes and the independent risk challenge returned NO_NEW_VALUABLE_FINDINGS. No builds or source modifications were performed, per the review contract.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review result: request changes.
I found three distinct correctness/lifecycle issues: one unreserved non-spill merge allocation, a stale default orphan-file preview, and a lost rewrite cancellation during planning.
Critical checkpoint summary:
- BE memory and ownership: async admission, reservation transfer, dispatch/revocation locking, external report aggregation/ACK/retry, and late cleanup ownership were traced. Existing live threads cover the other previously raised cases; the non-spill EOS merge-output reservation gap remains.
- FE transactions and maintenance: commit fencing, rollback/quiescence, report acceptance, cache/MVCC, and nested routing were traced. The dry-run refresh and cancellation-handoff issues remain.
- Schema and compatibility: nested defaults/requiredness, field identity, projection/null maps, delete paths, nested partitions, Thrift carriers, and mixed-version gates were rechecked with no additional non-duplicate defect.
- User focus: no additional user-provided review focus was supplied.
No builds or tests were run because the review instructions prohibit them.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Found six blocking correctness and memory-accounting issues:
- Sort admission reduces retained sorted destinations across partitions to one transient maximum.
- Spill-merge admission omits the serialized buffer and parsed protobuf retained by every input reader.
- A whole-block average cannot bound an isolated oversized scalar or nested row.
- Nested identity-partition overwrite and RowDelta predicates bind a leaf name instead of the full schema path.
- Static syntax for a nested identity partition can label a file with a value different from the unchanged row leaf written into it.
- The mixed-version scan gate omits schema provenance of files imported by a non-fast-forward cherry-pick.
I reviewed the full 139-file authoritative diff, the existing threads and review bodies, the required FileScanner V2 guidance, and the focused tests and ownership paths. Two independent convergence rounds completed; the final BE, external-ownership, Iceberg/FE, and risk-focused passes all returned NO_NEW_VALUABLE_FINDINGS. Existing orphan-deletion, rewrite-quiescence, caller-owned MVCC retry, and complex-default concerns were duplicate-fenced rather than resubmitted.
User focus: no additional user-provided review focus was specified. Builds and tests were not run because the review instructions prohibited builds.
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
2 participants
What problem does this PR solve?
This is a semantic backport of the external-write and schema-evolution fixes from #66345, #66348, #66408, #66529, and #66750 to
branch-4.1.The source PRs were implemented on the connector-plugin architecture, while
branch-4.1still uses the legacy external-table stack. This change ports the invariants and failure handling to the branch-specific owners instead of mechanically cherry-picking plugin classes.Key behavior preserved on
branch-4.1:The Paimon validation-detail fix from #66408 and canonical
Paimoncapitalization from #66750 are already present in the legacy Paimon paths onbranch-4.1; their observable contracts were verified and no plugin-only code was introduced.Architecture-specific notes
Tests
SuiteJobLookupTest: 1 test passed.The full local BE unit-test link is unavailable because the configured third-party Arrow bundle lacks a header required by an unchanged source file; targeted compilation covers every changed translation unit.
Pick from master: #66345 #66348 #66408 #66529 #66750