feat: cache immutable metadata content in FileIO by manuzhang · Pull Request #875 · apache/iceberg-cpp

manuzhang · 2026-08-06T08:15:20Z

Summary

Add a shared, bounded FileIO content cache for immutable manifest lists and manifests.
Keep table metadata JSON and data-file reads on their existing direct I/O paths.
Configure caching with Iceberg's io.manifest.cache-* property names and defaults across registry-created and catalog-injected FileIO instances.
Cover cache reuse, eviction, concurrency, normalized locations, invalidation, boolean parsing, and Arrow input-resource lifetimes with regression tests.

Why

Repeated scans reread the same immutable manifest lists and manifests. Sharing a bounded byte cache on FileIO avoids those reads across scans while allowing individual files or the whole cache to be invalidated.

Cache property names and defaults match Iceberg Java, including case-insensitive boolean parsing. Registry-created FileIO instances and REST resolvers configure caching from their properties; SQL and in-memory catalogs configure injected FileIO instances during catalog creation. The cache is owned by FileIO and remains disabled unless io.manifest.cache-enabled is set to true.

Follow-up

Parsed manifest-list reuse is tracked separately in #948.

Validation

Core, REST catalog, and SQL catalog static builds in default and ICEBERG_CXX20_COMPAT=ON modes.
68 focused cache, FileIO, ResolvingFileIO, REST FileIO, and Arrow IO tests passed in each mode.
The seven new regressions failed against the previous implementation before passing with these fixes.
C++20 public API consumer test passed with core, REST, and SQL public headers.
Avro reader syntax checks passed in both API modes.
pre-commit run --show-diff-on-failure --from-ref upstream/main --to-ref HEAD and git diff --check passed.

Focused Arrow IO tests use locally installed Arrow 24. Full bundled Arrow 25 integration tests were not run locally.

Copilot

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 + MetadataCacheOptions and integrate it into FileIO via ReadFileCached() / NewCachedInputFile(), with Java-compatible property names and defaults.
Persist snapshot manifest parsing results via a Snapshot-owned shared cache state and return std::span<const ManifestFile> from SnapshotCache.
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.

Copilot

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

ReaderOptions now exposes cache_content, but not all reader implementations appear to honor it. For example, parquet_reader.cc still calls arrow::OpenArrowInputStream(options.io, options.path, options.length) without passing options.cache_content, so enabling cache_content would have no effect for Parquet reads. If the intent is that this flag controls whether the underlying FileIO uses NewCachedInputFile, 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;

Copilot

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);

Copilot

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_data is eagerly allocated for every Snapshot instance and the manifest-list parse results appear to be retained for the lifetime of the Snapshot (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 to MetadataCache.

  /// Internal lazy state shared by Snapshot copies so manifest lists are parsed once.
  mutable std::shared_ptr<internal::SnapshotCacheData> cache_data =
      internal::MakeSnapshotCacheData();

Copilot

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();
  }

Copilot

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

NewCachedInputFile checks the cache using file_location, but CachedInputFile later uses input_file_->location() (location_) as the cache key. If a FileIO normalizes/rewrites locations (e.g., resolves paths, strips schemes, redirects), this can cause systematic cache misses and duplicate cache entries. Consider using input_file->location() consistently for (1) the GetIfPresent lookup and (2) the location passed into cached loads, while still preserving the original file_location for 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_ on size_t, which can underflow if total_bytes_ ever exceeds max_total_bytes (even transiently due to future changes). A safer, more self-documenting check is while (!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 when cache_content is true and io->MetadataCacheEnabled() is true (due to the earlier guard). The else if (length.has_value()) { ... } else { ... } branch is therefore dead code in this path and can be simplified to reduce cognitive overhead (e.g., remove the else cases 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));

Copilot

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

NewCachedInputFile only skips wrapping when size > max_content_length, but a file can also be larger than max_total_bytes. In that case CachedInputFile::Open() will still read the entire file into memory via MetadataCache::Get() (and then drop it from the cache), which can cause large transient allocations and defeats the intent of max_total_bytes as a bound. Consider also bypassing the cached wrapper when size > 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);

github-actions · 2026-09-17T00:46:32Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

wgtmac · 2026-09-17T02:10:38Z

Copilot review overview

3 open issues

