[I] Support reporting heap size of custom `PageIndexProvider` [arrow-rs]

["alamb (via GitHub)" <gi...@apache.org>]

alamb opened a new issue, #11028:
URL: https://github.com/apache/arrow-rs/issues/11028

   **Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
   
   - Follow on to https://github.com/apache/arrow-rs/pull/10842 from @etseidl
   
   We added the [`PageIndexProvider`](https://github.com/apache/arrow-rs/blob/79dbf5a3524cb8077d58eaec21c17f97de4ba4ca/parquet/src/file/metadata/page_index.rs#L139) trait so users can supply custom page index representations. However there is no way for `PageIndexProvider` to report their memory usage in [`ParquetMetaData::memory_size`](https://github.com/apache/arrow-rs/blob/79dbf5a3524cb8077d58eaec21c17f97de4ba4ca/parquet/src/file/metadata/mod.rs#L231)
   
   Since one of the main motivations for the provider trait is more efficient caching of parquet metadata, callers need accurate memory accounting for custom providers as well.
   
   See here for some prior attempts at exposing `HeapSize` publicly:
   - https://github.com/apache/arrow-rs/pull/9138
   - https://github.com/apache/arrow-rs/pull/9842
   
   **Describe the solution you'd like**
   
   Add a way for the `PageIndexProvider` trait to report its heap size so `ParquetMetaData::memory_size` can include it. Ideally implementors could re-use the [`HeapSize`](https://github.com/apache/arrow-rs/blob/79dbf5a3524cb8077d58eaec21c17f97de4ba4ca/parquet/src/file/metadata/memory.rs#L36) machinery the parquet crate already implements.
   
   **Describe alternatives you've considered**
   
   - Add a required (or defaulted-to-zero) `heap_size(&self) -> usize` method directly on `PageIndexProvider`, without exposing the `HeapSize` trait publicly.
   - Make the `HeapSize` trait public (per the prior attempts above) and require `PageIndexProvider: HeapSize`.
   
   **Additional context**
   
   - Original review discussion: https://github.com/apache/arrow-rs/pull/10842#discussion_r3913858985
   


-- 
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: github-unsubscribe@arrow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Support reporting heap size of custom `PageIndexProvider` [arrow-rs]

["bharadwaj-pendyala (via GitHub)" <gi...@apache.org>]

bharadwaj-pendyala commented on issue #11028:
URL: https://github.com/apache/arrow-rs/issues/11028#issuecomment-5619939838

   I'd land Option A on its own. `PageIndexProvider` already defaults `is_complete`, so a defaulted `heap_size(&self) -> usize { 0 }` is additive for existing implementors. `memory_size` can replace `downcast_ref::<PageIndex>()` at `parquet/src/file/metadata/mod.rs:242` with the trait call. Built-in `PageIndex` would report the same number using its existing private `HeapSize` impl.
   
   Both prior attempts died on scope rather than merit: #9138 was a 2k-line new crate that never got reviewed, and #9842 stalled on where the trait should live. Public `HeapSize` still seems worth doing, but it doesn't have to gate accounting for custom providers. Does leaving public `HeapSize` for a follow-up fit what you had in mind?
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Support reporting heap size of custom `PageIndexProvider` [arrow-rs]

["etseidl (via GitHub)" <gi...@apache.org>]

etseidl commented on issue #11028:
URL: https://github.com/apache/arrow-rs/issues/11028#issuecomment-5629131201

   The issue is that calculating the heap size is quite complex and easy to do wrong. We have a number of implementations of `HeapSize` in the parquet crate for various containers the would be of great use to authors of custom providers. How to get them access to these is the real issue. Forcing each provider implementation to reinvent this wheel is not tenable. So I'm personally not a fan of simply adding `heap_size` to the provider trait.
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org