Repository navigation
[fix](iceberg) Commit every Iceberg file under the table's own location - #69392
Merged
morningman merged 12 commits intoNov 4, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69082 Problem Summary: DELETE, UPDATE and MERGE on an Iceberg table whose location FE normalizes for BE (oss://, cos://, cosn://, obs://, bos://, gs://, s3a://, s3n://, and Azure abfs/abfss/wasb/wasbs except OneLake) record the new delete files in Iceberg metadata with the internal s3:// address instead of the table's own scheme. Both v2 position delete files (delete_pos_*.parquet) and v3 deletion vector files (delete_dv_*.puffin) are affected; data files are not. Reproduce on an Iceberg table located at oss://bucket/...: CREATE TABLE t (id INT, v STRING) PROPERTIES ('format-version' = '2'); INSERT INTO t VALUES (1, 'a'), (2, 'b'), (3, 'c'); DELETE FROM t WHERE id = 1; SELECT file_path FROM t$delete_files; -- s3://bucket/.../data/delete_pos_xxx.parquet SELECT file_path FROM t$data_files; -- oss://bucket/.../data/... Doris itself still reads the table, because it normalizes every path it reads anyway, but other engines resolve the file system by scheme and fail: Spark or Trino reading the table, or running expire_snapshots / remove_orphan_files, look the delete file up as s3 (e.g. OSSFileIO rejects the s3 scheme, HadoopFileIO usually has no file system for it, ResolvingFileIO goes to AWS S3, ADLSFileIO rejects it as an invalid ADLS URI). Cause: data files are written under write_path (the s3:// form BE can address) and committed under original_write_path (VIcebergPartitionWriter). VIcebergDeleteSink had no such split: it generates delete file paths under output_path and reports exactly that path for the commit (VIcebergDeleteFileWriter sets file_path to its output path, and the puffin path is reported as written). Fix: apply the same split to delete files. VIcebergDeleteSink still writes under output_path but commits under table_location, the raw data location FE already sends, e.g. written to s3://b/db/t/data/delete_x.parquet and committed as oss://b/db/t/data/delete_x.parquet. Every FE version sets table_location to the raw form of output_path's directory (delete sink and merge sink alike), so no protocol change is needed. That contract is now written on table_location in DataSinks.thrift (comments only) and asserted by an FE unit test, since breaking it would record delete files that do not exist without any error. Delete files that an older version already committed with s3:// keep that address until they are rewritten. ### Release note Fix Iceberg DELETE/UPDATE/MERGE recording delete files with an s3:// path instead of the table's own location (e.g. oss:// or abfss://), which made other engines fail to read the table. ### Check List (For Author) - Test: Unit Test (VIcebergDeleteSinkTest.TestCommitPathUsesTableLocation, IcebergWritePlanProviderTest.planWriteDeleteAndMergeSinksKeepTableLocationInTheOutputDirectory; not run locally, CI runs them) - Behavior changed: Yes. Delete files are committed under table_location. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28771 ms |
Contributor
TPC-DS: Total hot run time: 151114 ms |
Contributor
ClickBench: Total hot run time: 25.21 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#69392 Problem Summary: A helper-only path test cannot detect missing conversion at the position-delete or deletion-vector commit sites. Write a real local Parquet position-delete file with a distinct OSS commit location and check the final commit metadata and the file's referenced data paths. Extend the shared-Puffin test with a distinct Azure commit location while retaining its physical-file and footer checks. Compute the shared Puffin commit path once before iterating over its blobs. ### Release note None ### Check List (For Author) - Test: Unit Test added/updated, not compiled or run locally; the user handles compilation. clang-format v16, check-format, build hygiene and git diff --check passed. - Behavior changed: No. The existing delete-file commit path fix is preserved. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28713 ms |
Contributor
TPC-DS: Total hot run time: 150929 ms |
Contributor
ClickBench: Total hot run time: 24.26 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The production change consistently restores the table's raw data-location URI for both position-delete files and deletion vectors. One P2 issue is reported inline: the new real-Parquet test leaves ExecEnv's Arrow memory pool uninitialized, so dictionary encoding dereferences a null pool. No P0/P1 issue was verified, and there are no existing review threads to carry forward.
Critical checkpoint conclusions:
- Goal and proof: the shared helper corrects commit metadata while retaining physical write paths; BE helper/v2/v3 tests and the FE directory-contract test target this behavior. The v2 write test needs the inline setup correction.
- Scope and reuse: one private helper serves both formats; the change is focused and follows the data writer's existing original/write-path split.
- Concurrency and locks: one AsyncResultWriter worker serializes open/write/close under its existing task context. Locations are initialized before use; no new shared mutation, lock order, or blocking critical section is introduced.
- Lifecycle and static initialization: physical-file ownership and deferred MERGE cleanup remain intact; no new static initialization or ownership cycle is added. Test environment initialization is the reported issue.
- Configuration: no configuration is added; existing custom data-location precedence remains intact.
- Compatibility and storage: no file payload format or public protocol symbol changes. Current producers derive both locations from the same captured directory; older Hadoop-Path producers are excluded by the existing coordinator ACK capability fence before writing.
- Thrift/protobuf: both touched sink definitions were compared on HEAD and branch-3.1/4.0/4.1/4.2. They are absent on 3.1/4.0; shared IDs, meanings, types, and optionality agree on 4.1/4.2. This patch changes comments only.
- Parallel paths: v2 position deletes, v3 shared Puffin output, direct DELETE, and UPDATE/MERGE forwarding were checked. Both cloud and shared-nothing use this external write path; internal MoW/version invariants do not apply.
- Conditions and errors: missing-location fallback matches both generators; their common base guarantees the prefix assertion. Comments explain the concrete URI failure. Existing Status propagation is preserved.
- Test coverage: new assertions cover raw commit URIs, unchanged referenced data-file URIs, partitions, shared-Puffin offsets/footer linkage, and trailing slashes. The FE fixture covers OSS normalization; actual Azure/cloud services and missing-output-path fallback are not newly exercised.
- Test results: no golden result files change. Builds and tests were not run under this static-only review contract; no author-reported execution was treated as independently verified.
- Observability: existing write/close logs, counters, and commit error context remain adequate for this path-string correction.
- Transactions, persistence, and failover: existing RowDelta publication and conflict validation remain unchanged; no new EditLog/replay state is introduced. Raw data-file identity and Puffin path-plus-offset deduplication are preserved.
- Writes, atomicity, and failures: write/report failure cleanup and MERGE outer-close cleanup retain the physical filesystem/path pair. The patch introduces no new crash ownership or partial-publication window; existing transaction-abort orphan behavior is unchanged.
- FE/BE variables: no fields are added. Direct and MERGE plans forward both existing optional locations, and future delete-file reads normalize the corrected raw URI before BE reopening.
- Performance and memory: only URI-sized temporaries are added per position-delete file or shared Puffin; no new bulk retention, scan, or allocation hotspot was found.
- Other concerns and focus: no additional user focus was supplied. URI authorities/escaping, custom directories, legacy producers, cleanup, and test dependencies received an additional sweep; no other verified defect remains.
Review coverage is complete for the supplied five-file diff and relevant static control flow. Validation limits are the prohibited runtime/build/test execution and lack of real cloud or cross-engine execution; there are no unresolved static candidates.
Review converged after 2 round(s).
| it->second.rows_to_delete.add(uint32_t {10}); | ||
| it->second.rows_to_delete.add(uint32_t {20}); | ||
|
|
||
| const auto status = sink->_write_position_delete_files(file_deletions); |
Contributor
There was a problem hiding this comment.
[P2] Initialize the Arrow memory pool before the Parquet write
This new test reaches the real Parquet writer, but its fixture never initializes ExecEnv::_arrow_memory_pool: it defaults to null, and run_all_tests.cpp only calls init_mem_tracker(), which does not set it. VIcebergDeleteFileWriter enables dictionary encoding, and VParquetWriter::_parse_properties() explicitly forwards that null pool into WriterProperties. In Arrow 24, the dictionary encoder retains it in buffered_indices_; the first value's insertion reaches the STL allocator, which calls pool_->Allocate without a null fallback. Thus running this test in isolation crashes before it can verify the commit-path assertions. Save, initialize, and restore the pool as VParquetWriterTest::SetUp/TearDown already does.
…elete write test ### What problem does this PR solve? Issue Number: None Related PR: apache#69392 Problem Summary: `TestWritePositionDeletesCommitsTableLocation` runs the real Parquet writer. The writer enables dictionary encoding and passes `ExecEnv::_arrow_memory_pool` to Arrow, but this fixture never initializes that pool. The test run alone therefore dereferences a null pool on the first dictionary insert. The test now sets the pool to Arrow's default pool and restores it afterwards, as `VParquetWriterTest` and `ArrowSchemaUtilTest` do. ### Release note None ### Check List (For Author) - Test: Unit Test (compiled and run in CI) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
…location ### What problem does this PR solve? Issue Number: None Related PR: apache#69392 Problem Summary: BE writes every Iceberg file under the location form it can address (output_path, e.g. s3://b/db/t/data for an oss:// table), while Iceberg metadata must record it under the table's own form (original_output_path or table_location, e.g. oss://b/db/t/data). The two writers did this in two different ways: - VIcebergTableWriter assembled both directories for every partition writer, in three copied branches of _create_partition_writer. - VIcebergDeleteSink generated only the write path and rebased it into the commit form afterwards (_commit_path_of, added earlier in this PR), while VIcebergDeleteFileWriter still reported its write path as file_path and relied on the caller to overwrite it. Nothing made a new writer, or a new kind of delete file in the delete sink, report the right form, and getting it wrong was silent: that is how delete files came to be committed as s3://. This commit gives both forms one owner: - IcebergOutputLocation holds a directory in both forms and gives the write path and the commit path of a file name, and the same pair for a partition subdirectory. It is header-only, so no unity batch moves. - The partition writer's WriteInfo carries one location instead of write_path, original_write_path and target_path (target_path was never read). VIcebergTableWriter builds the data location once in open() and takes each partition directory from it. - VIcebergDeleteFileWriter takes the location and a file name and reports the commit path itself. The delete sink names its position-delete and Puffin files and takes both paths of each from its location, so _commit_path_of goes away. - A table or delete sink without both location fields now fails in open() / init_properties() instead of committing the write path (delete sink) or a relative path such as /p=1/x.parquet (table sink). The FE of every branch (3.1, 4.0, 4.1, 4.2, master) sets both fields on every Iceberg sink, so no plan a supported FE sends is rejected. - Directories join file names with exactly one '/'. Data files under a write.data.path ending in '/' were placed at .../data//p=1/x.parquet in both forms; they now go to .../data/p=1/x.parquet, as Iceberg's own location provider places them. Delete files already joined this way. ### Release note Iceberg data files written under a write.data.path that ends with '/' no longer get a double '/' in their path. ### Check List (For Author) - Test: Unit Test - New IcebergOutputLocationTest; new cases in VIcebergPartitionWriterTest, VIcebergTableWriterTest, VIcebergDeleteSinkTest and VIcebergMergeSinkTest (data files and the merge sink's inner sinks write under output_path and commit under the table's form; plans without both location fields are rejected). - run-be-ut.sh --filter='*Iceberg*:VParquetWriterTest.*': 318 tests from 35 suites pass, including every new case. The tests that write files also pass each in a process of its own: VIcebergDeleteSinkTest.TestWritePositionDeletesCommitsTableLocation and the new cases in VIcebergTableWriterTest, VIcebergPartitionWriterTest and VIcebergMergeSinkTest. - Behavior changed: Yes. A sink plan without both location fields is rejected (no supported FE sends one). Data files under a write.data.path ending in '/' no longer get a double '/'. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…k's location ### What problem does this PR solve? Issue Number: None Related PR: apache#69392 Problem Summary: FE took every file path BE reported for an Iceberg write as it was: IcebergWriterHelper wrote commitData.getFilePath() into the DataFile / DeleteFile, and rollback() deleted that path through the table's FileIO. Nothing checked that the path was in the table's own location form. A writer that reported the address BE wrote the file at (the s3:// form FE normalizes oss://, cos://, cosn://, obs://, bos://, gs://, s3a://, s3n:// and Azure locations to) committed files that other engines could not open, and no test noticed. The Iceberg REST fixture's warehouse is s3a://, which is normalized to s3://, so the DML suites ran into exactly this case; the test_iceberg_update_delete_basic.out.org left in the repository records delete files as s3://warehouse/wh/... for tables located at s3a://warehouse/wh/.... Fix: - planWrite registers on the transaction the raw location every sink it builds carries: TIcebergTableSink.original_output_path, TIcebergDeleteSink.table_location, and both fields of TIcebergMergeSink. commitLocationsOf reads them back from the built sink, so the registered value is exactly what BE receives. - IcebergConnectorTransaction.addCommitData rejects a fragment whose file_path is not under one of them, or that has no path. This is where every report from every writer, current or future, enters FE. - Rejecting the report fails the statement cleanly: FE answers the report as not accepted (QeProcessorImpl), and BE deletes the files under the paths it wrote them at and cancels the query. A check at commit time instead would leave rollback() deleting the wrong path, i.e. orphan files. - DataSinks.thrift comments on original_output_path, table_location and TIcebergCommitData.file_path state the contract, including that "the same directory" can differ in more than the scheme (Azure moves the account out of the authority). Comments only, no protocol change. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergConnectorTransactionTest: files under every registered location are accepted; the normalized s3:// address, a sibling directory sharing the prefix, a fragment without a path and a fragment before any sink registered a location are rejected and not accumulated. - IcebergWritePlanProviderTest: INSERT, REWRITE, DELETE and UPDATE plans each register their sink's location, so a file under the oss:// data location is accepted and the s3:// form is rejected. - All 1634 tests of fe-connector-iceberg pass (5 skipped: live connectivity checks). Removing the registration in planWrite, or the check in addCommitData, makes the new cases fail. - Behavior changed: Yes. An Iceberg write whose BE reports a file outside the location its sink carries now fails instead of committing that path. Upgrade BE before FE: an upgraded FE rejects the s3:// delete file paths that a BE without the fix in this PR reports, so DELETE / UPDATE / MERGE on a table whose location FE normalizes fails until that BE is upgraded. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…port ### What problem does this PR solve? Issue Number: None Related PR: apache#69392 Problem Summary: When FE rejects a final external-file report (for example the Iceberg commit-location check finds a file outside the sink's commit location), QeProcessorImpl only answered BE with external_file_commit_data_accepted=false. The fragment was never marked done and the coordinator status was never set. BE treats the rejection as terminal: it removes the files and never resends the report. The coordinator join therefore kept waiting on the fragment latch until insert_timeout (4 hours by default), and the user saw a timeout instead of the real reason. Now QeProcessorImpl cancels the coordinator with the rejection reason before it answers BE. Both coordinators' cancel release the fragment latch, so the INSERT fails immediately with the validation error and the transaction is aborted. ### Release note None ### Check List (For Author) - Test: Unit Test (QeProcessorImplReportAckTest, run by CI) - Behavior changed: Yes. A rejected final external-file report now fails the statement immediately instead of waiting until timeout. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28246 ms |
Contributor
TPC-DS: Total hot run time: 151011 ms |
Contributor
ClickBench: Total hot run time: 25.1 s |
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28505 ms |
Contributor
TPC-DS: Total hot run time: 152067 ms |
Contributor
ClickBench: Total hot run time: 25.29 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Local pipeline review — ✅ PASSNotes for maintainers
Reviewed locally with the |
morningman
approved these changes
Nov 4, 2026
924060929
approved these changes
Nov 4, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
Iceberg files must be written through normalized storage locations while metadata retains the table's original URI. This PR centralizes both paths, registers each sink's original directory, validates incoming commit fragments, and cancels the statement when FE rejects a report.
Static review of c8d9172 against 8e10b23 found no blocking issues in the FE changes and necessary BE path/report/cleanup contracts. Sink coverage, directory boundaries, synchronization and failure propagation are consistent. Both previous inline findings are fixed.
APPROVE within this scope. No builds or tests were run. The supplied snapshot shows successful P0, compilation, FE/BE unit tests and external regression, but FE coverage failed and required-check rules were not supplied. This is not a full BE review or merge-readiness certification.
Automated FE-focused static review (gpt-6.1-sol, high). No build or tests were run.
xylaaaaa
pushed a commit
to xylaaaaa/doris
that referenced
this pull request
Nov 4, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69392 Problem Summary: The backported file-location regression was missing from the Iceberg P0 coverage matrix. Add risk R19, case W20, and the suite mapping from upstream so the matrix includes all supported write operations. ### Release note None ### Check List (For Author) - Test: Manual test; verified the coverage mapping against the backported regression and upstream PR. git diff --check passed. - Behavior changed: No - Does this need documentation: Yes; update the requested existing coverage matrix in this commit.
xylaaaaa
pushed a commit
to xylaaaaa/doris
that referenced
this pull request
Nov 4, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69392 Problem Summary: The 4.2 coverage fixture bypasses planning and constructs Iceberg transactions directly. It never registers a raw sink location, so the backported validation rejects its s3://bucket/events files before the existing transaction assertions run. Ten of eleven transaction coverage tests failed. Register the matching table, delete, or merge sink in beginWrite, adapting upstream 52c04ac to the release branch. Also add risk R19, case W20, and the file-location suite to the existing P0 coverage matrix. ### Release note None ### Check List (For Author) - Test: Unit Test; run-fe-ut.sh executed 117 tests across 15 classes, with 111 passing, 6 pre-existing disabled tests skipped, and no failures or errors. The transaction coverage class passed all 11 tests; all 4 known-bug cases remain disabled. Maven Checkstyle and git diff --check passed. - Behavior changed: No - Does this need documentation: Yes; update the requested existing coverage matrix in this commit.
3 of 16 tasks
csun5285
added a commit
to csun5285/doris
that referenced
this pull request
Nov 4, 2026
…eKeyIterator ### What problem does this PR solve? Issue Number: None Related PR: apache#69515 Problem Summary: SameKeyIterator had two behaviors switched by MergeDecision::drop_older_versions: for merge-on-read UNIQUE_KEYS it kept the first, newest, row of every key and dropped the others, and for AGG_KEYS, a sequence map and changelog scans it flagged the rows of one key for the fold above. One class with two modes needed a DORIS_CHECK to stop the flagging call in the dropping mode, and its name only described the second one. After the rebase onto master, UniqueKeyBlockIterator did nothing on its own either: the older versions were dropped below it, and master's row TTL change (apache#65858) moved the delete-sign filter of the compaction and checksum reads into BlockReader::next_block_with_aggregation. It was a second DirectBlockIterator. Now: - UniqueKeyIterator (unique_key_iterator.h) keeps only the newest row of every key and counts the others in merged_rows(). BlockReader and BetaRowsetReader::new_iterator (schema change, TopN) put it above the merge when the read's drop_older_versions is set. - SameKeyIterator only flags the rows of one key; it takes the key ordinals, not the decision. - Both share SameKeyFlags, as the key group of a vertical compaction does. - UniqueKeyBlockIterator is removed: a merge-on-read UNIQUE_KEYS read is a DirectBlockIterator over the UniqueKeyIterator, with the delete sign filtered in BlockReader as before. So a merge-on-read UNIQUE_KEYS read is: VHeapMergeIterator sorts, UniqueKeyIterator keeps the newest row of every key, DirectBlockIterator hands the Blocks out. ### Release note None ### Check List (For Author) - Test: Unit Test - BlockReaderUniqueKeysTest checks the DirectBlockIterator over a UniqueKeyIterator; VGenericIteratorsTest and BlockReaderChangelogMergeTest build the merge the way the readers do - 363 tests of the iterator, compaction, rowid conversion, tablet reader, schema change and row TTL suites pass (built with format/transformer/vparquet_writer_test.cpp set aside: it does not compile on master since apache#69392 changed VIcebergPartitionWriter's WriteInfo) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
morningman
added a commit
that referenced
this pull request
Nov 4, 2026
…berg WriteInfo (#70209) ### What problem does this PR solve? Issue Number: None Related PR: #69392, #69971 Problem Summary: **In short.** `doris_be_test` does not compile on master since #69971 was merged, so the BE UT of every PR fails with `BE BUILD FAIL`. This fixes the one test case that breaks it. **The problem.** #69392 replaced the `write_path`, `original_write_path` and `target_path` fields of `IPartitionWriterBase::WriteInfo` with one `IcebergOutputLocation location`, and updated the callers it knew about. #69971, merged after it, added `VParquetWriterTest.HiveAndIcebergParquetPageIndexOptions`, which still sets the old fields: ``` be/test/format/transformer/vparquet_writer_test.cpp:380:41: error: no matching constructor for initialization of 'VIcebergPartitionWriter' ``` Each PR built alone, so both were green. TeamCity's BE UT has failed this way for PRs merged with master `0eb5131db62` (for example build 1092024 of #69605). **How this PR fixes it.** The case builds its `WriteInfo` as the other Iceberg case in the same file does, `.location = IcebergOutputLocation(".", ".")`. Nothing else changes.
hubgeter
added a commit
to hubgeter/doris
that referenced
this pull request
Nov 5, 2026
### Background Iceberg writers carry separate physical and metadata directory forms in IcebergOutputLocation. Their WriteInfo aggregate requires this location to be initialized before a writer can be constructed. ### What problem does this PR solve? Issue Number: None Related PR: apache#69376 (follow-up), apache#69392 (output-location interface change) PR Type: Test Problem Summary: The unsupported-Parquet-codec test still default-constructed WriteInfo and assigned its removed write_path field. After merging current master this prevents the BE unit-test target from compiling. Initialize the physical and metadata directories explicitly while retaining the existing zero-file-creation regression assertions. ### Details - Failure: BE UT compilation reports a deleted WriteInfo default constructor and a nonexistent write_path member in the new codec-rejection test. - Cause: the output-location refactor changed WriteInfo after the PR branched. - Stability evidence: use the same aggregate pattern as current master tests, with both local directory forms set to '.', FILE_LOCAL, and no brokers. Keep the unique filename, SyncPoint callback and all assertions unchanged. ### Testing - clang-format 16 on the four PR files, build hygiene, and git diff --check passed. - git range-diff confirmed the four original patches are preserved by rebase. - ASAN run-be-ut.sh --run --filter=VIcebergPartitionWriterTest.UnsupportedParquetCodecDoesNotCreateFile -j 2 was attempted locally, but stopped during dependency preparation before compilation. Required submodules were incomplete and the mandatory libgoogle_cloud_cpp_oauth2.a is absent. No local runtime test pass is claimed. ### Release note None ### Check List (For Author) - Test: static checks passed; local unit-test execution remains unverified. - Behavior changed: No. - Does this need documentation: No. - Pre-submission status: Partially verified: BE UT compilation and execution need verification on the prepared CI environment. ### AI Disclosure - Tool: Codex. - Generated: test initialization and this commit message. - Human review: Not performed in this automated session.
xylaaaaa
pushed a commit
to xylaaaaa/doris
that referenced
this pull request
Nov 7, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69392, apache#70173, apache#69841, apache#70185 Problem Summary: Merge the latest branch-4.2 into the Iceberg file-location backport. Preserve the session partition-overwrite mode together with bound-sink location registration. Adapt the newly added Parquet writer test to IcebergOutputLocation, and update overwrite fixtures for explicit modes, retained snapshots, and registered commit locations. Check path acceptance and rejection for both static and dynamic overwrite modes. ### Release note None ### Check List (For Author) - Test: Unit Test via run-fe-ut.sh --run (16 related FE classes; 122 passed, 6 pre-existing disabled cases skipped, zero failures or errors); Java Checkstyle; clang-format 16; Parquet test translation-unit syntax check via clang-tidy and the existing ASAN compilation database. BE unit tests and external regressions are delegated to CI. - Behavior changed: No; preserve both the release branch's overwrite behavior and this PR's file-location validation. - Does this need documentation: No
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: #69082
Problem Summary:
In short. DELETE, UPDATE and MERGE on an Iceberg table recorded the delete files they write (v2 position delete files and v3 deletion vector files) under the internal
s3://address BE writes them at, instead of the table's own location (oss://,s3a://,abfss://, ...). Doris could not always read such a table back, and other engines and maintenance procedures resolved those files with the wrong file system. This PR commits delete files under the table's location. It also makes every Iceberg file Doris writes take both of its paths from one place in BE and pass one check in FE, so the next writer cannot repeat the mistake without failing.Background
Every Iceberg sink FE sends carries two forms of the table's data location:
output_path): FE rewritesoss://,cos://,cosn://,obs://,bos://,gs://,s3a://,s3n://and Azureabfs[s]:///wasb[s]://locations (except OneLake) tos3://;original_output_pathonTIcebergTableSink,table_locationonTIcebergDeleteSink, both onTIcebergMergeSink.BE writes each file under the first form and reports it in
TIcebergCommitData.file_path, which FE writes into the manifests as it is. So the reported path must be in the second form.The problem, and what it cost
Reproduce on an Iceberg table located at
oss://bucket/...:No storage properties found for schema: s3, and Spark failed withUnsupportedFileSystemException: No FileSystem for scheme "s3".expire_snapshots/remove_orphan_filesmay resolve the wrong file system from the recorded scheme, depending on the engine and FileIO. Doris's ownremove_orphan_filesrefuses to run on such a table (Cannot determine whether listed and reachable file locations are equivalent).write_pathand committed underoriginal_write_path(VIcebergPartitionWriter), butVIcebergDeleteSinkhad no such split; it reported theoutput_path-based path it wrote.VIcebergDeleteFileWriterreported its write path by default, and FE accepted any path. CI did run on such a location: the Iceberg REST fixture's warehouse iss3a://, which FE normalizes tos3://. But the DML suites only checked that delete file paths contain/data/delete_pos_, and a straytest_iceberg_update_delete_basic.out.orgin the repository records the wrongs3://warehouse/wh/...paths.How this PR fixes it
[fix](be) Commit Iceberg delete files under the table's own location,[test](be) Verify Iceberg delete file commit pathsand[test](be) Initialize the Arrow memory pool in the Iceberg position delete write test:VIcebergDeleteSinkwrites underoutput_pathand commits position delete files and deletion vectors undertable_location. An FE unit test pins that both sinks carryingtable_locationname the same directory asoutput_path, andDataSinks.thriftdocuments the contract. The position delete write test sets up the Arrow memory pool the Parquet writer allocates from, so it also passes when run alone (CI review bot finding on this PR).[refactor](be) Take both paths of every Iceberg file from one output location:IcebergOutputLocation(header-only) holds one directory in both forms and gives the write path and the commit path of a file name, and of a partition subdirectory.VIcebergTableWriter/VIcebergPartitionWriter), the delete file writer and the Puffin path all take their paths from it.VIcebergDeleteFileWriternow reports the commit path itself instead of its write path.open()/init_properties()instead of committing a write path or a relative path. The FE of every branch (3.1, 4.0, 4.1, 4.2, master) sets both fields on every Iceberg sink./.[improvement](fe) Reject Iceberg commit data reported outside the sink's location:planWriteregisters on the transaction the raw location(s) each sink it builds carries, read back from the built sink.IcebergConnectorTransaction.addCommitData, where every report from every writer enters FE, rejects a file that is not under a registered location, or has no path. FE then answers the report as not accepted, and BE deletes the files under the paths it wrote them at and cancels the query, so a wrong path fails the statement and leaves no orphan files.[test](regression) Check that every Iceberg write records its files under the table location:test_iceberg_write_file_locationsruns INSERT, INSERT OVERWRITE PARTITION, branch INSERT, DELETE, UPDATE, MERGE andrewrite_data_fileson a v2 table, and INSERT, DELETE, UPDATE and MERGE on a v3 table. After each operation every file in$all_filesmust start with the table's LOCATION plus/data/. A new write operation belongs in this suite too.test_iceberg_update_delete_basicchecks the prefix of its delete files as well, and the stray.out.orgis removed.oss://,s3a://,abfss://, ...s3://...s3://...original_output_path/table_location(no supported FE sends one)/p=1/x.parquet, delete files ass3://...write.data.pathending with/.../data//p=1/x.parquet.../data/p=1/x.parquet, as Iceberg's own location provider places themDelete files that an older version already committed with
s3://keep that address until they are rewritten or expired.Upgrade BE before FE, as usual. An upgraded FE rejects the
s3://delete file paths that a BE without this fix reports, so until that BE is upgraded, DELETE / UPDATE / MERGE on a table whose location FE normalizes fails instead of committing the wrong path.Classes, and how they call each other
IcebergWritePlanProvider.planWrite(FE) builds the sink and registers the raw location(s) it carries with the transaction (commitLocationsOf).IcebergConnectorTransaction.addCommitData(FE) accepts a reported file only under a registered location;IcebergWriterHelperthen turns the fragments intoDataFile/DeleteFile.IcebergOutputLocation(BE, new) is one directory in its write form and its commit form.VIcebergTableWriterbuilds the data location inopen()and hands eachVIcebergPartitionWriterits partition directory (WriteInfo.location).VIcebergDeleteSinkwrites position delete files throughVIcebergDeleteFileWriterand deletion vectors into one Puffin file, both from its location.VIcebergMergeSinkbuilds one of each.Not changed: Thrift fields (comments only), the scan side, and how FE normalizes locations.
Release note
Fix Iceberg DELETE/UPDATE/MERGE recording delete files with an
s3://path instead of the table's own location (e.g.oss://,s3a://orabfss://), which could prevent Doris and other engines from reading the table. Iceberg data files written under awrite.data.paththat ends with/no longer get a double/in their path.Check List (For Author)
run-be-ut.sh --filter='*Iceberg*:VParquetWriterTest.*': 318 tests from 35 suites pass, including every new case. The tests that write files also pass each in a process of its own:VIcebergDeleteSinkTest.TestWritePositionDeletesCommitsTableLocation(the CI review bot finding) and the new cases inVIcebergTableWriterTest,VIcebergPartitionWriterTestandVIcebergMergeSinkTest.fe-connector-icebergpass (including the new cases inIcebergConnectorTransactionTestandIcebergWritePlanProviderTest); removing the registration inplanWriteor the check inaddCommitDatamakes the new cases fail.s3a://warehouse/wh/):test_iceberg_write_file_locationsandtest_iceberg_update_delete_basicpass. The v2 table ends with 15 data files and 3 position delete files, the v3 table with 6 data files and 3 deletion vector files, all unders3a://warehouse/wh/<db>/<table>/data/.test_iceberg_merge_into_basic,test_iceberg_merge_into_advanced,test_iceberg_update_delete_advanced,test_iceberg_rewrite_data_files,test_iceberg_v3_row_lineage_rewrite_data_files,test_iceberg_v3_row_lineage_update_delete_merge,test_iceberg_static_partition_overwrite,test_iceberg_write_branch_dml_boundary,test_iceberg_write_overwrite_delete_files,test_iceberg_write_dml_modes_evolution,test_iceberg_write_partition_path,test_iceberg_write_transform_partitions.test_iceberg_insert_overwrite,test_iceberg_write_insertandtest_iceberg_write_partitionsneed the Hive Metastore docker and were not run locally.abfss://), Hadoop catalog and explicit SharedKey: v2/v3 DELETE/UPDATE/MERGE, Spark reads and Dorisexpire_snapshotspassed. Other cloud providers and REST-vended credentials were not tested.write.data.pathending with/no longer get a double/.Check List (For Reviewer who merge this PR)