feat: add ReplacePartitions core class (PR1 of 2 for #637)#776
feat: add ReplacePartitions core class (PR1 of 2 for #637)#776shangxinli wants to merge 6 commits into
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
| // the partition values are empty and naturally match every file under that | ||
| // spec — no separate AlwaysTrue path is needed, and validation stays scoped | ||
| // to the spec rather than the whole table. | ||
| ICEBERG_BUILDER_RETURN_IF_ERROR(DropPartition(spec_id, file->partition)); |
There was a problem hiding this comment.
unpartitioned replace is spec-scoped, but Java treats it as table-wide.
…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.
| // No-op update: no partitions were staged and no table-wide replace was | ||
| // requested, so there is nothing to conflict with. Calling the validators | ||
| // with AlwaysTrue here would turn an empty builder into a full-table check. | ||
| if (!replace_by_row_filter_ && replaced_partitions_.empty()) { |
There was a problem hiding this comment.
Java rejects no-data-files replace. Suggest to call DataSpec() like Java.
There was a problem hiding this comment.
Not sure where it's been implemented. Can you check again?
There was a problem hiding this comment.
Sorry, the previous "Done" was premature — DataSpec() wasn't actually being called at that point. It's now in place: Validate() calls DataSpec() in the partitioned path (!replace_by_row_filter_), which throws if no files were staged or if staged files span multiple partition specs, matching Java's BaseReplacePartitions.validate.
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. |
| ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto spec, base().PartitionSpecById(spec_id)); | ||
|
|
||
| ICEBERG_BUILDER_RETURN_IF_ERROR(AddDataFile(file)); | ||
| if (spec->fields().empty()) { |
There was a problem hiding this comment.
Java's PartitionSpec.isUnpartitioned() returns true not only for an empty field list but also for specs where all partition transforms are void/alwaysNull. With an all-void partition spec, BaseReplacePartitions.apply goes through the unpartitioned path and deletes by alwaysTrue(), replacing the whole table. This check only looks at fields().empty(), so C++ will treat an all-void spec as partitioned and call DropPartition, leaving files from other equivalent unpartitioned/all-void specs instead of doing the table-wide replace.
There was a problem hiding this comment.
Fixed. Replaced the fields().empty() guard with an is_unpartitioned lambda that matches Java's PartitionSpec.isUnpartitioned() — a spec is treated as unpartitioned if it has no fields or all transforms are void. All-void specs now route through DeleteByRowFilter(AlwaysTrue()) instead of DropPartition.
| // Match Java BaseReplacePartitions: require at least one staged data file. | ||
| // Use the flags AddFile() sets — `DataSpec()` would also error on multi-spec | ||
| // stages and is not the guard we want here. | ||
| if (!replace_by_row_filter_ && replaced_partitions_.empty()) { |
There was a problem hiding this comment.
Java still calls dataSpec() from validate/apply, which enforces that exactly one partition spec is represented by the staged data files. This guard only checks that at least one partition was recorded, so a replace containing files from multiple spec IDs can commit in C++ even though Java rejects it. That can produce snapshot semantics that don't align with Java after spec evolution.
There was a problem hiding this comment.
Fixed. Validate() now calls DataSpec() in the partitioned path, which errors if no files were staged or if staged files span more than one partition spec — matching Java's behavior. The unpartitioned path skips it because replace_by_row_filter_ is only set after a successful AddFile(), so at least one file is already guaranteed.
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.
| // file and that all staged files share exactly one partition spec. | ||
| // DataSpec() enforces both invariants; ignore the returned spec here since | ||
| // replace_by_row_filter_ / replaced_partitions_ already scope validation. | ||
| if (!replace_by_row_filter_) { |
There was a problem hiding this comment.
Please call DataSpec() before this branch unconditionally. If an unpartitioned or all-void file is added first, replace_by_row_filter_ becomes true and the single-spec check is skipped; a later file from another spec then commits a table-wide replace, while Java rejects mixed specs.
|
|
||
| std::string ReplacePartitions::operation() { return DataOperation::kOverwrite; } | ||
|
|
||
| Status ReplacePartitions::Validate(const TableMetadata& current_metadata, |
There was a problem hiding this comment.
Please add direct ReplacePartitions tests before merging this core class. Cover partitioned and unpartitioned/all-void replacement, empty and mixed specs, append-only validation, same- and different-partition concurrency, retry summary retention, and DV cleanup. Existing base-class tests do not exercise this orchestration.
| } | ||
|
|
||
| ReplacePartitions& ReplacePartitions::ValidateAppendOnly() { | ||
| FailAnyDelete(); |
There was a problem hiding this comment.
Please translate the fail-any-delete error in ReplacePartitions, as BaseReplacePartitions.apply() does. C++ currently returns "Operation would delete existing data", while Java returns "Cannot commit file that conflicts with existing partition: ...".
| // partitions; without it a concurrent delete in a replaced partition would | ||
| // commit silently. | ||
| if (replace_by_row_filter_) { | ||
| ICEBERG_RETURN_UNEXPECTED(ValidateNoNewDeleteFiles( |
There was a problem hiding this comment.
Please run ValidateDeletedDataFiles before ValidateNoNewDeleteFiles in both branches, matching Java. When both conflict types exist, the current order reports a different validation failure.
| ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto spec, base().PartitionSpecById(spec_id)); | ||
|
|
||
| ICEBERG_BUILDER_RETURN_IF_ERROR(AddDataFile(file)); | ||
| // A spec is effectively unpartitioned if it has no fields, or every field |
There was a problem hiding this comment.
Please keep these comments focused on the rule. For example: "Specs with no non-void transforms are unpartitioned." Remove the Java comparison and implementation-history detail here and in the validation block below.
| std::optional<int64_t> starting_snapshot_id_; | ||
| bool validate_conflicting_data_{false}; | ||
| bool validate_conflicting_deletes_{false}; | ||
| // True once an AddFile() call has staged a file whose partition spec is |
There was a problem hiding this comment.
Please describe this member directly, for example: "True when replacement uses an AlwaysTrue row filter." Keep the Java comparison and design rationale in the PR discussion.
| /// \return Reference to this for method chaining | ||
| ReplacePartitions& ValidateNoConflictingData(); | ||
|
|
||
| /// \brief Enable validation that no conflicting delete files were added concurrently. |
There was a problem hiding this comment.
Please align this contract with Java. This validation also rejects concurrent data-file removals, not only newly added delete files; mention that it prevents undeleting rows removed by a concurrent commit.
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