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?
| /// 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
There was a problem hiding this comment.
My understanding is that UpdateSnapshotReference should be added to the Transaction's pending updates, whereas SnapshotManager should no, just as Gang noted below, SnapshotManager has its own commit logic.
src/iceberg/update/snapshot_update.h
Outdated
| /// | ||
| /// \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?
There was a problem hiding this comment.
To be precise, I was suggesting to remove ToBranch by reusing SetTargetBranch because they are literally equivalent. If we think ToBranch is a better name, we should just rename it without adding a new one.
There was a problem hiding this comment.
Got it, changed to SetTargetBranch.
src/iceberg/update/snapshot_update.h
Outdated
| /// | ||
| /// \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.
To be precise, I was suggesting to remove ToBranch by reusing SetTargetBranch because they are literally equivalent. If we think ToBranch is a better name, we should just rename it without adding a new one.
|
|
||
| /// \brief Roll this table's data back to a specific Snapshot identified by id. | ||
| /// | ||
| /// \param snapshot_id Long id of the snapshot to roll back table data to |
There was a problem hiding this comment.
| /// \param snapshot_id Long id of the snapshot to roll back table data to | |
| /// \param snapshot_id id of the snapshot to roll back table data to |
| /// \brief Create a new tag pointing to the given snapshot id. | ||
| /// | ||
| /// \param name Tag name | ||
| /// \param snapshot_id Snapshot ID for the head of the new tag |
There was a problem hiding this comment.
Let's be consistent with id, ID and Snapshot ID in these comments.
There was a problem hiding this comment.
All changed to Snapshot ID since it's used in other places.
| return *this; | ||
| } | ||
|
|
||
| SnapshotManager& SnapshotManager::SetCurrentSnapshot(int64_t snapshot_id) { |
There was a problem hiding this comment.
IIUC, we need to call CommitIfRefUpdatesExist() before anything in this SetCurrentSnapshot and RollbackToXXX below because different pending updates like UpdateSnapshotReference and SetSnapshot cannot be interleaved.
There was a problem hiding this comment.
Yes, the Java impl behaves this way, but I'm not entirely sure about our implementation. After calling CommitIfRefUpdatesExist(), the transaction state becomes committed, which should prevent any further commits.
There was a problem hiding this comment.
Maybe we can remove CommitIfRefUpdatesExist and put its logic directly into Commit.
| Result<std::shared_ptr<Snapshot>> SnapshotManager::Apply() { return base().Snapshot(); } | ||
|
|
||
| Status SnapshotManager::Commit() { | ||
| ICEBERG_RETURN_UNEXPECTED(CheckErrors()); |
There was a problem hiding this comment.
Shouldn't we call CheckErrors() in the Apply()?
There was a problem hiding this comment.
Apply() removed, so I think the CheckErrors() here is still needed.
| return *this; | ||
| } | ||
|
|
||
| Result<std::shared_ptr<Snapshot>> SnapshotManager::Apply() { return base().Snapshot(); } |
There was a problem hiding this comment.
If nothing happens on the transaction side and nowhere calls this, perhaps we don't even need the Apply function?
There was a problem hiding this comment.
Reasonable, let's just remove it for now.
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.