refactor: Add read_from() and write_to() to TableMetadata#1523
refactor: Add read_from() and write_to() to TableMetadata#1523liurenjie1024 merged 12 commits intoapache:mainfrom
read_from() and write_to() to TableMetadata#1523Conversation
CTTY
changed the title
TableMetadataIO to read and write metadataread() and write() to TableMetadata
crates/catalog/glue/src/catalog.rs
Outdated
| .new_output(&metadata_location)? | ||
| .write(serde_json::to_vec(&metadata)?.into()) | ||
| .await?; | ||
| TableMetadata::write(&self.file_io, &metadata, &metadata_location).await?; |
There was a problem hiding this comment.
I also found MetadataLocation proposed here quite interesting:
Would love to hear more thoughts about it
There was a problem hiding this comment.
I think this should not change the interface here. We could change the write interface to sth like following:
fn write(&self, impl ToString)to generalize the arguments.
There was a problem hiding this comment.
I changed it to
fn write(&self, impl AsRef<str>)
because file_io.new_input/new_output takes AsRef<str>
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr, generally LGTM! Just one minor nit!
crates/catalog/glue/src/catalog.rs
Outdated
| .new_output(&metadata_location)? | ||
| .write(serde_json::to_vec(&metadata)?.into()) | ||
| .await?; | ||
| TableMetadata::write(&self.file_io, &metadata, &metadata_location).await?; |
There was a problem hiding this comment.
I think this should not change the interface here. We could change the write interface to sth like following:
fn write(&self, impl ToString)to generalize the arguments.
CTTY
changed the title
read() and write() to TableMetadataread_from() and write_to() to TableMetadata
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr, LGTM!
2 participants
Which issue does this PR close?
TableMetadatato new location. #1388What changes are included in this PR?
TableMetadataIOto read/write metadata from/to a locationAre these changes tested?
Added unit test