anoopj commented on code in PR #3259:
URL: https://github.com/apache/iceberg-rust/pull/3259#discussion_r4076676407


##########
crates/iceberg/src/io/object_cache.rs:
##########
@@ -490,6 +490,46 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn test_get_manifest_list_ignores_format_version() {
+        let mut fixture = TableTestFixture::new();
+        fixture.setup_manifest_files().await;
+
+        let current_snapshot = 
fixture.table.metadata().current_snapshot().unwrap();
+
+        // A single ObjectCache is bound to one table whose format version is 
fixed,
+        // so the format version cannot disambiguate two lookups at the same
+        // manifest-list location. Two metadata refs differing only in format 
version
+        // must therefore share one cache entry.
+        let mut metadata_v1 = fixture.table.metadata().clone();
+        metadata_v1.format_version = FormatVersion::V1;
+        let metadata_v1: TableMetadataRef = Arc::new(metadata_v1);
+        assert_ne!(
+            fixture.table.metadata().format_version,
+            metadata_v1.format_version
+        );
+
+        let object_cache = ObjectCache::new(fixture.table.file_io().clone(), 
None);
+
+        // Cold miss under the table's real format version populates the cache.
+        let inserted = object_cache
+            .get_manifest_list(current_snapshot, &fixture.table.metadata_ref())
+            .await
+            .unwrap();
+        assert_eq!(inserted.entries().len(), 1);
+
+        // Warm hit under a different format version at the same location 
returns the
+        // same cached entry rather than reparsing.
+        let cached = object_cache
+            .get_manifest_list(current_snapshot, &metadata_v1)
+            .await
+            .unwrap();
+        assert!(
+            Arc::ptr_eq(&inserted, &cached),

Review Comment:
   Rewrote it on the upgrade path: write a real v1 manifest list, cache it 
under v1, then read the same location under v2.  Went with one test instead of 
a companion since that warm-hit ptr_eq already covers the shared-entry check 
the old test did. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to