etseidl commented on code in PR #11297: URL: https://github.com/apache/arrow-rs/pull/11297#discussion_r4300210319
########## parquet/src/file/metadata/mod.rs: ########## @@ -116,15 +116,18 @@ pub(crate) use writer::ThriftMetadataWriter; /// This structure is read by the various readers in this crate or can be read /// directly from a file using the [`ParquetMetaDataReader`] struct. /// +/// The individual fields of this structure are stored in a way that make cloning +/// this structure low-cost. +/// /// See the [`ParquetMetaDataBuilder`] to create and modify this structure. /// /// [`parquet.thrift`]: https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift #[derive(Debug, Clone)] pub struct ParquetMetaData { /// File level metadata - file_metadata: FileMetaData, + file_metadata: Arc<FileMetaData>, /// Row group metadata - row_groups: Vec<RowGroupMetaData>, + row_groups: Arc<Vec<RowGroupMetaData>>, Review Comment: > I think we can make this Arc<Vec<Arc<RowGroupMetaData>>> in the future if that turns out to be a problem Yes. I'd like to eventually pull the column chunk metadata out of the row group and instead offer it up the same way as page indexes or bloom filters...they're all indexed by `(row_group_idx, column_idx)` anyway, why pay all the allocation pain for all of these vectors of metadata buried in vectors of structs? >Another minor thing here to save a few bytes (maybe 16 bytes per instance) is to instead of >``` > row_groups: Arc<Vec<RowGroupMetaData>>,` >``` > >remove the Vec part: >``` > row_groups: Arc<[RowGroupMetaData]>, >``` Can do, but that has a few implications for the `ParquetMetaDataBuilder`. In particular, `add_row_group` gets messy: ```rust pub fn add_row_group(mut self, row_group: RowGroupMetaData) -> Self { let mut v = self.0.row_groups.to_vec(); v.push(row_group); self.0.row_groups = v.into(); self } ``` (Claude suggested a way to sometimes avoid the clone, but it's still an ugly round trip). I could go back to the builder as a normal struct rather than a tuple struct so the `row_groups` is converted to a vector once when you create the builder, and then converted back to an `Arc` slice in `build`, but then that's a lot of copying going on if you don't want to update the row groups. (Another $.30 worth of tokens fixed that as well, but with an increase in builder complexity). I think I'd prefer to defer this change to a rewrite of `ParquetMetaData` (it's providers all the way down 🤣), but I'm happy to make the change if you still want to. Just let me know. -- 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]
