You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "anoopj (via GitHub)" <gi...@apache.org> on 2026/09/15 21:26:33 UTC
[PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
anoopj opened a new pull request, #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235
## 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<SchemaId>), 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`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4020835260
##########
crates/iceberg/src/io/object_cache.rs:
##########
@@ -444,6 +443,43 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_manifest_list_with_no_schema_id() {
+ use std::collections::HashMap;
+
+ use crate::spec::{Operation, Snapshot, Summary};
Review Comment:
micro nit: I think typically we do test imports at the top
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4028989181
##########
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 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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4021455241
##########
crates/iceberg/src/io/object_cache.rs:
##########
@@ -444,6 +443,43 @@ mod tests {
);
}
+ #[tokio::test]
+ async fn test_get_manifest_list_with_no_schema_id() {
+ use std::collections::HashMap;
+
+ use crate::spec::{Operation, Snapshot, Summary};
Review Comment:
Fixed. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4028853662
##########
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:
That is a good idea. I added a second `get_manifest_list` call to excercise this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4034091612
##########
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 also think we should we should remove FormatVersion, a manifest list could be uniquely identified by a path. IIRC they were initially added for parsing manifest list.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4020368979
##########
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:
Another alternative is to continue to use `Option<SchemaId>` - but it doesn't seem necessary
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cache): drop schema-id from the manifest-list cache key [iceberg-rust]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #3235:
URL: https://github.com/apache/iceberg-rust/pull/3235#discussion_r4066494228
##########
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:
https://github.com/apache/iceberg-rust/pull/3259 as followup
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org