fix(cache): drop schema-id from the manifest-list cache key

[anoopj]

What changes are included in this PR?

ObjectCache::get_manifest_list built the cache key from snapshot.schema_id().unwrap(), but schema-id is optional on snapshots in every table version (v1-v3 per spec, and already read as Option), so a snapshot that omits it panicked on every scan (the object cache is on by default).

schema-id does not belong in this key: ManifestListReader::load parses the manifest list from its bytes and the table format version only and never reads schema-id, and the manifest-list location is already unique per snapshot.

Are these changes tested?

Added new test test_get_manifest_list_with_no_schema_id

[anoopj]

Another alternative is to continue to use Option<SchemaId> - but it doesn't seem necessary

[xanderbailey]

Nice find! I think this change makes sense to me. I think Java just caches on location from what I can see https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/io/ContentCache.java#L233. Their caching is setup a little differently to ours but the idea holds.

[xanderbailey]

micro nit: I think typically we do test imports at the top

[anoopj]

Fixed. Thanks!

[laskoviymishka]

Nice find — the schema_id().unwrap() was always going to panic on a snapshot that omits schema-id, which the spec allows in every version, and dropping it from the cache key is the correct fix.

One thing I'd add before merge: the new test only calls get_manifest_list once, so it proves the panic is gone but never exercises the cached return. A second call asserting the same result would cover the warm-cache path — and if that call reuses the original current_snapshot, it also demonstrates the nice side effect here, that the schema-id Some and None variants at the same location now share a cache entry.

I left a smaller note inline about whether format_version still needs to be in the key at all, but that's out of scope for this PR.

I have no objection to merge as is, but would keep this soak for some time to other to take a look

[laskoviymishka]

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:

// 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.

[anoopj]

That is a good idea. I added a second get_manifest_list call to excercise this.

[laskoviymishka]

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?

[anoopj]

I think format_version should stay. The Java and Rust caching models have different semantics. The Java implementation caches raw bytes and parses after retrieval, so the cached value is version-agnostic. Rust implementation caches the parsed values, so the cached value is version-specific.

If we ever move Rust impelementation to a byte-cache like Java's, format_version could then come out of the key too, but that's a separate change.