alamb commented on code in PR #11297:
URL: https://github.com/apache/arrow-rs/pull/11297#discussion_r4158903674
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -333,28 +329,57 @@ impl PartialEq for ParquetMetaData {
/// .add_row_group(last_row_group)
/// .build();
/// ```
-pub struct ParquetMetaDataBuilder(ParquetMetaData);
+pub struct ParquetMetaDataBuilder {
+ /// File level metadata
+ file_metadata: Arc<FileMetaData>,
Review Comment:
As in this would be `file_metadata: FileMetadata`
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -333,28 +329,57 @@ impl PartialEq for ParquetMetaData {
/// .add_row_group(last_row_group)
/// .build();
/// ```
-pub struct ParquetMetaDataBuilder(ParquetMetaData);
+pub struct ParquetMetaDataBuilder {
+ /// File level metadata
+ file_metadata: Arc<FileMetaData>,
Review Comment:
the builder maybe would keep the field un_arc'd until construction 🤔
##########
parquet/src/file/metadata/thrift/encryption.rs:
##########
@@ -296,12 +296,16 @@ pub(crate) fn parquet_metadata_with_encryption(
.map_err(|e| general_err!("Could not parse metadata: {}", e))?;
let ParquetMetaData {
- mut file_metadata,
+ file_metadata,
row_groups,
page_index: _,
file_decryptor: _,
} = parquet_meta;
+ // this is called right after creating parquet_meta, so there should be no
other references
Review Comment:
I wonder if we could use something like a `ParquetMetadataBuilder` here --
like
```rust
let metadata = parquet_meta
.into_builder()
.decrypt(file_decryption_properties)
.build()
```
(somewhat unrelated to this PR, just trying to imagine we could encapsulate
the Arc::unwrap_or_clone stuff
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -333,28 +329,57 @@ impl PartialEq for ParquetMetaData {
/// .add_row_group(last_row_group)
/// .build();
/// ```
-pub struct ParquetMetaDataBuilder(ParquetMetaData);
+pub struct ParquetMetaDataBuilder {
+ /// File level metadata
+ file_metadata: Arc<FileMetaData>,
Review Comment:
But that is something we could revisit
##########
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 mean we made the page_index an Arc, now we make the file and row group
metadata Arcs too which seems reasonable.
However, it seems to me like one of the main usecases / things that is hard
to do now, is to fetch only metadata for some row groups (rather than a copy of
the entire thing)
--
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]
