feat: cache immutable manifest content per table by manuzhang · Pull Request #875 · apache/iceberg-cpp

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

Summary

Repeated scans currently reread immutable manifests and manifest lists. Add an opt-in, bounded content cache owned by each loaded Table, shared by its manifest readers and scan builders.

Give independently loaded tables separate byte budgets, including tables that share a storage FileIO. Compatible refreshes and committed table handles retain their source table's warm cache.
Keep generic FileIO and ReaderOptions unchanged. A manifest-only I/O adapter carries the table's cache to the existing readers; data-file and table-metadata JSON reads use the storage FileIO directly.
Reuse internal::LruCache for weighted eviction. Cache binary std::vector<std::byte> content, use transparent string_view lookups, and retain canonical locations with a separately bounded alias index.
Skip empty and oversized files, validate lengths before storage calls, and close Arrow size probes on success and failure while preserving both operation and close errors.

Configuration and lifetime

Catalog I/O properties provide defaults, and table properties override them:

Property	Default
`io.manifest.cache-enabled`	`false`
`io.manifest.cache.max-total-bytes`	`104857600`
`io.manifest.cache.max-content-length`	`8388608`

The budget bounds retained content per table. Active readers and concurrent population buffers are outside that budget. Normalized aliases retain at most 64 KiB of strings per table and do not pin content.

Immutable content leaves the cache through weighted eviction or Table::ClearManifestCache(). Time-based expiration, per-file invalidation, and coalescing concurrent loads are not included; io.manifest.cache.expiration-interval-ms is ignored. Files above either byte limit bypass caching.

Validation

C++23 builds of the core, Arrow/Avro bundle, REST catalog, and SQL catalog library.
Eight affected ctest targets pass: util_test, arrow_test, manifest_test, scan_test, table_test, table_update_test, catalog_test, and rest_catalog_test.
Regressions cover independent table limits; compatible and incompatible refreshes; commit reuse; readers outliving their table; real manifest and manifest-list reads and scans for format versions 1–3; canonical and alias hits without storage resolution; empty files; early length validation; and closing failed Arrow size probes.
Positional ReaderOptions aggregate initialization remains covered.
Pre-commit passes on all PR files.

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

🟡 Changes recommended

An active moderate issue remains because NewCachedInputFile can pass an out-of-range size_t length to the virtual method before validating it, risking truncation or an invalid request.

2 open issues

7 resolved since last review

NewCachedInputFile checks the cache using file_location, but CachedInputFile later uses…
The PR description states the cache is for “metadata JSON, manifest lists, and manifests”, but this…
cached->size() is a size_t that is narrowed to int64_t without bounds checking. If a cached…
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…
Size() now opens and retains a RandomAccessFile in pending_input_ until the next…

_{Review effort: Lite}

+  std::unique_ptr<InputFile> input_file;
+  if (length.has_value()) {
+    ICEBERG_ASSIGN_OR_RAISE(input_file, NewInputFile(file_location, *length));


Copilot

🔵 Needs a closer look

Human assessment is needed because the change has multiple interacting file-resource, cache-hit, and content-length behaviors with potentially significant correctness and resource-management consequences.

1 open issue

Validate cached input length before calling virtual method

1 resolved since last review

PR description says the cache covers “metadata JSON, manifest lists, and manifests”, but the…

_{Review effort: Lite}

Copilot

🟡 Changes recommended

Active moderate and critical findings remain in the cache implementation, affecting cache-hit reuse and allowing unbounded growth from zero-length entries.

3 open issues

_{Review effort: Lite}

+  int64_t operator()(const std::string& /*location*/,
+                     const MetadataContent& content) const {
+    return static_cast<int64_t>(content->size());
+  }


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


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

_{Review effort: Lite}

+  void ReserveForUnitWeight() {
+    // Only with unit weights does the maximum weight bound the number of entries.  The
+    // map size can temporarily exceed it by one, see Replace().
+    if constexpr (std::is_same_v<Weigher, UnitWeigher>) {
+      map_.reserve(static_cast<size_t>(max_weight_) + 1);
+    }
+  }


    ICEBERG_ARROW_ASSIGN_OR_RETURN(auto input, fs_->OpenInputFile(file_info));
    ICEBERG_ARROW_ASSIGN_OR_RETURN(auto size, input->GetSize());
+    ICEBERG_ARROW_RETURN_NOT_OK(input->Close());
+    file_size_ = size;


+constexpr auto kMaxCacheWeight = static_cast<size_t>(std::numeric_limits<int64_t>::max());
+
+template <typename T>
+Result<T> ParseNumberProperty(
+    const std::unordered_map<std::string, std::string>& properties, std::string_view key,
+    T default_value) {


Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Keep bounded content-cache state on Table and share it with manifest readers, scans, and compatible refresh and commit handles. Configure independent table budgets from catalog and table properties while keeping FileIO and ReaderOptions unchanged. Reuse weighted LruCache for binary content and bounded normalized aliases. Skip empty and oversized files, validate lengths before storage access, and close Arrow size probes on every path while preserving errors.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

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