feat: add ReplacePartitions core class (PR1 of 2 for #637) - #776
Conversation
ae973f6 to
e33f433
Compare
Adds iceberg::ReplacePartitions — the dynamic partition overwrite
operation. Each AddFile() registers the file's partition for
replacement of all existing data and delete files in that partition.
Produces an overwrite snapshot with "replace-partitions=true" in the
summary. Unpartitioned tables replace all existing data files.
Extends MergingSnapshotUpdate (matching Java's BaseReplacePartitions)
so the full data+delete manifest pipeline, custom-summary-property
handling, and conflict-validation helpers are inherited. AddFile()
unconditionally calls DropPartition(spec_id, file->partition) — for
unpartitioned specs the partition value is empty and the filter
manager matches every file in that spec, so no separate AlwaysTrue
path is needed. Touched partitions are tracked in a PartitionSet;
Validate() uses the partition-scoped overloads of
ValidateAddedDataFiles / ValidateNoNewDeleteFiles, or skips entirely
when no partitions were staged.
Changes:
* iceberg::ReplacePartitions extending MergingSnapshotUpdate with
builder API (AddFile, ValidateAppendOnly, ValidateFromSnapshot,
ValidateNoConflictingData, ValidateNoConflictingDeletes) and
Validate() override.
* SnapshotSummaryFields::kReplacePartitions = "replace-partitions".
* MergingSnapshotUpdate::SetSummaryProperty promoted from private to
protected so subclasses can stash custom summary entries that
survive commit retry via the cached-rebuild path.
* Forward declaration in type_fwd.h.
* CMake + Meson source registration.
Public API wiring (Table::NewReplacePartitions(),
Transaction::NewReplacePartitions()) and end-to-end tests are
deferred to PR2.
Tracking: apache#775
Related: apache#637
e33f433 to
12dc230
Compare
| current_metadata, starting_snapshot_id_, replaced_partitions_, snapshot, io)); | ||
| } | ||
| if (validate_conflicting_deletes_) { | ||
| ICEBERG_RETURN_UNEXPECTED(ValidateNoNewDeleteFiles( |
There was a problem hiding this comment.
Java BaseReplacePartitions.validate also calls validateDeletedDataFiles, so concurrent overwrite/delete commits in the replaced partition are rejected
…aFiles - AddFile() now uses DeleteByRowFilter(AlwaysTrue()) for unpartitioned specs instead of DropPartition with empty partition values, matching Java BaseReplacePartitions which treats unpartitioned tables as a table-wide replace. - Validate() now also calls ValidateDeletedDataFiles when ValidateNoConflictingDeletes is enabled, mirroring Java where validateNewDeletes gates both checks. This rejects concurrent overwrite/delete commits in the replaced partitions. - New replace_by_row_filter_ flag drives the AlwaysTrue path in Validate() for the unpartitioned case.
manuzhang
left a comment
There was a problem hiding this comment.
It looks replace_partitions.h is missing from src/iceberg/update/meson.build.
|
In the PR description, should "the iceberg::ReplacePartitions class extending SnapshotUpdate" be changed to "the iceberg::ReplacePartitions class extending MergingSnapshotUpdate"? |
|
|
||
| namespace iceberg { | ||
|
|
||
| /// \brief Replaces partitions in a table with new data files. |
There was a problem hiding this comment.
Do we need to mention This is provided to implement SQL compatible with Hive table operations but is not recommended. Instead, use the {@link OverwriteFiles overwrite API} to explicitly overwrite data. like java?
There was a problem hiding this comment.
Changed to '/// \note This is provided to implement SQL compatible with Hive table operations but is not recommended. Instead, use OverwriteFiles to explicitly overwrite data.'
…date - Add replace_partitions.h to src/iceberg/update/meson.build (manuzhang) - Add Java-style note recommending OverwriteFiles for non-Hive use (zhjwpku) - Validate(): call DataSpec() to reject no-data-files replace, matching Java BaseReplacePartitions which throws when no file was added (manuzhang)
DataSpec() also errors on multi-spec stages, which would silently break a ReplacePartitions that touches files from more than one spec. Use the flags AddFile() sets (replace_by_row_filter_ / replaced_partitions_) instead — the same condition that already gated the no-op early-return below, now promoted to an error to match Java BaseReplacePartitions.
Updated the PR description — "extending SnapshotUpdate" → "extending MergingSnapshotUpdate". Thanks for catching that. |
wgtmac
left a comment
There was a problem hiding this comment.
Codex was used to review Java parity against Iceberg Java's ReplacePartitions, BaseReplacePartitions, MergingSnapshotProducer, and TestReplacePartitions. The inline comments cover the remaining behavior, API documentation, and test gaps.
- Call DataSpec() unconditionally in Validate so a replace mixing an unpartitioned/all-void file with files from another spec is rejected instead of committing a table-wide replace. - Override Apply() to translate the fail-any-delete error into Java's "Cannot commit file that conflicts with existing partition" wording. - Run ValidateDeletedDataFiles before ValidateNoNewDeleteFiles in both branches to match Java's deterministic failure ordering. - Trim Java-comparison/implementation-history comments to focus on the rule; align the ValidateNoConflictingDeletes contract with Java. - Add direct ReplacePartitions tests: partitioned and unpartitioned replacement, empty and mixed specs, append-only validation, and same/different-partition data and delete concurrency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ions-core # Conflicts: # src/iceberg/update/merging_snapshot_update.h
wgtmac
left a comment
There was a problem hiding this comment.
Codex was used for this Java parity review against the current Iceberg Java ReplacePartitions, BaseReplacePartitions, and tests. The remaining inline comments cover public contracts, comment cleanup, and focused test gaps.
- replace_partitions.h/.cc: drop Java-comparison and fix-history detail from Apply()/Validate() and the header; state only the contract. - Document the default idempotent-validation behavior on the class and correct the ValidateAppendOnly() contract wording (fails on any partition replacement and pre-existing delete files, not just data-file deletion). - ReplaceLeavesOtherPartitions: assert live file paths instead of summary counts. - AppendOnlyConflictTranslated: assert the full translated message with the partition path (x=1). - Add tests: all-void spec table-wide replace, unpartitioned conflict validation, concurrent data-file removal rejected by ValidateNoConflictingDeletes, no-validation idempotent same-partition replace, and format-v3 DV cleanup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- NoValidationSamePartitionConcurrentReplaceCommits: assert the replacement is the only live file after the idempotent commit, not just that it succeeded. - AllVoidSpecReplacesWholeTable: also assert added-data-files == 1 so the summary fully pins the all-void file as the sole survivor. - ReplaceCleansUpDeletionVectors: assert the exact DV path before and its absence after, instead of a raw live-delete count. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zhjwpku
left a comment
There was a problem hiding this comment.
Generally LGTM, except for two minor nits from me.
wgtmac
left a comment
There was a problem hiding this comment.
Thanks @shangxinli for working on this! I took the liberty to address open comments and align with the Java behavior as much as possible.
Summary
Adds the
iceberg::ReplacePartitionsclass — the dynamic partition overwrite operation. EachAddFile()registers the file's partition for replacement of all existing data files in that partition. Produces anoverwritesnapshot with"replace-partitions"="true"in the summary. Unpartitioned tables replace all existing data files.This is PR1 of 2 for #637. See tracking issue #775 for the split rationale.
What's in PR1
iceberg::ReplacePartitionsclass extendingMergingSnapshotUpdate:AddFile,ValidateAppendOnly,ValidateFromSnapshot,ValidateNoConflictingData,ValidateNoConflictingDeletesApply/Summary/CleanUncommittedoverridesManifestFilterManagerfor partition-based manifest filteringSnapshotSummaryFields::kReplacePartitions = "replace-partitions"constantReplacePartitionsforward declaration intype_fwd.hWhat's deferred to PR2
Transaction::NewReplacePartitions()API wiring onTable/Transactionreplace_partitions_test.cc)Design notes
ReplacePartitionssemantics: operation type isoverwrite, summary carries"replace-partitions"="true".FastAppend/MergeAppendpatterns (factoryMake(),MergingSnapshotUpdatebase, builder error collector).DeleteByRowFilter(AlwaysTrue()), matching JavaBaseReplacePartitions.Test plan
cmake --build build)clang-formatcleanTable/TransactionAPI wiringCloses part of #637 (PR2 will close it).
Tracking: #775