Repository navigation
[fix](fe) Bind Iceberg branch writes to the table's current schema - #69499
Open
morningman wants to merge 2 commits into
Open
morningman wants to merge 2 commits into
morningman wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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).
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
hubgeter
previously approved these changes
Oct 23, 2026
924060929
approved these changes
Oct 23, 2026
924060929
previously approved these changes
Oct 23, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28362 ms |
Contributor
TPC-DS: Total hot run time: 150596 ms |
Contributor
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) <noreply@anthropic.com>
morningman
force-pushed
the
b2m-t35k-iceberg-branch-write-current-schema
branch
from
October 23, 2026 09:22
a218feb to
2937f34
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
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(); |
Contributor
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.
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 28692 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 150375 ms |
Contributor
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) <noreply@anthropic.com>
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: #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.
Does this need documentation?
Check List (For Reviewer who merge this PR)