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 4 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?
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.
| // 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.
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