morningman opened a new pull request, #69499:
URL: https://github.com/apache/doris/pull/69499
### 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. For `INSERT [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: `SnapshotProducer` records `base.currentSchemaId()` as the schema of
the new snapshot, whatever its target ref. So master also needs
`validateBranchWriterSchema`, 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
bran
ch accepts the new name and rejects the old one. branch-4.1 and branch-4.2
(#66538) pin `table.schema()` for branch writes.
**The problem, and what it cost.** Reproduced on a local cluster with a
`file://` format-version 3 table `t (id INT, c INT)` holding one row, with a
branch `b` created before the change on main:
| Change on main after the branch | Statement | Master today | After this PR
(same as Spark and branch-4.2) |
|---|---|---|---|
| `RENAME COLUMN c c2` | `INSERT INTO t@branch(b) (id, c2) VALUES (1, 1)` |
`Unknown column 'c2' in target table` | writes the row |
| `RENAME COLUMN c c2` | `INSERT INTO t@branch(b) (id, c) VALUES (2, 2)` |
writes the row | `Unknown column 'c' in target table` |
| `ADD COLUMN p INT`, then `ADD PARTITION KEY p` | `INSERT INTO t@branch(b)
(id, c) VALUES (1, 1)` | `Iceberg partition field 1000 references source field
3 outside pinned schema 0` | writes the row, `p` is NULL |
| write-default of `default_int` changed from 35 to 36
(`test_iceberg_initial_defaults`, through the Iceberg API) | `INSERT OVERWRITE
TABLE t@branch(b) (id, default_int) VALUES (14, DEFAULT)` | writes 35 | writes
36 |
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.create` still checks that the target ref exists
and is a branch, not a tag. It then pins `table.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.
- `validateBranchWriterSchema` is removed. The written files now carry the
schema that Iceberg records for the new snapshot.
**Results.**
- Local cluster: the table above shows each statement before and after.
- Unit tests in `IcebergWritePlanProviderTest`: the five tests that pinned
the branch head schema now expect the current schema. Together they cover the
column names (also through `getWriteColumns` and 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's
`IcebergWriteSchemaContext`, 6 of these 7 tests fail and the tag test passes.
All 1645 tests of `fe-connector-iceberg` pass.
- Regression: `test_iceberg_initial_defaults`,
`write/test_iceberg_write_evolution_refs`,
`branch_tag/iceberg_branch_tag_schema_change_extended` and
`test_iceberg_schema_ref_actions_matrix` now expect what branch-4.2 expects. In
`test_iceberg_initial_defaults` the 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 of
`test_iceberg_initial_defaults` read 35, `test_iceberg_write_evolution_refs`
and `iceberg_branch_tag_schema_change_extended` get `Unknown column 'zone'` and
`Unknown column 'new_col'`, and in `test_iceberg_schema_ref_actions_matrix` the
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 <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
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.
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
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?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
