feat: add FastAppend #516
Open
feat: add FastAppend #516
+698
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wgtmac
requested changes
Jan 19, 2026
| /// When committing, these changes will be applied to the latest table snapshot. Commit | ||
| /// conflicts will be resolved by applying the changes to the new latest snapshot and | ||
| /// reattempting the commit. | ||
| class ICEBERG_EXPORT AppendFiles { |
Member
There was a problem hiding this comment.
IMHO, we don't need this class. The Java impl's AppendFiles extends SnapshotUpdate so that one has more methods to call. We can just return FastAppend from NewFastAppend() and then return AppendFiles|MergeAppend from NewAppend().
| /// \return Reference to this for method chaining | ||
| FastAppend& Set(const std::string& property, const std::string& value); | ||
|
|
||
| Kind kind() const override { return Kind::kUpdateSnapshot; } |
Member
There was a problem hiding this comment.
Should this be moved to the base SnapshotUpdate class?
| /// | ||
| /// \param branch The branch name | ||
| /// \return Reference to this for method chaining | ||
| FastAppend& ToBranch(const std::string& branch); |
Member
There was a problem hiding this comment.
Can we simplify reuse SetTargetBranch or rename it to ToBranch if this is a better name?
| /// \param property The property name | ||
| /// \param value The property value | ||
| /// \return Reference to this for method chaining | ||
| FastAppend& Set(const std::string& property, const std::string& value); |
Member
There was a problem hiding this comment.
Should we directly define this as a virtual function in the SnapshotUpdate?
| Result<std::vector<ManifestFile>> WriteNewManifests(); | ||
|
|
||
| private: | ||
| struct DataFilePtrHash { |
Member
There was a problem hiding this comment.
Should we move DataFilePtrHash and DataFilePtrEqual to manifest_entry.h (where DataFile is defined) or content_file_util.h (if DataFileSet is needed as below comment)?
| // Cleanup after committing is disabled for FastAppend unless there are | ||
| // rewritten_append_manifests because: | ||
| // 1.) Appended manifests are never rewritten | ||
| // 2.) Manifests which are written out as part of appendFile are already cleaned |
Member
There was a problem hiding this comment.
Suggested change
| // 2.) Manifests which are written out as part of appendFile are already cleaned | |
| // 2.) Manifests which are written out as part of AppendFile are already cleaned |
| new_manifests_.clear(); | ||
| } | ||
|
|
||
| // Clean up only rewritten_append_manifests as they are always owned by the table |
Member
There was a problem hiding this comment.
Suggested change
| // Clean up only rewritten_append_manifests as they are always owned by the table | |
| // Clean up only rewritten append manifests as they are always owned by the table |
| } | ||
|
|
||
| // Clean up only rewritten_append_manifests as they are always owned by the table | ||
| // Don't clean up append_manifests as they are added to the manifest list and are |
Member
There was a problem hiding this comment.
Suggested change
| // Don't clean up append_manifests as they are added to the manifest list and are | |
| // Don't clean up append manifests as they are added to the manifest list and are |
| std::vector<std::shared_ptr<DataFile>> files; | ||
| files.reserve(data_files.size()); | ||
| std::ranges::copy(data_files, std::back_inserter(files)); | ||
| ICEBERG_ASSIGN_OR_RAISE(auto written_manifests, WriteDataManifests(files, spec)); |
Member
There was a problem hiding this comment.
We need to revisit the signature of WriteDataManifests to use iterator begin and end of DataFile. We can add a TODO comment to WriteDataManifests for now.
| // If there are new files and manifests were already written, clean them up | ||
| if (has_new_files_ && !new_manifests_.empty()) { | ||
| for (const auto& manifest : new_manifests_) { | ||
| ICEBERG_RETURN_UNEXPECTED(DeleteFile(manifest.manifest_path)); |
HuaHuaY
reviewed
Jan 19, 2026
| for (const auto& manifest : append_manifests_) { | ||
| ManifestFile updated = manifest; | ||
| updated.added_snapshot_id = snapshot_id; | ||
| manifests.push_back(updated); |
| for (const auto& manifest : rewritten_append_manifests_) { | ||
| ManifestFile updated = manifest; | ||
| updated.added_snapshot_id = snapshot_id; | ||
| manifests.push_back(updated); |
| // Clean up new manifests that were written but not committed | ||
| if (!new_manifests_.empty()) { | ||
| for (const auto& manifest : new_manifests_) { | ||
| if (committed.find(manifest.manifest_path) == committed.end()) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if (committed.find(manifest.manifest_path) == committed.end()) { | |
| if (!committed.contains(manifest.manifest_path))) { |
| // not compacted | ||
| if (!rewritten_append_manifests_.empty()) { | ||
| for (const auto& manifest : rewritten_append_manifests_) { | ||
| if (committed.find(manifest.manifest_path) == committed.end()) { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.