BlakeOrth commented on code in PR #18855:
URL: https://github.com/apache/datafusion/pull/18855#discussion_r2550800835
##########
datafusion/execution/src/cache/cache_unit.rs:
##########
@@ -111,64 +113,224 @@ impl CacheAccessor<Path, Arc<Statistics>> for
DefaultFileStatisticsCache {
///
/// Collected files metadata for listing files.
///
-/// Cache is not invalided until user calls [`Self::remove`] or
[`Self::clear`].
+/// # Internal details
+///
+/// The `capacity` parameter controls the maximum number of entries in the
cache, using a Least
+/// Recently Used eviction algorithm. When adding a new entry, if the total
number of entries in
+/// the cache exceeds `capacity`, the least recently used entries are evicted
until the total
+/// entries are lower than the `capacity`.
///
/// [`ListFilesCache`]: crate::cache::cache_manager::ListFilesCache
#[derive(Default)]
pub struct DefaultListFilesCache {
- statistics: DashMap<Path, Arc<Vec<ObjectMeta>>>,
+ state: Mutex<DefaultListFilesCacheState>,
+}
+
+impl DefaultListFilesCache {
+ pub fn new(capacity: usize, ttl: Duration) -> Self {
+ Self {
+ state: Mutex::new(DefaultListFilesCacheState::new(capacity, ttl)),
+ }
+ }
+
+ pub fn cache_limit(&self) -> usize {
+ self.state.lock().unwrap().capacity
+ }
+
+ pub fn cache_ttl(&self) -> Duration {
+ self.state.lock().unwrap().ttl
+ }
+}
+
+pub(super) const DEFAULT_LIST_FILES_CACHE_LIMIT: usize = 128 * 1024; // ~130k
objects
+pub(super) const DEFAULT_LIST_FILES_CACHE_TTL: Duration = Duration::new(600,
0); // 10min
+
+pub struct DefaultListFilesCacheState {
+ lru_queue: LruQueue<Path, (Arc<Vec<ObjectMeta>>, Instant)>,
+ capacity: usize, // TODO: do "bytes" matter here, or should we stick with
"entries"?
+ size: usize,
+ ttl: Duration,
+}
+
+// TODO: Do we even want to support "default" here?
+impl Default for DefaultListFilesCacheState {
+ fn default() -> Self {
+ Self {
+ lru_queue: LruQueue::new(),
+ capacity: DEFAULT_LIST_FILES_CACHE_LIMIT,
+ size: 0,
+ ttl: DEFAULT_LIST_FILES_CACHE_TTL,
+ }
+ }
+}
+
+impl DefaultListFilesCacheState {
+ fn new(capacity: usize, ttl: Duration) -> Self {
+ Self {
+ lru_queue: LruQueue::new(),
+ capacity,
+ size: 0,
+ ttl,
+ }
+ }
+
+ fn get(&mut self, key: &Path) -> Option<Arc<Vec<ObjectMeta>>> {
Review Comment:
Ah, thanks for pointing this out. I ran into and resolved one merge conflict
related to that PR and didn't realize it may have had a subtly wider impact
than just the method that caused a conflict on my rebase. I will make sure to
revisit this.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
