feat: extend table scan to support v2 deletes#489
Conversation
src/iceberg/table_scan.h
Outdated
| /// \param from_snapshot_id the start snapshot ID (inclusive) | ||
| /// \note InvalidArgument will be returned if the start snapshot is not an ancestor of | ||
| /// the end snapshot | ||
| TableScanBuilder& FromSnapshotInclusive(int64_t from_snapshot_id); |
There was a problem hiding this comment.
TableScanBuilder& FromSnapshot(int64_t from_snapshot_id, bool inclusive);
TableScanBuilder& FromSnapshot(const std::string& ref, bool inclusive);
How about merging these interfaces of FromSnapshot into this one? Can ref also be changed to tag? SnapshotRef might be Branch or Tag. Using ref here would be a bit strange
There was a problem hiding this comment.
It makes sense to combine them. I don't think the ref applies to tag.
| std::optional<int64_t> limit; | ||
| bool from_snapshot_id_inclusive{false}; | ||
| std::optional<int64_t> from_snapshot_id; | ||
| std::optional<int64_t> to_snapshot_id; |
There was a problem hiding this comment.
How about defining a VersionRange or DataRange to express a specific version or version range?
There was a problem hiding this comment.
I think that may be a little bit complicated and from/to do not need to appear at the same time.
There was a problem hiding this comment.
snapshot_id can also be expressed as [-1, snapshot_id], It can avoid passing multiple parameters when used internally.
There was a problem hiding this comment.
Let's revisit this once we add incremental scan support.
Co-authored-by: Guotao Yu <guotao.yugt@gmail.com>
Co-authored-by: Guotao Yu <guotao.yugt@gmail.com>
Co-authored-by: Zehua Zou <zehuazou2000@gmail.com>
|
Thanks all for the review! |
5 participants
No description provided.