feat: add SnapshotManager #542
Conversation
The test cases are derived from Java's TestSnapshotManager.java. Since MergeAppend is not supported yet, some tests are omitted for now and can be added later.
evindj
left a comment
evindj
left a comment
There was a problem hiding this comment.
Overall LGTM only question that I have is why is UpdateSnapshotReference and SnapShotManager both exposed. Also, is there a way we can integration test this change?
| protected: | ||
| // These snapshot IDs correspond to the snapshots in the TableMetadataV2Valid.json | ||
| static constexpr int64_t kOldestSnapshotId = 3051729675574597004; | ||
| static constexpr int64_t kCurrentSnapshotId = 3055729675574597004; |
There was a problem hiding this comment.
since the metadata file is read in the base test setup, will it make sense to extract these in variables instead of copy pasta?
| /// and tags) and commit the changes. | ||
| Result<std::shared_ptr<UpdateSnapshotReference>> NewUpdateSnapshotReference(); | ||
|
|
||
| /// \brief Create a new SnapshotManager to manage snapshots. |
There was a problem hiding this comment.
Can you be a bit more descriptive here especially how it differs from NewUpdateSnapshotReference() I am a little confuse as to why the transaction would expose both since SnapshotManager seems to be a wrapper on top of UpdateSnapshotReference
| ~SnapshotManager() override; | ||
|
|
||
| // TODO(xxx): is this correct? | ||
| Kind kind() const final { return Kind::kUpdateSnapshotReference; } |
There was a problem hiding this comment.
According to https://github.com/apache/iceberg-cpp/blob/main/src/iceberg/update/update_snapshot_reference.h this looks correct. But would still probably want to wait for someone who knows more about this space.
There was a problem hiding this comment.
We need to pay attention to https://github.com/apache/iceberg/blob/f23486ffd7499d2b2f0bfd8ce7552c361fd86796/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L150-L153. SnapshotManager has its own commit logic so it looks tricky here.
|
|
||
| SnapshotManager& SnapshotManager::Cherrypick(int64_t snapshot_id) { | ||
| ICEBERG_BUILDER_RETURN_IF_ERROR(CommitIfRefUpdatesExist()); | ||
| // TODO(anyone): Implement cherrypick operation |
There was a problem hiding this comment.
Is there an issue for this?
There was a problem hiding this comment.
I guess the reason is the lack of CherryPickOperation implementation.
| /// \brief Table update. | ||
| class TableMetadataBuilder; | ||
| class TableUpdate; | ||
| class SnapshotManager; |
There was a problem hiding this comment.
Should we put it at line 195 below?
| /// | ||
| /// \param branch Which is name of SnapshotRef of type branch | ||
| /// \return Reference to this for method chaining | ||
| auto& ToBranch(this auto& self, const std::string& branch) { |
There was a problem hiding this comment.
Should we rename SetTargetBranch to ToBranch as they are equivalent?
| ~SnapshotManager() override; | ||
|
|
||
| // TODO(xxx): is this correct? | ||
| Kind kind() const final { return Kind::kUpdateSnapshotReference; } |
There was a problem hiding this comment.
We need to pay attention to https://github.com/apache/iceberg/blob/f23486ffd7499d2b2f0bfd8ce7552c361fd86796/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L150-L153. SnapshotManager has its own commit logic so it looks tricky here.
| /// \param table_name The name of the table | ||
| /// \param table The table to manage snapshots for | ||
| /// \return A new SnapshotManager instance, or an error if the table doesn't exist | ||
| static Result<std::shared_ptr<SnapshotManager>> Make(const std::string& table_name, |
There was a problem hiding this comment.
Can we remove this ctor? transaction internally holds a table which has table_identifier_ to get its name? We are slightly different than Java because we always create a transaction instance in the Table instance to create any update.
|
|
||
| SnapshotManager& SnapshotManager::Cherrypick(int64_t snapshot_id) { | ||
| ICEBERG_BUILDER_RETURN_IF_ERROR(CommitIfRefUpdatesExist()); | ||
| // TODO(anyone): Implement cherrypick operation |
There was a problem hiding this comment.
I guess the reason is the lack of CherryPickOperation implementation.
| /// \brief Commit any pending reference updates if they exist. | ||
| Status CommitIfRefUpdatesExist(); | ||
|
|
||
| bool is_external_transaction_; |
There was a problem hiding this comment.
I don't think we need this. Transaction::auto_commit_ has already taken good care of it.
| SnapshotManager& SnapshotManager::CreateBranch(const std::string& name) { | ||
| if (base().current_snapshot_id != kInvalidSnapshotId) { | ||
| ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto current_snapshot, base().Snapshot()); | ||
| if (current_snapshot != nullptr) { |
There was a problem hiding this comment.
It should be an error if current_snapshot_id exists but snapshot is null.
3 participants
The test cases are derived from Java's TestSnapshotManager.java. Since MergeAppend is not supported yet, some tests are omitted for now and can be added later.