[fix](iceberg) Enforce external write correctness - #66112
Conversation
|
run buildall |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 2nd, 2026 1:27 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 30257 ms |
TPC-DS: Total hot run time: 177986 ms |
ClickBench: Total hot run time: 25.23 s |
|
/review |
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Keep generic external columns nullable until the Iceberg-specific read and write schema handling from PR apache#65851 is available.
|
run buildall |
|
/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. |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Aug 2nd, 2026 1:27 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 29196 ms |
TPC-DS: Total hot run time: 177718 ms |
ClickBench: Total hot run time: 25.04 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Request changes: this revision still has one data-file ownership gap and two coverage/diagnostic regressions. I found three new, non-duplicate issues and left the already-open review threads un-repeated.
Critical checkpoint conclusions:
- Goal and proof: The PR substantially improves Iceberg MERGE cardinality enforcement, failed-write cleanup, nullable truncate handling, and Paimon/CTAS behavior. It does not fully prove failed-write atomicity: a dynamic writer can lose cleanup ownership after creating its object, and the new late-rollover regression does not deterministically reach the lifecycle it claims to test. Explicit-engine CTAS also gains an unknown-catalog null dereference.
- Scope and focus: The patch is broad but its Iceberg, CTAS/Paimon, protocol, and test changes are traceable to the stated follow-up fixes. No additional user focus was supplied, so I reviewed the whole changed-file set.
- Concurrency and thread safety: No new thread or lock primitive is introduced. SQL MERGE rows with the same target row ID are routed to the same destination fragment instance and sink-local validator. The CTAS concurrent-ownership race is already covered by an existing thread; the new unknown-catalog issue is a distinct single-command validation path.
- Error handling: Status propagation across the revised merge close paths is generally explicit. The dynamic partition writer still has a post-create
open()error path with no cleanup owner, and eager CTAS sink construction turns a normal catalog lookup error into aNullPointerException. - Memory safety and ownership: The compact per-file Roaring cardinality state avoids the former per-row string retention, and no new unsafe buffer lifetime was found. Physical file ownership is incomplete only on the reported dynamic writer open path.
- Data correctness: Row-ID colocation makes duplicate detection exact across blocks for SQL MERGE, while plain UPDATE carries
require_cardinality_check=false. No new snapshot, row, or partition-transform correctness issue was found beyond the reported failed-object ownership gap and existing threads. - BE null/nullable handling: INSERT, MERGE, and UPDATE projections materialize const wrappers before string truncate runs; the direct nullable nested column and null map remain row-aligned and the original null map is restored. I found no reachable
ColumnConst(ColumnNullable(String))failure after tracing those boundaries. - Lifecycle and static initialization: Deferred data/delete cleanup now survives sibling close failures and successful rollover. Static/nonpartitioned writers, sort-roll replacement, position-delete, and Puffin paths retain owners; dynamic partition creation before owner-map insertion remains the reported exception. No cross-translation-unit static initialization dependency was added.
- Configuration: No new runtime-tunable configuration item was added. The BE execution-version maximum moves to 11 and is used as the compatibility gate; existing session settings in regressions do not require dynamic process reconfiguration.
- Compatibility: The new thrift boolean is optional, UPDATE explicitly sends false, old FE requests omit it, and version 10 uniformly disables enforcement on new BEs. The operational mixed-version configuration concern is already raised in an existing thread and was not duplicated.
- Parallel paths and conditions: MERGE versus UPDATE, strict-consistency modes, partitioned versus unpartitioned/static writers, position-delete versus Puffin, CTAS with and without
IF NOT EXISTS, and stable/concurrent Paimon targets were traced. The two missed parallel/conditional cases are the reported dynamic open failure and explicit-engine unknown catalog. - Tests and expected results: The patch adds BE/FE unit coverage and activates Iceberg/Paimon regressions; the inspected expected-result changes are consistent with the SQL assertions. Coverage is still missing for post-create/pre-registration failure and explicit-engine unknown catalog, and the late-rollover object-count case can pass without reaching a closed-file state.
- Test execution: Per the review-only instruction, I did not build Doris or run tests. Formatting/checkstyle checks were green when inspected; compile and FE/BE/Cloud unit-test checks were still pending, so this review does not claim runtime validation.
- Observability: Existing status messages, warning logs for deletion failures, runtime profiles, and cardinality diagnostics cover the new normal paths. The reported unowned object is not visible to cleanup logging because its owner is lost before registration.
- Transactions and persistence: No EditLog schema or replay path is changed. External catalog create/no-op semantics and failed Iceberg publication were traced; the pre-existing rollback-by-name race remains covered by an existing thread.
- Data writes and atomicity: Outer MERGE close now retains data and delete handles until both sides succeed, but post-create dynamic writer failure can still orphan an unpublished HDFS object. FE or BE failure semantics otherwise revealed no additional new issue beyond existing threads.
- FE/BE variable propagation:
require_cardinality_check, merge partition information, and execution version are carried through logical/physical sinks, translation, thrift, runtime state, and BE construction. Missing/false/true cases were checked. - Performance: Operation-aware distribution avoids the prior unmatched-insert hotspot, and the cardinality structure is compact. No additional CPU, lock, or asymptotic regression was substantiated; performance concerns already raised in existing threads were not duplicated.
- Other issues: The unknown-catalog CTAS diagnostic regression is reported inline. No other distinct issue survived final deduplication.
| // to update a target row. Tiny blocks and files force a valid row to roll before | ||
| // the late duplicate, and the entire statement must still leave no data object behind. | ||
| sql """set batch_size = 1""" | ||
| // A single pipeline instance preserves the ordered source sequence needed to prove late cleanup. |
There was a problem hiding this comment.
[P2] Make the late-rollover sequence deterministic
parallel_pipeline_task_num=1 does not make this a single global sink. The new MERGE_PARTITIONED path sends the unmatched INSERT by its Iceberg partition while both matched UPDATE rows use the row-id channel, so the duplicate can fail in one VIcebergMergeSink before the insert channel has closed any data file. In that execution the object count still stays unchanged and this passes without exercising _closed_files cleanup. This is a distinct follow-up to the existing rollover thread: add a barrier/failpoint that confirms a data file has closed and been recorded before releasing the duplicate row, then keep the object-store assertion.
| io::FileDescription file_description = { | ||
| .path = fmt::format("{}/{}", _write_info.write_path, _get_target_file_name()), | ||
| .fs_name {}}; | ||
| _path = fmt::format("{}/{}", _write_info.write_path, _get_target_file_name()); |
There was a problem hiding this comment.
[P1] Register the data file before open() can fail
_path becomes a physical file before compression validation and transformer initialization, but the dynamic-partition caller adds this writer to _partitions_to_writers only after open() succeeds. For example, an Iceberg Parquet table with write.parquet.compression-codec=gzip reaches create_file() with TFileCompressType::GZ and then returns Unsupported compress type; the local writer is destroyed before either the active-writer cleanup or this new closed-file callback owns the path. HdfsFileWriter only closes that empty file in its destructor, so a failed MERGE/UPDATE leaves it orphaned. Transfer cleanup ownership immediately after creation, or register/close the dynamic writer on open failure, and cover this post-create failure path with an object-store assertion.
| } | ||
| // Reject unsupported destinations before publishing metadata; rollback by table name | ||
| // cannot distinguish this CTAS table from a concurrent replacement with the same name. | ||
| sinkQuery = UnboundTableSinkCreator.createUnboundTableSink(createTableInfo.getTableNameParts(), |
There was a problem hiding this comment.
[P2] Validate the catalog before eager sink construction
With an explicit recognized engine, a missing catalog reaches this line: paddingEngineName() only checks catalog existence when the engine is omitted, checkEngineWithCatalog() falls through on a null catalog, and targetTableExists() returns false. createUnboundTableSink() then evaluates curCatalog.getClass() and throws a NullPointerException, whereas the former Env.createTable() ordering returned Unknown catalog. This is distinct from the existing Paimon-target diagnostic thread because neither the catalog nor the table exists here. Resolve the catalog before sink prevalidation (or make the factory report the standard error) and cover explicit-engine CTAS against an unknown catalog.
TPC-H: Total hot run time: 29757 ms |
TPC-DS: Total hot run time: 176889 ms |
ClickBench: Total hot run time: 25.6 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
…t onto the connector SPI Upstream #66112 (8305fe7) makes an iceberg `MERGE INTO` reject a target row that more than one source row matches, independently of `enable_strict_consistency_dml`. It did that in fe-core classes this branch no longer has (`LogicalIcebergMergeSink`, `PhysicalIcebergMergeSink`, `planner.IcebergMergeSink`, `IcebergMergeCommand`, `IcebergUpdateCommand`), so the rebase brought in the BE half and the thrift field (`TIcebergMergeSink.require_merge_cardinality_check`) with no FE producer: BE resolved the unset field to false and its duplicate-match validation was dead code. `UPDATE` and `MERGE INTO` synthesize the same sink here, so the statement kind is carried the way upstream carries it - one boolean stamped at the plan builder: ExternalRowLevel{Merge,Update}PlanBuilder (true / false) -> Logical/PhysicalExternalRowLevelMergeSink -> RequestPropertyDeriver (MERGE keeps the merge distribution even with enable_strict_consistency_dml off - BE can only see the duplicates when both matches reach one instance) -> PhysicalPlanTranslator -> PluginDrivenTableSink -> ConnectorWriteHandle#isRequireMergeCardinalityCheck (new default-false SPI method) -> IcebergWritePlanProvider stamps TIcebergMergeSink. The flag is kept off the existing `WriteOperation` axis on purpose: it is a statement-level SQL requirement, not a sink dialect, and the engine also reads it for the distribution decision. Every other connector keeps the default false, so no non-iceberg write changes. Also updates PhysicalExternalRowLevelMergeSinkTest for the behaviour #66112 changed in a hunk that merged cleanly: with `enable_iceberg_merge_partitioning` off the sink now asks for a DistributionSpecMerge that routes only the DELETE images by row id, instead of hashing every row by a row id that is NULL on the insert side (which serialized all inserts onto one exchange channel). The test still encoded the old DistributionSpecHash and was failing. Tests: unit coverage for each hop - origin (both polarities), deriver, translator threading, and the thrift stamp; 4-way mutation check confirms each hop's test goes red when its link is broken. FE 173 tests green in the touched classes; checkstyle 0 violations. E2E: regression-test/.../iceberg/write/test_iceberg_write_merge_duplicate_source_negative (un-gated by #66112) is the end-to-end gate and needs a docker run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…resh onto the plugin bridge Upstream #66112 (8305fe7) fixed three CREATE TABLE defects in fe-core's legacy paimon subsystem, which this branch has migrated into the generic plugin bridge. The upstream code merged cleanly but is inert here, so each is re-implemented where it now belongs. All three are connector-agnostic on this branch, not paimon-only. 1. Reject an unsupported CTAS destination BEFORE publishing metadata. Upstream moved the sink construction ahead of `Env.createTable`; that hunk is on this branch, but `UnboundTableSinkCreator` admits ANY PluginDrivenExternalCatalog, so a CTAS into a connector with no write path (paimon / es / hudi / trino) still created the remote table and only failed later in PhysicalPlanTranslator. The fallback then dropped the table BY NAME - which cannot tell this statement's table from a concurrent creator's table of the same name, i.e. it can drop someone else's. The CTAS overload of createUnboundTableSink now asks the connector-level write provider first. A gateway connector answers it (hive/iceberg/jdbc/maxcompute return a provider), so only the genuinely write-less connectors are refused, with the same wording the translator used. 2. Refresh the FE table-name cache on an IF NOT EXISTS no-op. `PluginDrivenExternalCatalog#createTable` returned true BEFORE the post-create cache invalidation, so a table created out-of-band (another engine / another FE) stayed invisible to SHOW TABLES and to the following SELECT. A neighbouring probe happened to self-heal it, which made the guarantee accidental; it is now explicit. 3. Report a concurrent creator instead of swallowing it. The existence probe and the remote create are not atomic. The paimon connector forwarded `isIfNotExists()` as paimon's `ignoreIfExists`, so a creator that won the race in between was silently no-op'd and this statement looked like the creator - a CTAS would then INSERT into, and on failure roll back, the winner's table. Paimon now always creates with `ignoreIfExists=false` (as upstream's fe-core arm now does) and the bridge turns the resulting failure back into an IF NOT EXISTS no-op returning true, after re-probing that the table really is there. Without IF NOT EXISTS the failure still surfaces. Tests: bridge tests for both no-op arms and both race polarities, a new UnboundTableSinkCreatorTest for the CTAS admission gate, and the paimon DDL tests updated to the new contract (they encoded the pre-#66112 passthrough). Mutation check: removing the cache refresh or the admission gate turns the respective tests red. 76 tests green in the touched classes; checkstyle 0 violations. E2E: regression-test/.../paimon/test_paimon_ctas_atomicity_negative (un-gated by #66112) covers all three and needs a docker run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t onto the connector SPI Upstream #66112 (8305fe7) makes an iceberg `MERGE INTO` reject a target row that more than one source row matches, independently of `enable_strict_consistency_dml`. It did that in fe-core classes this branch no longer has (`LogicalIcebergMergeSink`, `PhysicalIcebergMergeSink`, `planner.IcebergMergeSink`, `IcebergMergeCommand`, `IcebergUpdateCommand`), so the rebase brought in the BE half and the thrift field (`TIcebergMergeSink.require_merge_cardinality_check`) with no FE producer: BE resolved the unset field to false and its duplicate-match validation was dead code. `UPDATE` and `MERGE INTO` synthesize the same sink here, so the statement kind is carried the way upstream carries it - one boolean stamped at the plan builder: ExternalRowLevel{Merge,Update}PlanBuilder (true / false) -> Logical/PhysicalExternalRowLevelMergeSink -> RequestPropertyDeriver (MERGE keeps the merge distribution even with enable_strict_consistency_dml off - BE can only see the duplicates when both matches reach one instance) -> PhysicalPlanTranslator -> PluginDrivenTableSink -> ConnectorWriteHandle#isRequireMergeCardinalityCheck (new default-false SPI method) -> IcebergWritePlanProvider stamps TIcebergMergeSink. The flag is kept off the existing `WriteOperation` axis on purpose: it is a statement-level SQL requirement, not a sink dialect, and the engine also reads it for the distribution decision. Every other connector keeps the default false, so no non-iceberg write changes. Also updates PhysicalExternalRowLevelMergeSinkTest for the behaviour #66112 changed in a hunk that merged cleanly: with `enable_iceberg_merge_partitioning` off the sink now asks for a DistributionSpecMerge that routes only the DELETE images by row id, instead of hashing every row by a row id that is NULL on the insert side (which serialized all inserts onto one exchange channel). The test still encoded the old DistributionSpecHash and was failing. Tests: unit coverage for each hop - origin (both polarities), deriver, translator threading, and the thrift stamp; 4-way mutation check confirms each hop's test goes red when its link is broken. FE 173 tests green in the touched classes; checkstyle 0 violations. E2E: regression-test/.../iceberg/write/test_iceberg_write_merge_duplicate_source_negative (un-gated by #66112) is the end-to-end gate and needs a docker run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…resh onto the plugin bridge Upstream #66112 (8305fe7) fixed three CREATE TABLE defects in fe-core's legacy paimon subsystem, which this branch has migrated into the generic plugin bridge. The upstream code merged cleanly but is inert here, so each is re-implemented where it now belongs. All three are connector-agnostic on this branch, not paimon-only. 1. Reject an unsupported CTAS destination BEFORE publishing metadata. Upstream moved the sink construction ahead of `Env.createTable`; that hunk is on this branch, but `UnboundTableSinkCreator` admits ANY PluginDrivenExternalCatalog, so a CTAS into a connector with no write path (paimon / es / hudi / trino) still created the remote table and only failed later in PhysicalPlanTranslator. The fallback then dropped the table BY NAME - which cannot tell this statement's table from a concurrent creator's table of the same name, i.e. it can drop someone else's. The CTAS overload of createUnboundTableSink now asks the connector-level write provider first. A gateway connector answers it (hive/iceberg/jdbc/maxcompute return a provider), so only the genuinely write-less connectors are refused, with the same wording the translator used. 2. Refresh the FE table-name cache on an IF NOT EXISTS no-op. `PluginDrivenExternalCatalog#createTable` returned true BEFORE the post-create cache invalidation, so a table created out-of-band (another engine / another FE) stayed invisible to SHOW TABLES and to the following SELECT. A neighbouring probe happened to self-heal it, which made the guarantee accidental; it is now explicit. 3. Report a concurrent creator instead of swallowing it. The existence probe and the remote create are not atomic. The paimon connector forwarded `isIfNotExists()` as paimon's `ignoreIfExists`, so a creator that won the race in between was silently no-op'd and this statement looked like the creator - a CTAS would then INSERT into, and on failure roll back, the winner's table. Paimon now always creates with `ignoreIfExists=false` (as upstream's fe-core arm now does) and the bridge turns the resulting failure back into an IF NOT EXISTS no-op returning true, after re-probing that the table really is there. Without IF NOT EXISTS the failure still surfaces. Tests: bridge tests for both no-op arms and both race polarities, a new UnboundTableSinkCreatorTest for the CTAS admission gate, and the paimon DDL tests updated to the new contract (they encoded the pre-#66112 passthrough). Mutation check: removing the cache refresh or the admission gate turns the respective tests red. 76 tests green in the touched classes; checkstyle 0 violations. E2E: regression-test/.../paimon/test_paimon_ctas_atomicity_negative (un-gated by #66112) covers all three and needs a docker run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Validate unsupported CTAS sinks before publishing table metadata, so setup failures cannot leave an unusable table or remove a concurrent replacement. - Preserve nullable string wrappers in truncate partition transforms. - Enforce Iceberg MERGE cardinality routing independently of `enable_strict_consistency_dml`. - Detect duplicate target matches with file-path-interned Roaring bitmaps and expose retained validation-state bytes in the sink profile. - Activate the negative regressions for the three retained Jira fixes and align the duplicate-match oracle with the emitted diagnostic. - Full build: `./build.sh --fe --be -j 48` - FE: `CreateTableCommandTest`, `IcebergDDLAndDMLPlanTest` (19 tests) - BE: `VIcebergMergeSinkTest` (8 tests, including 100,000 matched rows) - Regression: `test_paimon_ctas_atomicity_negative` (1 suite, passed) - Iceberg end-to-end SQL: duplicate-source MERGE rejected atomically; nullable/merge truncate writes and logical rows verified; physical partition values verified from the same Iceberg tables through Spark metadata. The Doris-side `$partitions`/`$snapshots` checks in the Iceberg suites currently hit an unrelated JNI scanner initialization abort on the current master test binary; the equivalent table state and metadata assertions above passed. (cherry picked from commit 8305fe7)
- Validate unsupported CTAS sinks before publishing table metadata, so setup failures cannot leave an unusable table or remove a concurrent replacement. - Preserve nullable string wrappers in truncate partition transforms. - Enforce Iceberg MERGE cardinality routing independently of `enable_strict_consistency_dml`. - Detect duplicate target matches with file-path-interned Roaring bitmaps and expose retained validation-state bytes in the sink profile. - Activate the negative regressions for the three retained Jira fixes and align the duplicate-match oracle with the emitted diagnostic. - Full build: `./build.sh --fe --be -j 48` - FE: `CreateTableCommandTest`, `IcebergDDLAndDMLPlanTest` (19 tests) - BE: `VIcebergMergeSinkTest` (8 tests, including 100,000 matched rows) - Regression: `test_paimon_ctas_atomicity_negative` (1 suite, passed) - Iceberg end-to-end SQL: duplicate-source MERGE rejected atomically; nullable/merge truncate writes and logical rows verified; physical partition values verified from the same Iceberg tables through Spark metadata. The Doris-side `$partitions`/`$snapshots` checks in the Iceberg suites currently hit an unrelated JNI scanner initialization abort on the current master test binary; the equivalent table state and metadata assertions above passed. (cherry picked from commit 8305fe7)
- Validate unsupported CTAS sinks before publishing table metadata, so setup failures cannot leave an unusable table or remove a concurrent replacement. - Preserve nullable string wrappers in truncate partition transforms. - Enforce Iceberg MERGE cardinality routing independently of `enable_strict_consistency_dml`. - Detect duplicate target matches with file-path-interned Roaring bitmaps and expose retained validation-state bytes in the sink profile. - Activate the negative regressions for the three retained Jira fixes and align the duplicate-match oracle with the emitted diagnostic. - Full build: `./build.sh --fe --be -j 48` - FE: `CreateTableCommandTest`, `IcebergDDLAndDMLPlanTest` (19 tests) - BE: `VIcebergMergeSinkTest` (8 tests, including 100,000 matched rows) - Regression: `test_paimon_ctas_atomicity_negative` (1 suite, passed) - Iceberg end-to-end SQL: duplicate-source MERGE rejected atomically; nullable/merge truncate writes and logical rows verified; physical partition values verified from the same Iceberg tables through Spark metadata. The Doris-side `$partitions`/`$snapshots` checks in the Iceberg suites currently hit an unrelated JNI scanner initialization abort on the current master test binary; the equivalent table state and metadata assertions above passed. (cherry picked from commit 8305fe7)
…sistency fixes (#66246) ### What problem does this PR solve? This backports the following lakehouse fixes to `branch-4.1`: - #66112: enforce external write correctness for Iceberg operations. - #66223: preserve pruned Paimon struct field reads. - #66007: preserve metadata generations and nested schema semantics across Iceberg, Paimon, Hudi, ORC, and complex-type materialization. The #66007 changes were adapted to the `branch-4.1` APIs while preserving the source PR's behavior. A minimal nullable-cast helper required by the backport was included instead of pulling the unrelated master refactor that originally introduced it. ### Check List - FE focused unit tests: 202 successful test executions across the initial suite and the Paimon follow-up run. - FE Maven reactor: `BUILD SUCCESS`. - BE affected production and test translation units: compiled successfully with ASAN settings. - BE #66007 ORC and complex-rematerialization tests: 18 tests passed on the exact #66007 head. - `git diff --check`: passed.
Proposed changes
enable_strict_consistency_dml.Test
./build.sh --fe --be -j 48CreateTableCommandTest,IcebergDDLAndDMLPlanTest(19 tests)VIcebergMergeSinkTest(8 tests, including 100,000 matched rows)test_paimon_ctas_atomicity_negative(1 suite, passed)The Doris-side
$partitions/$snapshotschecks in the Iceberg suites currently hit an unrelated JNI scanner initialization abort on the current master test binary; the equivalent table state and metadata assertions above passed.