morningman opened a new pull request, #69358:
URL: https://github.com/apache/doris/pull/69358
### What problem does this PR solve?
Issue Number: None
Related PR: #69229
Problem Summary:
**In short.** Doris rejects every write to a format-v1 Iceberg table whose
current partition spec holds a void field, the field Iceberg keeps for each
partition field dropped from a v1 table. This PR pins such a spec as it is,
skips void fields wherever the write path looks for a partition source column,
and lets BE write NULL for a void field without resolving its source. The
problem came up in the end-to-end checks for #69229.
**Background.** Dropping a partition field from a v1 table does not remove
it from the spec: Iceberg replaces it with a void transform, which yields NULL
for every row (v2 removes the field). The column the field came from can be
dropped afterwards, which Iceberg allows because a void field never reads it.
Before planning a write, Doris pins the schema and the partition spec in
`IcebergWriteSchemaContext`, and at commit time it checks that the table still
has the pinned spec with the same definition.
**The problem, and what it cost.**
| Current spec of the v1 table | Spark 4.0 with Iceberg 1.10 | Doris master
| Doris with this PR |
|---|---|---|---|
| `[void(region)]`, region kept or dropped | Writes | INSERT and INSERT
OVERWRITE fail: `pinned spec 1 is not available with the same definition; retry
the statement` | Written like an unpartitioned table and committed under spec 1
|
| `[void(region), identity(category)]`, region dropped | Reads, but cannot
write: its partition key needs an accessor for every source field | Planning
fails: `Iceberg partition field 1000 references source field 2 outside pinned
schema 1` | Written under spec 1 with region NULL, routed by category |
| `[void(region), identity(category)]`, region kept | Writes | Works | Works
|
| The tables above after an upgrade to format v2 | As above | DELETE, UPDATE
and MERGE fail the same way | Work |
- `bindPartitionSpec` replaces every spec whose `isPartitioned()` is false
with an unpartitioned stand-in that has the same spec id. An all-void spec is
not partitioned in that sense but still has fields, so the stand-in's JSON
never equals the table's spec and the commit-time check always fails.
- `validateWriterMetadataSources` requires the source column of every
partition field, void included, and BE
`VIcebergTableWriter::_to_iceberg_partition_columns` resolves the source of
every field as well.
**How this PR fixes it.**
- `IcebergWriteSchemaContext` gives the unpartitioned stand-in only to a
spec without fields and binds every other spec as it is, so the pinned
definition equals the table's. Its source check skips void fields.
- `IcebergWritePlanProvider` skips void fields in the INSERT routing
(`getWriteDistribution`) and in the MERGE insert partitioning
(`getWritePartitioning`). They are NULL for every row; without the skip, a
dropped source turns the write random, and a table with many partitions can
exceed `table_sink_partition_write_max_partition_nums_per_writer` on one writer.
- BE `VIcebergTableWriter` no longer resolves a source for a void field and
fills a NULL column for it, so the file lands under `region=null/...` and
commits NULL for the field, as Spark writes the same spec while the column
exists. Iceberg itself allows dropping that column, because a void field never
reads it.
- An all-void spec stays unpartitioned for the write: the sink carries no
spec, BE writes unpartitioned files, and the commit builds the data files with
`DataFiles.builder(spec)` and no partition values, the path Spark's
unpartitioned writer takes. A dynamic overwrite replaces the whole table, since
`ReplacePartitions` checks `isUnpartitioned()`.
- During a rolling upgrade, a new FE with an old BE still fails for a void
field whose source column was dropped, with the old BE's `outside writer
schema` error before any file is written, so no exec version gate is needed.
Branch-4.1 and branch-4.2 have the same code in fe-core
`IcebergWriteSchemaContext` and in BE.
**Results.**
- Unit tests: the new
`IcebergWritePlanProviderTest#writeSchemaPinsV1VoidPartitionFields` and
`#writeRoutingSkipsV1VoidPartitionFields`,
`IcebergConnectorTransactionTest#insertCommitsNullForV1VoidPartitionFields`
(pins, commits and checks the committed data file for an all-void spec and for
a void field whose source was dropped) and
`VIcebergTableWriterTest.VoidPartitionFieldNeedsNoSourceColumn` pass, with all
1625 tests of fe-connector-iceberg and 91 BE tests of the Iceberg writer,
partition, MERGE / DELETE sink and related suites. Reverting each of the six
changes (the stand-in, the source check, the INSERT routing, the MERGE
partitioning, the BE source resolution, the BE NULL column) fails the tests
that cover it.
- Regression test: `test_iceberg_write_v1_void_partition` creates three v1
tables in Spark (void only, void only with the column dropped, void plus
identity with the column dropped), writes them with Doris INSERT and INSERT
OVERWRITE, upgrades them to merge-on-read v2 and runs DELETE, UPDATE and MERGE,
comparing every step with Spark, including Spark's partition pruning on the
identity field. With the master connector the first Doris INSERT fails with the
`pinned spec 1` error; with this PR the suite passes, as do all 43 suites of
`external_table_p0/iceberg/write`. Spark only reads the void plus identity
table, which it cannot write.
- Manual test, on a local Hadoop catalog with the same three shapes: INSERT,
INSERT OVERWRITE, and DELETE / UPDATE / MERGE after a v2 upgrade return the
expected rows, and the Iceberg API shows every new data and delete file under
the current spec with a NULL void value (`region=null/category=x/` for the
identity table, the table root for the void-only tables).
### Release note
Doris can write format-v1 Iceberg tables whose partition spec contains void
fields left by dropped partition fields, also after their source columns were
dropped.
### 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)
- [ ] 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:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- 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]
