You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by gi...@apache.org on 2026/09/22 21:42:55 UTC
(iceberg-rust) branch main updated: fix(cache): drop format-version from the manifest-list cache key (#3259)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
The following commit(s) were added to refs/heads/main by this push:
new 830bb8865 fix(cache): drop format-version from the manifest-list cache key (#3259)
830bb8865 is described below
commit 830bb886542527f4e8956f03040e7c407b621401
Author: Anoop Johnson <an...@apache.org>
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};