Skip to content

ParquetMetaData memory size is not reported accurately when encryption is enabled #8472

Closed

Description

@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

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:

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

Activity

  1. etseidl commented on Oct 9, 2025

    @etseidl
    Contributor

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

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

    I have no idea how to handle the LessSafeKey.

    Similarly

    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?

  2. adamreeve commented on Oct 10, 2025

    @adamreeve
    Contributor

    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?

  3. alamb commented on Oct 16, 2025

    @alamb
    ContributorAuthor

    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

  4. adamreeve commented on Oct 17, 2025

    @adamreeve
    Contributor

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

  5. etseidl commented on Oct 17, 2025

    @etseidl
    Contributor

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

  6. adamreeve commented on Oct 17, 2025

    @adamreeve
    Contributor

    Sure, that would be useful thanks!

  7. etseidl commented on Oct 17, 2025

    @etseidl
    Contributor

    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.

  8. self-assigned this
    on Oct 19, 2025
  9. added a commit that references this issue on Oct 27, 2025
    06c49db
  10. alamb commented on Nov 19, 2025

    @alamb
    ContributorAuthor

    label_issue.py automatically added labels {'parquet'} from #8671

  11. alamb commented on Nov 19, 2025

    @alamb
    ContributorAuthor

    label_issue.py automatically added labels {'performance'} from #8574

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions