-
Notifications
You must be signed in to change notification settings - Fork 215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add conversion from FileMetaData
to ParquetMetadata
#1074
base: main
Are you sure you want to change the base?
feat: Add conversion from FileMetaData
to ParquetMetadata
#1074
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jonathanc-n , LGTM! Just a minor concern about thrift depdenency, let's wait for @Xuanwo to take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most changes looks good to me. It's better if we can remove some unwarp()
.
min_max_agg.produce(), | ||
) | ||
}; | ||
let parquet_metadata = ParquetMetaDataReader::decode_metadata(&buffer).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about returning an error instead?
let metadata = writer.close().await.map_err(|err| { | ||
Error::new(ErrorKind::Unexpected, "Failed to close parquet writer.").with_source(err) | ||
})?; | ||
|
||
let written_size = self.written_size.load(std::sync::atomic::Ordering::Relaxed); | ||
|
||
Ok(vec![Self::to_data_file_builder( | ||
let parquet_metadata = Arc::new(self.thrift_to_parquet_metadata(metadata).unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same.
Which issue does this PR close?
What changes are included in this PR?
Add conversion from filemetadat to parquet metadata using thrift
decode_metadata
Are these changes tested?