Repository navigation
[fix](fe) Bind Iceberg branch writes to the table's current schema - #69499
morningman merged 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all nine changed files at a218feb8b0981efa6a17163b11fabe8d2b06fdd8. No substantiated new findings and no existing inline findings to carry forward. No additional user focus was provided.
Branch writes now bind column names, types, defaults, partitioning and sort metadata to the table-current schema while retaining the selected branch as the parent snapshot. This matches Iceberg's documented schema selection and the repository-pinned Iceberg 1.12.0 snapshot implementation.
Critical checkpoint conclusions:
- Goal and evidence: The central schema resolver and commit check implement current-schema branch writes. Changed unit and regression cases cover renames, current defaults, required fields, partition-source evolution, invalid refs and subsequent schema changes.
- Scope and reuse: The production change is focused, reuses the existing statement-scoped writer context and removes obsolete branch-schema compatibility traversal.
- Concurrency and locks: No new threads or locks. Concurrent external metadata changes are checked before/after transaction initialization, before commit and at the final metadata CAS, including SDK retries; existing lock order is unchanged.
- Lifecycle: The immutable context remains statement-owned. Transaction leases, cleanup and static initialization are unchanged; no new ownership cycle was found.
- Configuration: No configuration items are added or changed.
- Compatibility: Branch column binding intentionally changes to current names/defaults. No public SPI, file-format or wire-layout change is introduced; existing serializers and field-ID handling remain in use.
- Thrift/protobuf: No definitions are changed; cross-release field-ID checks are inapplicable.
- Parallel paths: INSERT and static/dynamic/whole-table/empty OVERWRITE retain branch routing. Main row-level writes and REWRITE retain their established paths. Parquet/ORC and historical tag/snapshot cases were inspected.
- Conditional checks and errors: Missing/tag refs still fail explicitly. Schema/format/table identity and retained spec/order checks remain; omitted required fields without defaults and explicit invalid nulls retain engine/BE enforcement.
- Test coverage: Changed JUnit 5 cases and all four regression suites were inspected, including negative names/refs and main/branch/tag isolation. Historical-source INSERT is EXPLAIN-only; required-without-default and later-schema cases primarily verify metadata/context validation rather than full SQL or retry execution.
- Expected results: All changed golden rows match their SQL, including default 36 and evolved nested payload/note positions. Exact overwrite output checks replacement and retained rows. Branch payload.extra/metric lack direct value assertions; golden-file generation provenance was not independently verified.
- Observability: Existing errors identify table/schema/ref or field context and provide retry guidance. This change adds no operational subsystem requiring new metrics or logging.
- Persistence and failover: No Doris EditLog or persisted FE metadata is changed. Iceberg's existing transaction commit/retry protocol remains authoritative; crash/failover behavior was not exercised.
- Data writes and atomicity: Target binding, physical sink schema and commit-file metadata consume the same pinned context. Existing atomic metadata commit, overwrite conflict checks and rollback cleanup remain in place; no introduced write-loss or isolation defect was found statically.
- FE/BE variables: No new variables or wire fields. Existing schema JSON carries the current shape, bound ordinals/IDs are checked, and downstream recursive required-value validation remains active.
- Performance: The change removes repeated branch compatibility indexes and adds no new remote operation or hot loop. No introduced performance issue was identified.
- Other concerns: Separate read/write metadata namespaces prevent historical source schemas from replacing the writer schema. All investigated risks have evidence-based dispositions; no unresolved correctness candidate remains.
This was a static-only review: no builds or tests were run and no repository source files were modified. Runtime test results reported by the author were not independently reproduced. The test-depth limits above do not establish a verified defect.
Review converged after 2 round(s).
FE UT Coverage ReportIncrement line coverage `` 🎉 |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28362 ms |
TPC-DS: Total hot run time: 150596 ms |
ClickBench: Total hot run time: 24.39 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#66538, apache#65851 Problem Summary: A write to an Iceberg branch (INSERT [OVERWRITE] INTO t@branch(b)) pins the schema of the branch head snapshot as its writer schema, while a read of the branch uses the table-current schema. Iceberg validates a branch commit against the table-current schema and stamps that schema on the new snapshot, Spark binds branch writes to the current schema, and the Iceberg docs ("Schema selection with branches and tags") say writes to a branch use the table schema. So once the table schema evolves, a write to an older branch rejects the current column names and accepts the old ones, writes the branch head's write-default instead of the current one, and fails outright when the current partition spec has a source column the branch head schema does not have. Master also needs an extra check (validateBranchWriterSchema) because the files it writes carry a different schema than the one Iceberg records for the new snapshot. branch-4.1 and branch-4.2 (apache#66538) bind branch writes to table.schema(). This change pins table.schema() for branch writes, keeps the check that the target is an existing branch and not a tag, and fences the commit on the table-current schema id for branch writes as for writes to main. The branch head checks are removed. ### Release note Fix: Writes to an Iceberg branch now use the table's current schema (column names, write-defaults and partition spec), as Spark does, instead of the schema of the branch's head snapshot. ### Check List (For Author) - Test: Regression test / Unit Test - test_iceberg_initial_defaults (a branch write gets the current write-default 36), write/test_iceberg_write_evolution_refs, branch_tag/iceberg_branch_tag_schema_change_extended and test_iceberg_schema_ref_actions_matrix (an older branch takes the current column names and rejects the old ones) now expect what branch-4.2 expects. They pass locally; without this change each of them fails at the new expectation - IcebergWritePlanProviderTest: a branch created before a rename writes the new name; the branch write uses the current write-defaults, binds a current required column without a default, and binds the current partition spec whose source column the branch head lacks; a schema change after pinning fails the commit; a tag or a missing ref is rejected - Behavior changed: Yes. See the release note. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
a218feb to
2937f34
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes for one P1 correctness issue: a branch INSERT OVERWRITE can discard the writer generation used to expand DEFAULT, then silently bind those values to reordered columns during its inner INSERT. The inline finding gives the concrete interleaving and repair direction. No existing blocking inline comments apply.
Reviewed all nine changed paths and the upstream binding/default, sink, transaction refresh/retry, branch targeting, and regression-result paths at 2937f34. The current-schema rule itself matches Iceberg's schema-selection contract; the remaining issue is preserving that schema through the complete statement.
Critical checkpoint conclusions:
- Goal and evidence: current names, required-column metadata, write-defaults, partition-source evolution, and late schema rejection are covered by inspected unit/regression cases. C1 prevents full statement-generation correctness.
- Scope and clarity: the production change is focused; the obsolete branch/current compatibility walker is appropriately removed when the writer uses the current schema.
- Concurrency and locks: external schema updates can race with the two overwrite planning passes (C1). Begin, refresh, precommit, and final metadata-CAS checks protect the retained inner pin. No new locks or lock-order changes.
- Lifecycle and initialization: statement scope, tracked table leases, transaction ownership, and cleanup mechanisms are unchanged. No new static initialization dependencies.
- Configuration: no settings added or changed.
- Compatibility: the branch column/default binding change is intentional and described in the release note. No public connector API, symbol, or storage-format change.
- Thrift/protobuf: untouched; no field-ID or enum change.
- Parallel paths: checked normal INSERT, static/dynamic/empty/whole-table OVERWRITE, main versus branch, tag/numeric historical sources, and existing row-level/REWRITE boundaries. Branch targeting and overwrite ancestry/conflict checks remain intact.
- Conditional checks and errors: missing refs and tag write targets still fail explicitly. Schema/format/spec/order/identity drift is rejected after the inner pin; the lost outer pin in C1 bypasses the intended error.
- Test coverage: added/updated cases cover current names/defaults, required-field column metadata, missing/tag refs, and partition evolution. They omit the inter-pass concurrent reorder in C1; the required-column unit case does not itself execute a branch omission error.
- Expected results: all changed rows match their SQL projections and setup, including DEFAULT 36, nested payload labels, and branch/main/tag isolation. The full overwrite result still detects surviving duplicate seed rows after removal of the count assertion.
- Observability: existing table/schema/spec identifiers and retry errors remain useful; no separate new metric/logging requirement found. C1 can currently commit without an error.
- Transactions and persistence: Iceberg metadata CAS/replay validation remains installed; no Doris EditLog or replay format changes. No new failover mechanism or persistence gap found beyond the discarded planning witness.
- Writes, atomicity, and crashes: the metadata commit remains atomic and targets the requested branch. C1 can atomically publish wrong column values. No new crash-cleanup owner or leak path identified; crash behavior was not executed.
- FE/BE data passing: existing sink schema/spec/order carriers are reused; no new transmitted variable or send path.
- Performance and memory: the per-field branch compatibility walk is removed; no new expensive loop or allocation pattern. BE MemTracker, internal partition visibility, and MoW delete-bitmap mechanics are unchanged.
- Other concerns and user focus: no additional user focus was supplied; no further distinct actionable issue found after complete-scope and missed-problem rechecks.
Validation limits: static review only, as instructed. No builds, tests, cluster runs, or runtime reproduction were performed; expected-output generation provenance was not independently established. These limits do not leave an unresolved code candidate. Review convergence and delivery are reported separately by this invocation.
Review converged after 2 round(s).
| } | ||
| // A branch selects only the parent snapshot / ref. Iceberg validates a branch write against the | ||
| // table-current schema and stamps that schema on the new snapshot, and Spark binds it the same way. | ||
| Schema schema = table.schema(); |
There was a problem hiding this comment.
[P1] Preserve the writer generation across overwrite replanning
For a v3 table ordered [a, b] with write-defaults 1 and 2, run INSERT OVERWRITE t@branch(br) VALUES (DEFAULT, DEFAULT). InsertOverwriteTableCommand.run first pins that schema and normalizePlan produces InlineTable(1, 2). If another Iceberg client commits updateSchema().moveFirst("b").commit() before the inner INSERT starts, InsertIntoTableCommand.initPlan calls resetConnectorStatementScope() and discards both the original schema and metadata identity. It then pins [b, a] while needNormalizePlan=false retains those already-expanded literals. BindSink.getColumnToChildOutput maps by ordinal, producing Sink[b <- 1, a <- 2]: the defaults are silently written into the wrong columns, and all schema checks pass against the second pin. Before this change, a table-only reorder left both branch-head schema pins unchanged. Preserve the outer writer context/identity across this handoff, or restart normalization from the original DEFAULT expressions after repinning; cover the inter-pass reorder with a controlled concurrency test.
There was a problem hiding this comment.
Fixed in 4999011. The interleaving is real, and it is not specific to branch writes: main-branch INSERT OVERWRITE on master has had the same exposure since #65851 (the outer pass pins the writer schema and then expands DEFAULT and bare rows by position against it) and #66529 (every attempt of the inner insert resets the statement scope and pins again). Before this PR a branch overwrite was exposed only when a concurrent commit moved the branch head onto a snapshot with another schema; binding branch writes to the table's current schema widened that to the same table-level changes as main.
The overwrite now records on the inner insert's PluginDrivenInsertCommandContext the writer columns and write metadata identity it normalized the plan against (InsertUtils.recordNormalizedWriteGeneration). After the inner insert pins again, InsertUtils.checkNormalizedWriteGeneration compares them (columns by position: name, field ID, type, nullability and default) and fails the statement with "retry the statement" when either differs, instead of binding the expanded values by position to the new schema. This covers main and branch overwrites alike, and an insert that normalizes its own plan is unaffected. branch-4.2 avoids the hole differently: it carries the pinned IcebergWriteSchemaContext on the unbound sink into the inner insert, so both passes share one generation, and its commit fence rejects a later change with "retry the statement". InsertUtilsTest replays the two pins with a reordered schema and with a changed identity (both fail without the check) and with an unchanged generation.
FE UT Coverage ReportIncrement line coverage `` 🎉 |
TPC-H: Total hot run time: 28692 ms |
FE Regression Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 150375 ms |
ClickBench: Total hot run time: 24.29 s |
…n its planning passes ### What problem does this PR solve? Issue Number: None Related PR: apache#66538, apache#65851 Problem Summary: Review follow-up. INSERT OVERWRITE into a connector table plans twice. InsertOverwriteTableCommand.run pins the writer schema and normalizes the plan against it: bare DEFAULT and empty rows are expanded by position and values are cast to the pinned column types. The inner InsertIntoTableCommand that runs the plan is built with needNormalizePlan=false, and every attempt of its initPlan resets the statement's connector scope and pins the writer schema again. Without a column list BindSink then maps the normalized values by position onto the second pin, and every later fence (the bound write metadata identity, validateBoundWriteColumns, the begin and commit schema checks) compares against that second pin. So when another client reorders the columns between the passes, e.g. updateSchema().moveFirst("b") on [a, b] during INSERT OVERWRITE t VALUES (DEFAULT, DEFAULT) the defaults are silently written as b <- 1, a <- 2. Main-branch overwrites have had this exposure since apache#65851 (pin, then expand defaults) and apache#66529 (repin on every attempt); the previous commit gives branch overwrites the same one, because a branch write now pins the table's current schema. branch-4.2 carries the pinned writer context from the outer pass into the inner insert, so both passes share one generation. The overwrite now records on the inner insert's context the writer columns and write metadata identity it normalized against, and the inner insert, after pinning again, fails the statement with "retry the statement" when either differs. Nothing changes when the table did not change between the passes or for any other insert. ### Release note None ### Check List (For Author) - Test: Unit Test - InsertUtilsTest: a reordered writer schema and a changed write metadata identity between the passes fail; the same generation, and an insert that normalizes its own plan, pass - InsertOverwriteTableCommandTest, InsertOverwriteDroppedTargetTest, InsertOverwriteSkipAuthResetTest, IcebergStaticPartitionLiteralTest, InsertIntoTableCommandTest, InsertIntoTableCommandTableStreamTest, CloudInsertProfileRetryTest, DictionaryInsertTargetDropRaceTest, PluginDrivenTableSinkBindingTest, PluginDrivenTableSinkTest, ReadLockTest, RowLevelDmlCommandTest - Behavior changed: Yes. An INSERT OVERWRITE into a connector table whose writer schema or write metadata changes while the statement plans now fails with "retry the statement" instead of binding its values by position to the new schema. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all 14 changed files at 49990114c9502537c6c096da0dd1eeeb8c833b6e: no new actionable findings.
The existing P1 overwrite-generation finding is fixed at this head: the outer overwrite records its normalization schema and metadata identity, and every inner insert attempt compares the repinned generation before binding normalized values. It is excluded from existing blocking IDs and is not reposted.
Critical checkpoint conclusions:
- Goal and evidence: branch writes now bind current table columns, defaults and partition sources while preserving the branch destination. Connector unit tests and four regression suites cover the intended behavior; tests were inspected, not executed.
Core invariants: the modified paths use the existing Iceberg atomic publication boundary and reject stale writer generations. Doris partition visible-version logic, MoW delete bitmaps and BE memory accounting are untouched. - Scope and reuse: changes are focused on writer-schema selection and a generic overwrite-generation handoff; shared helpers avoid connector-specific branching in FE planning.
- Concurrency and locking: concurrent external metadata commits are checked at repinning, transaction opening and final Iceberg metadata CAS. The new comparison runs before positional binding; no new threads, shared mutable state or lock-order changes were introduced.
- Lifecycle: normalization metadata stays on the inner command context across statement-scope reset and retries; ordinary inserts carry no normalization pin. No new external resources, ownership cycles or static initialization dependencies were found.
- Configuration: no settings were added or changed.
- Compatibility: branch target names/defaults intentionally follow the current schema, and metadata changes between overwrite planning passes now produce a retry error. No persisted format or plugin SPI signature changes.
- Thrift/protobuf: no definition changes.
- Parallel paths: reviewed main/branch INSERT and OVERWRITE, static/dynamic partitions, historical source relations, row-level write schema use, and connectors without pinned write columns. No additional affected path requiring this fix was identified.
- Conditional checks: missing/tag refs remain rejected; absent normalization metadata bypasses only the new overwrite-specific check; changed schemas or metadata fail explicitly.
- Test coverage: tests cover renamed/current columns, write defaults, required-column metadata, partition source evolution, invalid refs, precommit schema change, reordered generations, changed identities and unchanged-generation success. The two-pin tests replay the interleaving at helper level rather than executing a concurrent full command; required-without-default coverage asserts metadata rather than the full analysis error.
- Expected results: all changed expected rows match their SQL inputs, including current default 36, branch payload/note values, overwrite replacement and main/tag isolation. Generation of the checked-in outputs was not independently verified.
- Observability: new failures identify the target table and instruct the caller to retry; no new metric or logging requirement was identified for this planning check.
- Transactions and persistence: Iceberg retains its existing atomic metadata commit and final schema/identity validation. No Doris journal, replay, failover or transaction-state persistence logic changed.
- Data writes and failures: generation mismatches abort before execution, and later schema changes are fenced before commit. No new partial-commit or crash-cleanup path was introduced; crash behavior was assessed statically.
- FE/BE propagation: no new wire variables or send paths.
- Performance: the new comparison is linear in the number of columns per overwrite planning attempt; no per-row work or additional remote request is introduced by the comparison.
- Other concerns and user focus: no additional user focus was supplied. All initial risk items were resolved by code-path verification; no unresolved review candidate or unreviewed changed file remains.
Validation limits: static analysis only, as required by the review contract. No builds, tests or runtime/concurrency experiments were run; reported author test executions were not independently reproduced.
Review converged after 2 round(s).
TPC-H: Total hot run time: 28926 ms |
TPC-DS: Total hot run time: 150114 ms |
ClickBench: Total hot run time: 24.24 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
…load ### What problem does this PR solve? Issue Number: N/A Related PR: apache#65851, apache#69488, apache#69499 Problem Summary: Required and nested ADD COLUMN defaults must retain their SQL values when converted to Iceberg metadata. Normalize decimal scale, valid Boolean whitespace, compact UUIDs and escaped strings; preserve nanosecond gap semantics and temporal precision while keeping unqualified zoned values compatible with existing connector inputs. Resolve Iceberg zoned defaults through the session timezone and reject non-finite floating defaults before metadata publication. Verify real Hadoop metadata commits/reloads, extend read/write regression coverage and update both existing v2 rejection assertions. ### Release note Typed literal defaults for required and nested Iceberg column additions preserve their values through metadata serialization and reads/writes. Non-finite floating defaults fail before commit. ### Check List (For Author) - Test: 89 focused FE tests passed; full build.sh --fe, Checkstyle and both connector architecture gates passed. The preset regression runner generated the new expected output and then successfully compared all four affected external suites against isolated REST/MinIO services. Local read/write validation used the branch-built FE and an ASAN BE cache; BE code is unchanged. - Behavior changed: Yes; correct typed default conversion and prevent publication of unreadable floating defaults. Existing SHOW CREATE default presentation remains unchanged. - Does this need documentation: Yes; the connector contract and fixture requirements are updated in this commit.
What problem does this PR solve?
Issue Number: None
Related PR: #66538, #65851
Problem Summary:
In short. A write to an Iceberg branch binds to the schema of the branch's head snapshot. Iceberg, Spark and the Iceberg docs all bind a branch write to the table's current schema, and so do branch-4.1 and branch-4.2 (#66538). This PR does the same on master.
Background. #65851 pins a statement-scoped writer schema for every Iceberg write (
IcebergWriteSchemaContext): the columns that the target list binds to, their write-defaults, and the partition spec and sort order bound to that schema. ForINSERT [OVERWRITE] INTO t@branch(b)it pins the schema of the branch head snapshot, although a read of the branch already uses the table-current schema. Iceberg commits a branch write with the table-current schema:SnapshotProducerrecordsbase.currentSchemaId()as the schema of the new snapshot, whatever its target ref. So master also needsvalidateBranchWriterSchema, which checks that the current schema can describe files written with the branch head schema. The Iceberg docs ("Schema selection with branches and tags") say that a write to a branch uses the table schema and only a read of a tag uses the snapshot's schema. Spark 4.0 with Iceberg 1.10.1 binds branch writes the same way: after a rename on main, a write to an older branch accepts the new name and rejects the old one. branch-4.1 and branch-4.2 (#66538) pintable.schema()for branch writes.The problem, and what it cost. Reproduced on a local cluster with a
file://format-version 3 tablet (id INT, c INT)holding one row, with a branchbcreated before the change on main:RENAME COLUMN c c2INSERT INTO t@branch(b) (id, c2) VALUES (1, 1)Unknown column 'c2' in target tableRENAME COLUMN c c2INSERT INTO t@branch(b) (id, c) VALUES (2, 2)Unknown column 'c' in target tableADD COLUMN p INT, thenADD PARTITION KEY pINSERT INTO t@branch(b) (id, c) VALUES (1, 1)Iceberg partition field 1000 references source field 3 outside pinned schema 0pis NULLdefault_intchanged from 35 to 36 (test_iceberg_initial_defaults, through the Iceberg API)INSERT OVERWRITE TABLE t@branch(b) (id, default_int) VALUES (14, DEFAULT)The partition case has no way out: only a successful write to the branch moves its head to a newer schema, and every write to it fails.
How this PR fixes it.
IcebergWriteSchemaContext.createstill checks that the target ref exists and is a branch, not a tag. It then pinstable.schema()for a branch, as for main, and binds the partition spec and sort order to that schema.validateCurrentSchema, the check before the commit, compares the pinned schema id with the table-current schema id for branch writes too. A schema change after the statement pinned its schema now fails the commit. Before, only a change of the branch head did.validateBranchWriterSchemais removed. The written files now carry the schema that Iceberg records for the new snapshot.Results.
IcebergWritePlanProviderTest: the five tests that pinned the branch head schema now expect the current schema. Together they cover the column names (also throughgetWriteColumnsand the sink's schema JSON), the write-defaults, a current required column with and without a default (without one, an omitted value fails at analysis, as for main), and a schema change after pinning, which must fail the commit. Two new tests cover a current partition spec whose source column the branch head lacks, and a tag or a missing ref, which is still rejected. With master'sIcebergWriteSchemaContext, 6 of these 7 tests fail and the tag test passes. All 1691 tests offe-connector-icebergpass.test_iceberg_initial_defaults,write/test_iceberg_write_evolution_refs,branch_tag/iceberg_branch_tag_schema_change_extendedandtest_iceberg_schema_ref_actions_matrixnow expect what branch-4.2 expects. Intest_iceberg_initial_defaultsthe branch write gets the current write-default 36. The other three write an older branch with the current column names and expect the old names to fail. All four pass locally. Without this change each one fails at the new expectation: the branch rows oftest_iceberg_initial_defaultsread 35,test_iceberg_write_evolution_refsandiceberg_branch_tag_schema_change_extendedgetUnknown column 'zone'andUnknown column 'new_col', and intest_iceberg_schema_ref_actions_matrixthe INSERT with the old column name succeeds.Release note
Fix: Writes to an Iceberg branch now use the table's current schema (column names, write-defaults and partition spec), as Spark does, instead of the schema of the branch's head snapshot.
Check List (For Author)
Test
On a local cluster with a
file://Iceberg catalog:CREATE TABLE t (id INT, c INT) PROPERTIES ('format-version'='3'), insert a row,ALTER TABLE t CREATE BRANCH b, then the changes and statements in the table above.Behavior changed:
See the release note. A write to a branch created before a schema change must use the current column names; the old names, which master accepted, now fail. An INSERT OVERWRITE into a connector table whose writer schema or write metadata changes while the statement plans now fails with "retry the statement" instead of writing its values into the columns of the new schema by position.
Does this need documentation?
Check List (For Reviewer who merge this PR)