This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3259-e5a18042b244e61556e8ac7daa33450f3c97cb07 in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit 830bb886542527f4e8956f03040e7c407b621401 Author: Anoop Johnson <[email protected]> AuthorDate: Tue Sep 22 21:36:02 2026 +0000 fix(cache): drop format-version from the manifest-list cache key (#3259) * fix(cache): drop format-version from the manifest-list cache key The manifest-list cache key now uses the location alone; format_version is no longer part of it. ManifestList::parse_with_version(bytes, version) is a pure function of its inputs, the bytes are immutable and uniquely identified by their location, and an ObjectCache is bound to a single table. So format_version can never disambiguate two lookups at the same location. The only case where it could differ for one cache is an in-place format upgrade (v1 -> v2 mid-cache-lifetime). Dropping it is more correct there: the cache serves the original correct v1 parse instead of a miss that would reparse the v1 bytes under v2. Adds test_get_manifest_list_ignores_format_version, asserting two lookups at one location under different format versions return the same cached Arc. Follow-up to the review thread on #3235: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4024075908 * PR feedback --- crates/iceberg/src/io/object_cache.rs | 124 +++++++++++++++++++++++++++++++--- 1 file changed, 114 insertions(+), 10 deletions(-) diff --git a/crates/iceberg/src/io/object_cache.rs b/crates/iceberg/src/io/object_cache.rs index 01a556b9c..dfc6c124c 100644 --- a/crates/iceberg/src/io/object_cache.rs +++ b/crates/iceberg/src/io/object_cache.rs @@ -21,8 +21,8 @@ use std::sync::Arc; use crate::encryption::EncryptionManager; use crate::io::FileIO; use crate::spec::{ - FormatVersion, Manifest, ManifestFile, ManifestList, ManifestListReader, ManifestReader, - SnapshotRef, TableMetadataRef, + Manifest, ManifestFile, ManifestList, ManifestListReader, ManifestReader, SnapshotRef, + TableMetadataRef, }; use crate::{Error, ErrorKind, Result}; @@ -36,7 +36,12 @@ pub(crate) enum CachedItem { #[derive(Clone, Debug, Hash, Eq, PartialEq)] pub(crate) enum CachedObjectKey { - ManifestList((String, FormatVersion)), + // Keyed by location alone. A location holds one set of immutable bytes, and + // a cache is bound to one table, so the first access parses those bytes + // under the table's then-current format version and every later access + // shares that entry. The version can still shift under a live cache across + // an in-place upgrade (`Table::with_metadata`), which is why the key omits it. + ManifestList(String), // The manifest-level `first_row_id` is part of the key because the parsed // manifest inherits it onto its entries: the same physical manifest can be // referenced with different offsets across snapshots and branches, so it @@ -156,10 +161,7 @@ impl ObjectCache { .map(Arc::new); } - let key = CachedObjectKey::ManifestList(( - snapshot.manifest_list().to_string(), - table_metadata.format_version, - )); + let key = CachedObjectKey::ManifestList(snapshot.manifest_list().to_string()); let cache_entry = self .cache .entry_by_ref(&key) @@ -225,9 +227,9 @@ mod tests { use crate::TableIdent; use crate::io::{FileIO, OutputFile}; use crate::spec::{ - DataContentType, DataFileBuilder, DataFileFormat, Literal, ManifestEntry, - ManifestListWriter, ManifestStatus, ManifestWriterBuilder, Operation, Snapshot, Struct, - Summary, TableMetadata, + DataContentType, DataFileBuilder, DataFileFormat, FormatVersion, Literal, + ManifestContentType, ManifestEntry, ManifestListWriter, ManifestStatus, + ManifestWriterBuilder, Operation, Snapshot, Struct, Summary, TableMetadata, }; use crate::table::Table; use crate::test_utils::test_runtime; @@ -348,6 +350,60 @@ mod tests { .unwrap(); manifest_list_write.close().await.unwrap(); } + + async fn setup_v1_manifest_files(&mut self) { + let current_snapshot = self.table.metadata().current_snapshot().unwrap(); + let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); + let current_partition_spec = self.table.metadata().default_partition_spec(); + + // Write data files + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v1(); + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/1.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(100) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(100))])) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + let data_file_manifest = writer.write_manifest_file().await.unwrap(); + + // Write to manifest list + let manifest_list_writer = self + .table + .file_io() + .new_output(current_snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v1( + manifest_list_writer, + current_snapshot.snapshot_id(), + current_snapshot.parent_snapshot_id(), + ); + manifest_list_write + .add_manifests(vec![data_file_manifest].into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } } #[tokio::test] @@ -490,6 +546,54 @@ mod tests { ); } + #[tokio::test] + async fn test_get_manifest_list_serves_cached_v1_parse_after_upgrade() { + let mut fixture = TableTestFixture::new(); + fixture.setup_v1_manifest_files().await; + + let current_snapshot = fixture.table.metadata().current_snapshot().unwrap(); + + // The manifest list on disk is v1. The fixture table is v2, so clone its + // metadata as v1 to model the state before an in-place upgrade; the real + // metadata_ref models the state after it. Both drive lookups at the same + // location through one cache. + let metadata_v2 = fixture.table.metadata_ref(); + let mut metadata_v1 = fixture.table.metadata().clone(); + metadata_v1.format_version = FormatVersion::V1; + let metadata_v1: TableMetadataRef = Arc::new(metadata_v1); + assert_ne!( + metadata_v2.format_version, metadata_v1.format_version, + "setup invariant: the two metadata refs must differ in format version" + ); + + let object_cache = ObjectCache::new(fixture.table.file_io().clone(), None); + + // Cold miss under v1 parses the v1 bytes as v1: content defaults to Data and + // the sequence number to 0. + let cached_under_v1 = object_cache + .get_manifest_list(current_snapshot, &metadata_v1) + .await + .unwrap(); + let entry = cached_under_v1.entries().first().unwrap(); + assert_eq!(entry.content, ManifestContentType::Data); + assert_eq!(entry.sequence_number, 0); + + // Warm hit under v2 at the same location returns the cached v1 parse rather + // than reparsing the v1 bytes as v2, so the upgraded caller sees the same + // correct content. + let cached_under_v2 = object_cache + .get_manifest_list(current_snapshot, &metadata_v2) + .await + .unwrap(); + assert!( + Arc::ptr_eq(&cached_under_v1, &cached_under_v2), + "lookups at one location must share a cache entry across format versions" + ); + let entry = cached_under_v2.entries().first().unwrap(); + assert_eq!(entry.content, ManifestContentType::Data); + assert_eq!(entry.sequence_number, 0); + } + #[tokio::test] async fn test_get_manifest_keys_on_first_row_id() { use crate::spec::{NestedField, PartitionSpec, PrimitiveType, Schema, SchemaRef, Type};
