ParquetMetaData memory size is not reported accurately when `encryption` is enabled

[alamb]

Describe the bug
While working on #8470 I noticed that the API to report memory usage when encryption was used undercounts the actual memory used

ParquetMetaData::memory_size is used for memory accounting for in memory parquet caches, and thus should be accurate

To Reproduce
Specifically this function

arrow-rs/parquet/src/file/metadata/mod.rs

Lines 281 to 286 in b8ae8e0

	pub fn memory_size(&self) -> usize {
	std::mem::size_of::<Self>()
	+ self.file_metadata.heap_size()
	+ self.row_groups.heap_size()
	+ self.column_index.heap_size()
	+ self.offset_index.heap_size()

Does not account for the heap allocations in the file_decryptor field:

arrow-rs/parquet/src/file/metadata/mod.rs

Line 191 in b8ae8e0

file_decryptor: Option<FileDecryptor>,

Expected behavior
ParquetMetaData::memory_size should report its actually heap allocation size (by implementing the HeapSize trait for FileDecryptor and all its subfields

Additional context

[etseidl]

So I've tried running down this rabbit hole, and hit two roadblocks. One is

arrow-rs/parquet/src/encryption/ciphers.rs

Lines 37 to 39 in 209eafa

	pub(crate) struct RingGcmBlockDecryptor {
	key: LessSafeKey,
	}

I have no idea how to handle the LessSafeKey.

Similarly

arrow-rs/parquet/src/encryption/decrypt.rs

Lines 275 to 278 in 209eafa

	enum DecryptionKeys {
	Explicit(ExplicitDecryptionKeys),
	ViaRetriever(Arc<dyn KeyRetriever>),
	}

How to get the size of something a user might pass in. HeapSize is private, so we can't add it to the bounds on KeyRetriever, and it seems weird to add heap_size to the trait.

@rok or @adamreeve any ideas?

[adamreeve]

I have no idea how to handle the LessSafeKey.

I got a bit lost trying to understand what's actually stored in there too. If it's important to have the memory usage be accurate, we could change FileDecryptor to only store the key bytes and create the RingGcmBlockDecryptor on demand? We might just want to check the performance overhead on a file that uses uniform encryption, but I think it should be negligible.

Handling the KeyRetriever seems tricky. It's possible that the same retriever could be shared across multiple files, so even if we knew the size of the KeyRetriever it might not be correct to add it to the memory used by each file. Although at least in our parquet-key-management crate we do currently create a new KeyRetriever instance per file, and this holds an Arc to some state that's shared between retriever instances.

Maybe it's fine to add a new heap_size method to the trait and have a default implementation that returns 0, and document that if a user creates a retriever per file and needs accurate memory accounting, they should implement this?

[alamb]

I think being as accurate as we can be (even if there is known missing accounting, like for KeyRetriever) would be an improvement, even if it isn't entirely accurate

Maybe we can leave a comment or something with a caveat about its accuracy

[adamreeve]

OK I can have a go at adding what we can account for, unless you've already made a start on this @etseidl?

[etseidl]

Thanks @adamreeve. If it will help I can point you to the branch I started.

[adamreeve]

Sure, that would be useful thanks!

[etseidl]

https://github.com/etseidl/arrow-rs/tree/parquet_meta_heapsize

I wasn't quite sure how to account for the HashMap either...that's likely an underestimate as well.