Repository navigation
[fix](iceberg) Write v1 tables whose partition spec has void fields - #69358
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69229 Problem Summary: Dropping a partition field from a format-v1 Iceberg table leaves a void field in the spec, and the field's source column may be dropped later. Spark writes such tables (it only reads a partitioned spec whose void field lost its column), but Doris rejected every write before any file was written: - When every field of the current spec is void, bindPartitionSpec pinned an unpartitioned stand-in (isPartitioned() is false for an all-void spec) whose JSON never matched the table's spec, so the commit-time check failed with "pinned spec N is not available with the same definition; retry the statement". - When the source column of a void field was dropped, validateWriterMetadataSources failed with "partition field ... references source field ... outside pinned schema", and BE VIcebergTableWriter would look up the missing column as well. This commit binds every spec that has fields as it is, skips void fields in the source check, in the INSERT routing and in the MERGE insert partitioning, and makes BE fill NULL for a void field without resolving its source. An all-void spec is still written as unpartitioned and committed under the table's spec, as Spark does. The same tables can also take DELETE, UPDATE and MERGE after an upgrade to format v2. ### 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: Unit Test (IcebergWritePlanProviderTest, IcebergConnectorTransactionTest, VIcebergTableWriterTest); Regression test (test_iceberg_write_v1_void_partition); Manual test (INSERT, INSERT OVERWRITE, and DELETE / UPDATE / MERGE after a v2 upgrade on a local Iceberg catalog) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
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 eight changed paths at a2a6965. No new actionable findings and no existing inline blocking findings. The FE, BE, and separate risk-integration reviewers all returned NO_NEW_VALUABLE_FINDINGS on the same empty inline set; required coverage and all candidate/risk dispositions are complete. The review converged after one round.
Critical-checkpoint conclusions:
- Goal and tests: the change preserves the real void-containing spec for schema/commit validation, skips source lookup for constant-null fields, and routes by active fields. Added FE pinning/routing/commit tests, BE two-row routing, and Spark/Doris regression cases target this behavior. Runtime success was not independently verified.
- Scope and reuse: the implementation is small and focused, reusing the existing partition transform, nullable column, sink, and commit abstractions.
- Concurrency and locking: changed FE loops use pinned metadata/local lists; transformed BE columns belong to the writer task. No new shared state, thread, lock, blocking critical section, or lock-order edge was found.
- Lifecycle and initialization: statement contexts, spec-field references, temporary columns, sorted-writer captures, table leases and file cleanup keep their existing ownership boundaries. No new cycle or cross-translation-unit static initialization dependency was introduced.
- Configuration: no configuration item or dynamic-update behavior changes.
- Compatibility: no stored format, SPI signature, or new wire dialect changes. All-void writes keep the actual FE spec ID while using the existing unpartitioned BE mode; the pinned SDK retains that ID in its DataFiles builder. An old BE still rejects a mixed void spec with a dropped source before creating a writer; the new support requires the fixed BE.
- Thrift/protobuf: no definitions, field IDs, or enum values changed.
- Parallel paths: INSERT, whole-table/static/dynamic overwrite, REWRITE and MERGE data writes share the relevant fixed path. DELETE commits retain original per-file spec IDs/tuples. Sorted/unsorted writers and Parquet/ORC were traced.
- Conditional checks and errors: comments explain why void sources are omitted and NULLs materialized. Nullable defaults short-circuit value/type access, valid static partitions admit active identity fields only, and missing nonvoid sources still fail explicitly.
- Test coverage: retained/dropped source, void-only/mixed identity, spec pinning, route fields, commit NULLs, Spark reads/pruning, overwrite and upgraded-v2 DELETE/UPDATE/MERGE are covered by the added tests. Existing missing-source rejection remains a negative guard. Multiple/reordered/nonstring void fields, static hybrids, branch/evolution and rollout interactions were checked statically.
- Expected results: all 12 ordered query tags match the output sections, and expected rows/spec IDs agree with the SQL state transitions. Runtime results and automatic output-generation provenance were not verified in this invocation.
- Observability: existing failure messages and writer profile counters cover the added constant-null path; no additional logging/metric requirement was identified.
- Persistence and failover: no new edit-log or FE-local persistent state. Existing schema/format/table/spec/order fences and SDK metadata CAS/retry remain the publication boundary. Native visible-version and MoW bitmap paths are outside these changes.
- Writes and atomicity: filtering hash inputs does not filter file partition tuples. Full specs reach data writers/commit conversion; historical delete fragments are grouped by their original spec. Existing transaction staging, conflict checks, abort and file-cleanup paths remain intact.
- FE-to-BE values: no new transmitted variable; existing partition-spec JSON retains the complete mixed spec while routing metadata carries only active fields.
- Performance and memory: new NULL materialization is linear in rows times void fields and uses existing column allocators/lifetimes. No new unbounded ownership, redundant metadata IO or asymptotic regression was found.
- Other risks: source/type lookup, static overrides, filtered-distribution/full-tuple alignment, all-void metadata and historical delete-spec concerns were verified and dismissed with evidence. No unresolved introduced-correctness candidate remains.
No additional user focus was supplied. A pre-existing support limit remains: historical files with non-null partition values whose source was dropped can reject row-ID DML until rewritten; the regression explicitly rewrites them first. The unchanged scan guard was checked on both base and head. This is not a new inline finding.
Validation was static only, as required: no builds, tests, or repository source edits were performed. The review covers the complete supplied diff; it does not claim runtime validation of the author-reported results.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28480 ms |
TPC-DS: Total hot run time: 150935 ms |
ClickBench: Total hot run time: 24.29 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
…rom master ### What problem does this PR solve? Issue Number: None Related PR: apache#69327, apache#69183, apache#69487, apache#69358 Problem Summary: The first commit of this PR takes the async-writer dependencies out of VIcebergTableWriter, so its constructor takes only the sink and the output expressions. Master has since added four tests that still pass two null dependencies: RejectsRequiredNullBeforeWriterCreation and RejectsNestedRequiredNullBeforeWriterCreation (apache#69327) and VoidPartitionFieldNeedsNoSourceColumn (apache#69358) in viceberg_table_writer_test.cpp, and IcebergRejectsNextPartitionBeforeOpeningAFile in external_table_writer_partition_limit_test.cpp (apache#69183, which apache#69487 made pin the writer schema). On that master doris_be_test does not compile. Construct the writer in these tests with the two arguments; nothing else in them changes. ### Release note None ### Check List (For Author) - Test: Unit Test - doris_be_test builds on master 2e7f79e with this PR; 64 tests in VIcebergTableWriterTest, VIcebergTableWriterLifecycleTest, ExternalTableWriterPartitionLimitTest, IcebergTableSinkOperatorTest, SpillIcebergTableSinkOperatorTest, VIcebergPartitionWriterTest, IcebergWriteValidatorTest, FullSorterTest and SorterReserveMemoryTest pass (ASAN) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
PR approved by anyone and no changes requested. |
|
PR approved by at least one committer and no changes requested. |
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.
[void(region)], region kept or droppedpinned spec 1 is not available with the same definition; retry the statement[void(region), identity(category)], region droppedIceberg partition field 1000 references source field 2 outside pinned schema 1[void(region), identity(category)], region keptbindPartitionSpecreplaces every spec whoseisPartitioned()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.validateWriterMetadataSourcesrequires the source column of every partition field, void included, and BEVIcebergTableWriter::_to_iceberg_partition_columnsresolves the source of every field as well.How this PR fixes it.
IcebergWriteSchemaContextgives 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.IcebergWritePlanProviderskips 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 exceedtable_sink_partition_write_max_partition_nums_per_writeron one writer.VIcebergTableWriterno longer resolves a source for a void field and fills a NULL column for it, so the file lands underregion=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.DataFiles.builder(spec)and no partition values, the path Spark's unpartitioned writer takes. A dynamic overwrite replaces the whole table, sinceReplacePartitionschecksisUnpartitioned().outside writer schemaerror 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
IcebergWriteSchemaContextand in BE.Results.
IcebergWritePlanProviderTest#writeSchemaPinsV1VoidPartitionFieldsand#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) andVIcebergTableWriterTest.VoidPartitionFieldNeedsNoSourceColumnpass, 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.test_iceberg_write_v1_void_partitioncreates 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 thepinned spec 1error; with this PR the suite passes, as do all 43 suites ofexternal_table_p0/iceberg/write. Spark only reads the void plus identity table, which it cannot write.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
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)