feat: add snapshot util - #420
Conversation
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
I haven't finished my review yet. Just post my findings so far. Will review it later.
| const Table& table, int64_t snapshot_id) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto start, table.SnapshotById(snapshot_id)); | ||
| if (!start) { | ||
| return InvalidArgument("Cannot find snapshot: {}", snapshot_id); |
There was a problem hiding this comment.
| return InvalidArgument("Cannot find snapshot: {}", snapshot_id); | |
| return NotFound("Cannot find snapshot: {}", snapshot_id); |
There was a problem hiding this comment.
BTW, perhaps this should be a ICEBERG_DCHECK.
| break; | ||
| } | ||
| auto parent_result = table.SnapshotById(current->parent_snapshot_id.value()); | ||
| if (!parent_result.has_value()) { |
There was a problem hiding this comment.
We might need to check if the error is NotFound and return the original error for other kind of errors.
| // Parent snapshot not found (e.g., expired), stop traversal | ||
| break; | ||
| } | ||
| current = parent_result.value(); |
There was a problem hiding this comment.
| current = parent_result.value(); | |
| current = std::move(parent_result.value()); |
| /// Snapshot. | ||
| /// | ||
| /// \param table The table | ||
| /// \return The oldest snapshot, or nullopt if there is no current snapshot |
There was a problem hiding this comment.
It would be good to document that all returned std::shared_ptr<Snapshot> cannot be nullptr.
|
|
||
| Result<std::optional<std::shared_ptr<Snapshot>>> SnapshotUtil::OldestAncestorAfter( | ||
| const Table& table, TimePointMs timestamp_ms) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto current, table.current_snapshot()); |
There was a problem hiding this comment.
Actually it is not an error if current snapshot is not found. We can return std::nullopt when the result error is NotFound.
| Result<std::optional<std::shared_ptr<Snapshot>>> SnapshotUtil::OldestAncestorAfter( | ||
| const Table& table, TimePointMs timestamp_ms) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto current, table.current_snapshot()); | ||
| if (!current) { |
There was a problem hiding this comment.
table.current_snapshot() should not return a nullptr, so this should be an error instead.
| Result<bool> SnapshotUtil::IsAncestorOf(const Table& table, int64_t snapshot_id, | ||
| int64_t ancestor_snapshot_id) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto ancestors, AncestorsOf(table, snapshot_id)); | ||
| for (const auto& snapshot : ancestors) { |
There was a problem hiding this comment.
nit: use std::ranges for this kind of processing.
| return std::nullopt; | ||
| } | ||
|
|
||
| std::shared_ptr<Snapshot> last_snapshot = nullptr; |
There was a problem hiding this comment.
What about using std::optional<std::shared_ptr<Snapshot>> so we don't need to check at line 111.
| return last_snapshot; | ||
| } | ||
|
|
||
| return ValidationFailed("Cannot find snapshot older than {}", |
There was a problem hiding this comment.
| return ValidationFailed("Cannot find snapshot older than {}", | |
| return NotFound("Cannot find snapshot older than {}", |
There was a problem hiding this comment.
I'm undecided which is better, ValidationFailed, NotFound or Invalid.
| int64_t to_snapshot_id) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto to_snapshot, table.SnapshotById(to_snapshot_id)); | ||
| if (!to_snapshot) { | ||
| return InvalidArgument("Cannot find snapshot: {}", to_snapshot_id); |
There was a problem hiding this comment.
InvalidArgument should be used for input argument. This is an invalid state so perhaps return Invalid?
79b4b5c to
2dc17d0
Compare
2 participants
No description provided.