laskoviymishka commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4024075908
##########
crates/iceberg/src/io/object_cache.rs:
##########
@@ -36,7 +36,7 @@ pub(crate) enum CachedItem {
#[derive(Clone, Debug, Hash, Eq, PartialEq)]
pub(crate) enum CachedObjectKey {
- ManifestList((String, FormatVersion, SchemaId)),
+ ManifestList((String, FormatVersion)),
Review Comment:
not for this PR, but now that `schema_id` is gone I'm a little curious what
`format_version` is still buying us. Each `ObjectCache` is bound to a single
table, so the format version is fixed for the cache's lifetime and can't
disambiguate two lookups — the location is already unique.
Given xanderbailey's note that Java keys on location alone, I'd either drop
it in a follow-up or leave a one-liner on why it stays, so the next reader
isn't left guessing. wdyt?
##########
crates/iceberg/src/io/object_cache.rs:
##########
@@ -444,6 +445,39 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_manifest_list_with_no_schema_id() {
+ let mut fixture = TableTestFixture::new();
+ fixture.setup_manifest_files().await;
+
+ let current_snapshot =
fixture.table.metadata().current_snapshot().unwrap();
+
+ // The spec marks `schema-id` optional in every version (v1-v3), so a
+ // snapshot may omit it; fetching its manifest list must not depend on
the
+ // schema-id being present.
+ let snapshot: SnapshotRef = Snapshot::builder()
+ .with_snapshot_id(current_snapshot.snapshot_id())
+ .with_sequence_number(current_snapshot.sequence_number())
+ .with_timestamp_ms(current_snapshot.timestamp_ms())
+ .with_manifest_list(current_snapshot.manifest_list())
+ .with_summary(Summary {
+ operation: Operation::Append,
+ additional_properties: HashMap::new(),
+ })
+ .build()
+ .into();
+ assert!(snapshot.schema_id().is_none());
+
+ let object_cache = ObjectCache::new(fixture.table.file_io().clone(),
None);
+
+ let result_manifest_list = object_cache
+ .get_manifest_list(&snapshot, &fixture.table.metadata_ref())
+ .await
+ .unwrap();
+
+ assert_eq!(result_manifest_list.entries().len(), 1);
Review Comment:
this proves the panic is gone, but it only hits the cold path — a single
`get_manifest_list` call, so the cached return is never exercised. If a
refactor reintroduced `schema_id` into the key on the insert path only, this
test would still pass while the cache quietly degraded to miss-every-time.
I'd add a second call and assert it matches:
```rust
// second call must hit the cache without rebuilding
let cached = object_cache
.get_manifest_list(&snapshot, &fixture.table.metadata_ref())
.await
.unwrap();
assert_eq!(cached.entries().len(), 1);
```
And if that second call uses the original `current_snapshot` (schema-id
`Some(1)`) instead, it also covers the merge this fix enables — the `Some` and
`None` variants at the same location now sharing one entry.
--
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]
