Skip to content

Add limit to DefaultFileStatisticsCache #19052

Closed

Description

@alamb

Also listing file statistics cache seems to not have any memory limit unlike metadata cache for example.

Is that by design , do you think we need to add similar limit for this cache too ?

Originally posted by @bharath-techie in #18971 (comment)

Basically the cache used in ListingTable comes from here:
https://github.com/apache/datafusion/blob/81512da2b0aaa474f6c4ba205b05eea7b3095176/datafusion/core/src/datasource/listing_table_factory.rs#L188-L187

Which somewhat unobviously sets a DefaultFileStatisticsCache here
https://github.com/apache/datafusion/blob/9f725d9c7064813cda0de0f87d115354b68d76e6/datafusion/catalog-listing/src/table.rs#L260-L259

The DefaultFileStatisticsCache has no limit:
https://github.com/apache/datafusion/blob/7d8b8602ad1be2f61f6a8ebb253ace9d85304ea7/datafusion/execution/src/cache/cache_unit.rs#L41-L40

Activity

  1. alamb commented on Dec 2, 2025

    @alamb
    ContributorAuthor

    My suggestion would be to implement @nuno-faria 's ideas here: #18953 (comment)

    And then once we had a real trait, add the ability to track memory used by Statistics and then use that measurement to implement a size limited cache

    This would be similar in design to @BlakeOrth 's PR here

    I suggest a sequence of smaller PRs:

    1. Add a method to statistics like Statistics::heap_size that reports the heap (allocation) size for statistics -- following the semantics of https://github.com/apache/arrow-rs/blob/b93fa52e47a83dff130709a6e80a4be5017dbf09/parquet/src/file/metadata/memory.rs#L35
    2. Update the DefaultFileStatisticsCache to have a memory limit and use the heap_size from 1 to constrain the size
    3. Add a config parameter , perhaps datafusion.runtime.statistics_cache_limit, similar to datafusion.runtime.metadata_cache_limit, and set the default statistics cache in
      let file_statistic_cache =
      config.table_files_statistics_cache.as_ref().map(Arc::clone);
      (following the example of DefaultFilesMetadataCache)
  2. alamb commented on Dec 2, 2025

    @alamb
    ContributorAuthor

    It turns out @alchemist51 also pointed this out in

    I didn't understand at first what was meant

    A little bit side topic. @alamb was looking into other caches, didn't understand why DefaultFileStatisticsCache isn't memory bound yet. Do we want to do it incrementally?

  3. jizezhang commented on Dec 9, 2025

    @jizezhang
    Contributor

    take

  4. mkleen commented on Dec 9, 2025

    @mkleen
    Contributor

    Dear @jizezhang I am already working on #19217, and this ticket is basically a requirement for it. I was wondering if it would be ok with you if I take it over. Let me know what you think.

  5. mkleen commented on Dec 9, 2025

    @mkleen
    Contributor

    However, I guess it's ok, we can merge it all together.

  6. jizezhang commented on Dec 9, 2025

    @jizezhang
    Contributor

    Hey @mkleen feel free to take it if that makes things easier.

  7. jizezhang commented on Dec 10, 2025

    @jizezhang
    Contributor

    @mkleen Could you try take this issue if you still plan to work on it? It looks like I cannot unassign myself...

  8. mkleen commented on Dec 10, 2025

    @mkleen
    Contributor

    take

  9. mkleen commented on Dec 10, 2025

    @mkleen
    Contributor

    Thank you @jizezhang

  10. alamb commented on Dec 31, 2025

    @alamb
    ContributorAuthor

    @mkleen and @jizezhang is there any update on this issue?

  11. mkleen commented on Dec 31, 2025

    @mkleen
    Contributor

    @mkleen and @jizezhang is there any update on this issue?

    Yes, I am working on it as part of #19217. We would like to have this for the next release, right?

  12. alamb commented on Jan 1, 2026

    @alamb
    ContributorAuthor

    @mkleen and @jizezhang is there any update on this issue?

    Yes, I am working on it as part of #19217. We would like to have this for the next release, right?

    Yes, please, let's try and get it done for 52.0.0 if possible (we are starting to test next week I believe)

    If you can break the work into a few smaller PRs I will be more likely to be able to help review it quickly

  13. abhita commented on Jan 27, 2026

    @abhita

    @mkleen @alamb
    As the plan here is to enforce limit on DefaultFileStatisticsCache , are we also planning on any mechanisms to handle stale entries here? perhaps through eviction or TTL?

  14. mkleen commented on Jan 28, 2026

    @mkleen
    Contributor

    @mkleen @alamb As the plan here is to enforce limit on DefaultFileStatisticsCache , are we also planning on any mechanisms to handle stale entries here? perhaps through eviction or TTL?

    @abhita Currently it's handled by eviction see #20047 for details.

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions