laskoviymishka commented on code in PR #3259:
URL: https://github.com/apache/iceberg-rust/pull/3259#discussion_r4075140593
##########
crates/iceberg/src/io/object_cache.rs:
##########
@@ -36,7 +36,10 @@ pub(crate) enum CachedItem {
#[derive(Clone, Debug, Hash, Eq, PartialEq)]
pub(crate) enum CachedObjectKey {
- ManifestList((String, FormatVersion)),
+ // Location alone: the file is immutable and uniquely identified by its
+ // path, and a cache is bound to one table, so the format version can't
+ // disambiguate two lookups at the same location.
+ ManifestList(String),
Review Comment:
I'd reword this comment — the "format version can't disambiguate" framing
isn't quite right, and it's load-bearing here.
`parse_with_version` genuinely branches on the version: V1 uses the explicit
V1 Avro reader-schema, V2/V3 read the file's embedded schema plus the extra
fields. So the parse product depends on the version, not just the bytes. And
the cache can outlive a format-version change — `ObjectCache` is built once in
`TableBuilder::build()` and survives `Table::with_metadata`, so
`table_metadata.format_version` can shift under a live cache.
For V1→V2 the two parses converge on the same values by spec, so this isn't
a live bug today. But the real invariant is "the first access to a location
parses with the matching version, and a location maps to one set of bytes" —
not "version can never matter." I'd state that instead, so whoever next touches
this (V3 `first_row_id` is the trap) doesn't lean on a claim that isn't true.
wdyt?
##########
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!(
Review Comment:
Tiny thing while we're here — this precondition has no failure message,
unlike the two asserts below it. If someone swaps the fixture to V1 metadata
later, it fires as a bare `left != right` and reads like a logic bug rather
than a broken setup assumption. A short message noting it's a setup invariant
would save that confusion.
##########
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:
This proves the safe direction but not the one the PR is really about.
Calling with the real V2 metadata first parses V2 bytes as V2 (always
correct), then the V1 hit just returns that same `Arc` — so `Arc::ptr_eq` only
tells us the cache didn't re-parse. The motivating case is the reverse: a V1
manifest list cached under V1, then read again after `with_metadata` upgrades
the table to V2. That's the path where dropping `format_version` actually
changes behavior.
I'd add a companion test that writes a real V1 manifest list, populates the
cache under V1, then reads at the same location under V2 metadata, asserting on
content (e.g. `sequence_number == 0`, `content == Data`) alongside the `ptr_eq`
— so we're proving the cached parse is correct for both callers, not just that
it's the same pointer. wdyt?
--
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]