NewCachedInputFile checks the cache using file_location, but CachedInputFile later uses… · New
Size() now opens and retains a RandomAccessFile in pending_input_ until the next… · New
PR description says the cache covers “metadata JSON, manifest lists, and manifests”, but the… · New

_{Review effort: Lite}

+  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()));
+  }


    ICEBERG_ARROW_ASSIGN_OR_RETURN(auto input, fs_->OpenInputFile(file_info));
    ICEBERG_ARROW_ASSIGN_OR_RETURN(auto size, input->GetSize());
+    file_size_ = size;
+    pending_input_ = std::move(input);


+  /// \brief Create an input handle backed by the metadata content cache when enabled.
+  ///
+  /// This is intended for immutable Iceberg manifest lists and manifests. Data files
+  /// and table metadata JSON should use NewInputFile or ReadFile directly.


Use Iceberg's unexpected wrapper for cache errors and test doubles so compatibility builds do not depend on std::unexpected. Co-authored-by: Codex <codex@openai.com>

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open issues

NewCachedInputFile checks the cache using file_location, but CachedInputFile later uses…
This change holds an open RandomAccessFile in pending_input_ after Size() is called. If… · New
The PR description says metadata JSON is included in the cache scope, but this doc comment… · New
The PR description states these properties are Java-compatible, but ParseBoolean only accepts… · New
PR description says the cache covers “metadata JSON, manifest lists, and manifests”, but the…
Size() now opens and retains a RandomAccessFile in pending_input_ until the next…

_{Review effort: Lite}

Use InputFile locations for cache lookup and invalidation, clearing the cache conservatively if a location can no longer be resolved. Close Arrow inputs after reading their sizes and reuse the existing Java-style boolean parser for cache properties. Add regression coverage for normalized locations, invalidation, input lifetimes, and case-insensitive boolean configuration. Co-authored-by: Codex <codex@openai.com>

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

8 open issues

NewCachedInputFile checks the cache using file_location, but CachedInputFile later uses…
cached->size() is a size_t that is narrowed to int64_t without bounds checking. If a cached… · New
The PR description states the cache is for “metadata JSON, manifest lists, and manifests”, but this… · New
The PR description states these properties are Java-compatible, but ParseBoolean only accepts…
The PR description says metadata JSON is included in the cache scope, but this doc comment…
This change holds an open RandomAccessFile in pending_input_ after Size() is called. If…
PR description says the cache covers “metadata JSON, manifest lists, and manifests”, but the…
Size() now opens and retains a RandomAccessFile in pending_input_ until the next…

_{Review effort: Lite}

shangxinli · 2026-10-28T17:56:23Z

+
+  EXPECT_THAT(input_file->Size(), HasValue(::testing::Eq(5)));
+  ASSERT_EQ(fs->opened_inputs.size(), 1);
+  EXPECT_TRUE(fs->opened_inputs.front().expired());


this assertion passes because the local shared_ptr goes out of scope at return, not because of the new explicit Close() call this test is meant to cover.

wgtmac · 2026-10-30T16:13:03Z

  /// \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;


Please append this field after the existing members. Inserting it before projection breaks valid callers such as ReaderOptions{path, {}, {}, io, schema}. Add coverage for the old aggregate initialization form.

wgtmac · 2026-10-30T16:13:04Z

+    return options_.expiration_interval_ms > 0 &&
+           now - entry.last_access >=
+               std::chrono::milliseconds(options_.expiration_interval_ms);


INT64_MAX milliseconds passes validation, but this comparison converts it to nanoseconds on libc++ and overflows. Compare elapsed milliseconds instead, or reject values that cannot be converted safely, and cover the upper boundary.

wgtmac · 2026-10-30T16:13:07Z

+  ASSERT_THAT(second_stream, IsOk());
+  ASSERT_THAT(second_stream.value()->Close(), IsOk());
+
+  EXPECT_EQ(file_io.open_count, 1);


This only opens and closes the cached stream. Add a regression that reads a real manifest and manifest list twice through their readers with caching enabled, checks the decoded rows, and verifies the underlying open counts.

manuzhang marked this pull request as ready for review August 6, 2026 09:48

Copilot AI lite review requested due to automatic review settings August 6, 2026 09:48

Copilot started reviewing on behalf of manuzhang August 6, 2026 09:48

View session