[Improvement] Fix dataset leak on cache replacement in LancePartitionStatisticStorage
- Dominant language
- Java
- Stars
- 3.2k
- Forks
- 935
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 298
Description
### What would you like to be improved?
When datasetCacheSize > 0, appendStatisticsImpl commits and then does cache.put(tableId, newDataset). Replacing an existing cache value does not trigger the current close path, because the cache is wired with evictionListener, while replacement is a removal event. The old Dataset can remain unclosed, causing leaked native/file handles during repeated statistics updates. Default config (datasetCacheSize=0) avoids this path, but cached deployments are affected.
### How should we improve?
Ensure replaced cache entries are closed on any removal, not just eviction. Update cache wiring to close Dataset via a removal callback that runs for replacement/removal/eviction events. Keep invalidateAll() on shutdown so cached datasets are closed at storage close.
Here's a unit test to help:
```
@Test
public void testDatasetCacheReplacementClosesOldDataset() throws Exception {
String location = Files.createTempDirectory("lance_stats_cache_replace").toString();
Map properties = Maps.newHashMap();
properties.put("location", location);
properties.put("datasetCacheSize", "10");
LancePartitionStatisticStorage storage = new LancePartitionStatisticStorage(properties);
try {
Cache cache = storage.getDatasetCache();
Dataset oldDataset = mock(Dataset.class);
Dataset newDataset = mock(Dataset.class);
cache.put(1L, oldDataset);
cache.put(1L, newDataset);
verify(oldDataset, times(1)).close();
} finally {
FileUtils.deleteDirectory(new File(location));
storage.close();
}
}
```
Contributor guide
Assessment
This issue has not been assessed yet.