alamb commented on code in PR #8574:
URL: https://github.com/apache/arrow-rs/pull/8574#discussion_r2417941727
##########
parquet/src/file/metadata/memory.rs:
##########
@@ -109,6 +122,7 @@ impl HeapSize for ColumnChunkMetaData {
+ self.unencoded_byte_array_data_bytes.heap_size()
+ self.repetition_level_histogram.heap_size()
+ self.definition_level_histogram.heap_size()
+ + self.geo_statistics.heap_size()
Review Comment:
💯
##########
parquet/src/file/metadata/memory.rs:
##########
@@ -70,10 +76,17 @@ impl HeapSize for String {
impl HeapSize for FileMetaData {
fn heap_size(&self) -> usize {
+ #[cfg(feature = "encryption")]
Review Comment:
- Does this change also fix https://github.com/apache/arrow-rs/issues/8472 ?
##########
parquet/tests/arrow_reader/bad_data.rs:
##########
@@ -80,12 +80,6 @@ fn test_invalid_files() {
#[test]
fn test_parquet_1481() {
let err = read_file("PARQUET-1481.parquet").unwrap_err();
- #[cfg(feature = "encryption")]
Review Comment:
Why did this test change?
##########
parquet/src/file/metadata/memory.rs:
##########
@@ -56,6 +56,12 @@ impl<T: HeapSize> HeapSize for Arc<T> {
}
}
+impl<T: HeapSize> HeapSize for Box<T> {
+ fn heap_size(&self) -> usize {
+ self.as_ref().heap_size()
+ }
+}
Review Comment:
Per the description of
https://github.com/apache/arrow-rs/blob/fe9a92fffe27866f46a546c521b1d14f6c66335b/parquet/src/file/metadata/memory.rs#L35-L39
So in that case I do think the size of `T` should be included. The size of
`self` (the pointer/Box) should not be included, but the memory it points to
should be
--
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]
