feat: add cross-query metadata and snapshot caching - #875
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces cross-query reuse for immutable Iceberg metadata I/O by adding a bounded metadata-content cache in FileIO and persisting parsed snapshot manifest-list entries on Snapshot instances to avoid repeated parsing across scans.
Changes:
- Add
MetadataCache+MetadataCacheOptionsand integrate it intoFileIOviaReadFileCached()/NewCachedInputFile(), with Java-compatible property names and defaults. - Persist snapshot manifest parsing results via a
Snapshot-owned shared cache state and returnstd::span<const ManifestFile>fromSnapshotCache. - Wire cache enablement through catalogs/registry and add unit tests + build system registration.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/iceberg/update/expire_snapshots.cc | Adjust manifest collection to work with the new span<const ManifestFile> cache surface. |
| src/iceberg/test/metadata_cache_test.cc | Adds coverage for cache reuse, eviction, invalidation, and concurrency behavior. |
| src/iceberg/test/meson.build | Registers the new metadata cache test with Meson. |
| src/iceberg/test/CMakeLists.txt | Registers the new metadata cache test with CMake. |
| src/iceberg/table_metadata.cc | Switches metadata JSON reads to go through the cached read path. |
| src/iceberg/snapshot.h | Adds snapshot-shared cache state and tightens manifest spans to const. |
| src/iceberg/snapshot.cc | Implements snapshot-shared manifest-list parsing/cache coalescing. |
| src/iceberg/metadata_cache.h | Introduces the public cache/options API and property contract. |
| src/iceberg/metadata_cache.cc | Implements the bounded, expiring, coalescing metadata cache. |
| src/iceberg/meson.build | Adds metadata_cache.cc and installs metadata_cache.h. |
| src/iceberg/manifest/manifest_reader.cc | Enables metadata-content caching for manifests and manifest lists via reader options. |
| src/iceberg/file_reader.h | Adds ReaderOptions::cache_content to control metadata caching. |
| src/iceberg/file_io.h | Adds cache configuration and cached read/input APIs to FileIO. |
| src/iceberg/file_io.cc | Implements cached reads, cached input wrapping, and cache configuration plumbing. |
| src/iceberg/file_io_registry.cc | Configures metadata caching during registry-based FileIO creation. |
| src/iceberg/CMakeLists.txt | Adds metadata_cache.cc to the build. |
| src/iceberg/catalog/sql/sql_catalog.cc | Wires cache configuration through SQL catalog properties when enabled. |
| src/iceberg/catalog/memory/in_memory_catalog.cc | Wires cache configuration through in-memory catalog properties when enabled. |
| src/iceberg/avro/avro_reader.cc | Passes caching intent down to Arrow input opening. |
| src/iceberg/arrow/arrow_io.cc | Adds an opt-in cached-input path when cache is enabled + requested. |
| src/iceberg/arrow/arrow_io_internal.h | Extends Arrow input open helper signature to accept cache intent. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/iceberg/file_reader.h:104
ReaderOptionsnow exposescache_content, but not all reader implementations appear to honor it. For example,parquet_reader.ccstill callsarrow::OpenArrowInputStream(options.io, options.path, options.length)without passingoptions.cache_content, so enablingcache_contentwould have no effect for Parquet reads. If the intent is that this flag controls whether the underlyingFileIOusesNewCachedInputFile, it should be forwarded consistently by readers that open Arrow input streams.
/// \brief FileIO instance to open the file.
std::shared_ptr<class FileIO> io;
/// \brief Cache this immutable metadata file's content when FileIO caching is enabled.
bool cache_content = false;
/// \brief The projection schema to read from the file. This field is required.
std::shared_ptr<class Schema> projection;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/iceberg/metadata_cache.cc:168
- MetadataCacheOptions::expiration_interval_ms is only validated for non-negativity. Extremely large values can overflow/behave unexpectedly when converted to std::chrono::milliseconds in IsExpired(), leading to incorrect eviction behavior. Consider rejecting values larger than std::chrono::milliseconds::max().count().
ICEBERG_PRECHECK(options.expiration_interval_ms >= 0,
"Metadata cache expiration interval must not be negative: {}",
options.expiration_interval_ms);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/iceberg/snapshot.h:422
cache_datais eagerly allocated for everySnapshotinstance and the manifest-list parse results appear to be retained for the lifetime of theSnapshot(no bounding/eviction). For tables with many snapshots and large manifest lists, this can cause unbounded memory retention across queries. Consider lazy-initializing the cache state on first use and/or adding a bounded/clearable policy similar toMetadataCache.
/// Internal lazy state shared by Snapshot copies so manifest lists are parsed once.
mutable std::shared_ptr<internal::SnapshotCacheData> cache_data =
internal::MakeSnapshotCacheData();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/iceberg/metadata_cache.cc:45
- ParseBoolean only accepts exact "true"/"false" and rejects common case variants (e.g., "TRUE"), which can break configuration coming from Java-style properties sources. Since this is meant to be Java-compatible, parse booleans case-insensitively.
Result<bool> ParseBoolean(std::string_view key, const std::string& value) {
if (value == "true") {
return true;
}
if (value == "false") {
src/iceberg/file_io.cc:213
- NewCachedInputFile checks the cache using the caller-provided file_location, but CachedInputFile later uses input_file_->location() as the cache key. If an InputFile implementation normalizes/aliases locations, this inconsistency can cause missed cache hits and duplicate entries; use the InputFile's location consistently.
if (auto cached = cache->GetIfPresent(file_location)) {
return std::make_unique<CachedInputFile>(std::move(input_file), std::move(cache),
static_cast<int64_t>(cached->size()));
}
src/iceberg/file_io.cc:281
- ClearMetadataCache currently clears only the byte-content MetadataCache, but it does not reset the per-FileIO SnapshotCacheData (parsed manifest-list entries). That means manifest-list parsing cache can grow without any clear/invalidation path even when callers explicitly clear metadata caches.
void FileIO::ClearMetadataCache() {
auto cache = GetMetadataCache();
if (cache != nullptr) {
cache->Clear();
}
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
d70ca60 to
433113f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
src/iceberg/file_io.cc:210
NewCachedInputFilechecks the cache usingfile_location, butCachedInputFilelater usesinput_file_->location()(location_) as the cache key. If aFileIOnormalizes/rewrites locations (e.g., resolves paths, strips schemes, redirects), this can cause systematic cache misses and duplicate cache entries. Consider usinginput_file->location()consistently for (1) theGetIfPresentlookup and (2) the location passed into cached loads, while still preserving the originalfile_locationfor error messages if desired.
Result<std::unique_ptr<InputFile>> FileIO::NewCachedInputFile(
std::string file_location, std::optional<size_t> length) {
std::unique_ptr<InputFile> input_file;
if (length.has_value()) {
ICEBERG_ASSIGN_OR_RAISE(input_file, NewInputFile(file_location, *length));
} else {
ICEBERG_ASSIGN_OR_RAISE(input_file, NewInputFile(file_location));
}
auto cache = GetMetadataCache();
if (cache == nullptr || !cache->options().enabled) {
return input_file;
}
if (auto cached = cache->GetIfPresent(file_location)) {
return std::make_unique<CachedInputFile>(std::move(input_file), std::move(cache),
static_cast<int64_t>(cached->size()));
}
src/iceberg/metadata_cache.cc:145
- The eviction condition uses
options_.max_total_bytes - total_bytes_onsize_t, which can underflow iftotal_bytes_ever exceedsmax_total_bytes(even transiently due to future changes). A safer, more self-documenting check iswhile (!lru_.empty() && (incoming_bytes + total_bytes_ > options_.max_total_bytes))(with overflow-safe ordering), which avoids unsigned underflow and makes the invariant explicit.
void EvictToFit(size_t incoming_bytes) {
while (!lru_.empty() && incoming_bytes > options_.max_total_bytes - total_bytes_) {
auto it = entries_.find(lru_.back());
if (it == entries_.end()) {
lru_.pop_back();
} else {
EraseLoaded(it);
}
}
}
src/iceberg/arrow/arrow_io.cc:535
- In this function, the
if (cache_content)branch is reached only whencache_contentis true andio->MetadataCacheEnabled()is true (due to the earlier guard). Theelse if (length.has_value()) { ... } else { ... }branch is therefore dead code in this path and can be simplified to reduce cognitive overhead (e.g., remove theelsecases here and keep the non-cached path entirely in the earlier branch).
int64_t size;
std::unique_ptr<InputFile> input_file;
if (cache_content) {
ICEBERG_ASSIGN_OR_RAISE(input_file, io->NewCachedInputFile(path, length));
} else if (length.has_value()) {
ICEBERG_ASSIGN_OR_RAISE(input_file, io->NewInputFile(path, *length));
} else {
ICEBERG_ASSIGN_OR_RAISE(input_file, io->NewInputFile(path));
Co-authored-by: Codex <codex@openai.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/iceberg/file_io.cc:230
NewCachedInputFileonly skips wrapping whensize > max_content_length, but a file can also be larger thanmax_total_bytes. In that caseCachedInputFile::Open()will still read the entire file into memory viaMetadataCache::Get()(and then drop it from the cache), which can cause large transient allocations and defeats the intent ofmax_total_bytesas a bound. Consider also bypassing the cached wrapper whensize > max_total_bytes.
if (std::cmp_greater(size, cache->options().max_content_length)) {
return input_file;
}
return std::make_unique<CachedInputFile>(std::move(input_file), std::move(cache), size);
Summary
Why
Repeated table loads and scans currently reread immutable Iceberg metadata files and reparse snapshot manifest lists. This adds cross-query reuse while preserving normal data-file I/O paths.