anoopj commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4039776352
##########
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:
I looked into this and I think you are right.
`ManifestList::parse_with_version(bytes, version)` is a pure function of its
two inputs, and the manifest-list bytes are immutable and uniquely identified
by the location.
`format_version` would only disambiguate if one cache instance saw the same
location under two versions. That can happen across an in-place format upgrade.
Consider the following scenario:
1. Table is at v1, and cache is hydrated.
2. Table gets upgraded to v2.
3. Now read happens. Question is what happens here.
So if `format_version` is in the cache key, 3 will always see a cache miss.
It will then reparse v1 file as v2. If `format_version` is not in the cache
key, the cache will serve the correct v1 parse output, so arguably more
correct.
I'll drop it in a follow-up so this PR stays scoped to the schema-id panic
fix.
--
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]
