chore(refactor): add manifest list reader#2541
Conversation
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey for this pr!
| 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) |
There was a problem hiding this 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.
There was a problem hiding this comment.
We don't have access to a table directly in the catalog utils, do you think we should do a broader refactor here?
There was a problem hiding this comment.
No, we can, you can take a look at the call site of this method.
There was a problem hiding this comment.
Yes, and we should make this method crate private, it should not be called by user directly.
| } | ||
|
|
||
| impl<'a> ManifestListReader<'a> { | ||
| pub(crate) fn new( |
There was a problem hiding this comment.
Maybe we can limit the visitilibyt to table module?
xanderbailey
force-pushed
the
xb/manifest-list-reader
branch
from
f0d5779 to
7cb919b
Compare
xanderbailey
force-pushed
the
xb/manifest-list-reader
branch
2 times, most recently
from
ac55903 to
0449759
Compare
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey for this pr, please help to resolve the conflicts.
| metadata: &TableMetadata, | ||
| metadata_location: Option<&str>, | ||
| ) -> Result<()> { | ||
| pub async fn drop_table_data(table_info: Table) -> Result<()> { |
There was a problem hiding this comment.
| pub async fn drop_table_data(table_info: Table) -> Result<()> { | |
| pub(crate) async fn drop_table_data(table_info: &Table) -> Result<()> { |
We should mark this crate private and accepts table ref only.
There was a problem hiding this comment.
It's called from external creates (glue, hms, sql) so I'm not sure we can make it crate pub can we?
xanderbailey
force-pushed
the
xb/manifest-list-reader
branch
from
0449759 to
0df26ed
Compare
2 participants
Which issue does this PR close?
Refactor required for PR which adds a
ManifestListReaderwhich mirrors the currentManifestListWriter. This will allow us to carry the encryption manager here also in the linked PRWhat changes are included in this PR?
Adds a new public struct
ManifestListReaderwhich carries the things required to read a manifest list. Refactors call-sites to use this new struct and adds a deprecation for the old method.Are these changes tested?