blackmwk commented on code in PR #2541:
URL: https://github.com/apache/iceberg-rust/pull/2541#discussion_r3332213052
##########
crates/iceberg/src/spec/manifest_list.rs:
##########
@@ -90,6 +90,41 @@ impl ManifestList {
}
}
+/// A manifest list reader that encapsulates the logic for loading and parsing
a [`ManifestList`]
+/// from a snapshot.
+pub struct ManifestListReader<'a> {
Review Comment:
I'm hestating add lifetime mark here, `FileIO` is clonable, and we have
`SnapshotRef`, `TableMetadataRef`.
##########
crates/iceberg/src/catalog/utils.rs:
##########
@@ -47,7 +47,9 @@ pub async fn drop_table_data(
// Load all manifest lists concurrently
let results: Vec<_> =
futures::future::try_join_all(metadata.snapshots().map(|snapshot|
async {
- let manifest_list = snapshot.load_manifest_list(io,
metadata).await?;
+ let manifest_list = ManifestListReader::new(snapshot, io, metadata)
Review Comment:
Why not use `table.manifest_list_reader` method? This problem with `new`
method is that we will need to do a lot of changes when adding a new parameter.
##########
crates/iceberg/src/io/object_cache.rs:
##########
@@ -126,8 +127,8 @@ impl ObjectCache {
table_metadata: &TableMetadataRef,
) -> Result<Arc<ManifestList>> {
if self.cache_disabled {
- return snapshot
- .load_manifest_list(&self.file_io, table_metadata)
+ return ManifestListReader::new(snapshot, &self.file_io,
table_metadata)
Review Comment:
As above.
##########
crates/iceberg/src/spec/manifest_list.rs:
##########
@@ -90,6 +90,41 @@ impl ManifestList {
}
}
+/// A manifest list reader that encapsulates the logic for loading and parsing
a [`ManifestList`]
+/// from a snapshot.
+pub struct ManifestListReader<'a> {
+ snapshot: &'a Snapshot,
+ file_io: &'a FileIO,
+ table_metadata: &'a TableMetadata,
+}
+
+impl<'a> ManifestListReader<'a> {
+ pub(crate) fn new(
Review Comment:
Maybe we can limit the visitilibyt to table module?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
