Skip to content

feat: add cross-query metadata and snapshot caching - #875

Open
manuzhang wants to merge 6 commits into
apache:mainfrom
manuzhang:agent/add-metadata-snapshot-cache
Open

feat: add cross-query metadata and snapshot caching#875
manuzhang wants to merge 6 commits into
apache:mainfrom
manuzhang:agent/add-metadata-snapshot-cache

Conversation

@manuzhang

@manuzhang manuzhang commented Aug 6, 2026

Copy link
Copy Markdown
Member

Summary

  • add a shared, bounded FileIO content cache for metadata JSON, manifest lists, and manifests
  • persist parsed manifest-list entries on snapshots for reuse across scans
  • wire Java-compatible cache properties through registry, SQL, and in-memory catalogs
  • add cache behavior and concurrency regression coverage

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.

@manuzhang
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 AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/iceberg/file_io_registry.cc
Comment thread src/iceberg/metadata_cache.cc
Copilot AI review requested due to automatic review settings August 6, 2026 11:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AI review requested due to automatic review settings August 6, 2026 11:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AI review requested due to automatic review settings August 6, 2026 11:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/iceberg/snapshot.h Outdated
Copilot AI review requested due to automatic review settings August 6, 2026 13:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

manuzhang and others added 5 commits August 17, 2026 16:45
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>
@manuzhang
manuzhang force-pushed the agent/add-metadata-snapshot-cache branch from d70ca60 to 433113f Compare August 17, 2026 10:06
Copilot AI review requested due to automatic review settings August 17, 2026 10:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/iceberg/snapshot.cc
Comment thread src/iceberg/snapshot.cc
Co-authored-by: Codex <codex@openai.com>
Copilot AI review requested due to automatic review settings August 17, 2026 11:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants