feat(iceberg): Add snapshot utils to scan ancestors#2285
feat(iceberg): Add snapshot utils to scan ancestors#2285CTTY wants to merge 3 commits intoapache:mainfrom
Conversation
| } | ||
|
|
||
| /// Iterate starting from `snapshot` (inclusive) to the root snapshot. | ||
| pub fn ancestors_of( |
There was a problem hiding this comment.
Why not make this part of TableMetadata?
There was a problem hiding this comment.
I think putting this in TableMetadata is also fine. This code structure is to echo what we have in Java: https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java
| } | ||
|
|
||
| /// Iterate starting from `snapshot` (inclusive) to `oldest_snapshot_id` (exclusive). | ||
| pub fn ancestors_between( |
There was a problem hiding this comment.
It should be put into Ancestors struct.
There was a problem hiding this comment.
Could you help me understand what are the benefits of doing so?
I think Ancestors here are internal details to help us iterate thru snapshots. the helper functions can be the convenient entry points
|
|
||
| use crate::spec::{SnapshotRef, TableMetadataRef}; | ||
|
|
||
| struct Ancestors { |
There was a problem hiding this comment.
Since we may have more conditions to build iterator, it's more idiomatic to implement IntoIterator for Ancestors, rather than Iterator.
There was a problem hiding this comment.
I'm not sure I fully understand, do you mean something like this?
let iter = Ancestors::new()
.until(oldest_snapshot_id)
.into_iter();
2 participants
Which issue does this PR close?
What changes are included in this PR?
Ancestorsto help scan past snapshotsAre these changes tested?
Yes