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.
|
|
||
| SnapshotManager& SnapshotManager::Cherrypick(int64_t snapshot_id) { | ||
| ICEBERG_BUILDER_RETURN_IF_ERROR(CommitIfRefUpdatesExist()); | ||
| // TODO(anyone): Implement cherrypick operation |
2 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.