From cfb25e048d3fa322bbd8b8b5c761309babc69f72 Mon Sep 17 00:00:00 2001 From: StandingMan Date: Wed, 24 Dec 2025 18:57:08 +0800 Subject: [PATCH 1/3] fix: return proper error rather than persisting error message on snapshot Signed-off-by: StandingMan --- crates/iceberg/src/spec/snapshot.rs | 86 +++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 4 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot.rs b/crates/iceberg/src/spec/snapshot.rs index 5371cf68f2..7d8e9e05d1 100644 --- a/crates/iceberg/src/spec/snapshot.rs +++ b/crates/iceberg/src/spec/snapshot.rs @@ -266,9 +266,9 @@ pub(super) mod _serde { use serde::{Deserialize, Serialize}; use super::{Operation, Snapshot, Summary}; - use crate::Error; use crate::spec::SchemaId; use crate::spec::snapshot::SnapshotRowRange; + use crate::{Error, ErrorKind}; #[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "kebab-case")] @@ -408,9 +408,19 @@ pub(super) mod _serde { timestamp_ms: v1.timestamp_ms, manifest_list: match (v1.manifest_list, v1.manifests) { (Some(file), None) => file, - (Some(_), Some(_)) => "Invalid v1 snapshot, when manifest list provided, manifest files should be omitted".to_string(), - (None, _) => "Unsupported v1 snapshot, only manifest list is supported".to_string() - }, + (Some(_), Some(_)) => { + return Err(Error::new( + ErrorKind::Unexpected, + "v1 snapshot invariant violated: manifest_list and manifests are both set", + )); + } + (None, _) => { + return Err(Error::new( + ErrorKind::FeatureUnsupported, + "v1 snapshot invariant violated: manifest_list is missing", + )); + } + }, summary: v1.summary.unwrap_or(Summary { operation: Operation::default(), additional_properties: HashMap::new(), @@ -517,6 +527,7 @@ mod tests { use chrono::{TimeZone, Utc}; + use crate::spec::TableMetadata; use crate::spec::snapshot::_serde::SnapshotV1; use crate::spec::snapshot::{Operation, Snapshot, Summary}; @@ -604,6 +615,73 @@ mod tests { ); } + #[test] + fn test_v1_snapshot_with_manifest_list_and_manifests() { + { + let metadata = r#" + { + "format-version": 1, + "table-uuid": "d20125c8-7284-442c-9aea-15fee620737c", + "location": "s3://bucket/test/location", + "last-updated-ms": 1700000000000, + "last-column-id": 1, + "schema": { + "type": "struct", + "fields": [ + {"id": 1, "name": "x", "required": true, "type": "long"} + ] + }, + "partition-spec": [], + "properties": {}, + "current-snapshot-id": 111111111, + "snapshots": [ + { + "snapshot-id": 111111111, + "timestamp-ms": 1600000000000, + "summary": {"operation": "append"}, + "manifest-list": "s3://bucket/metadata/snap-123.avro", + "manifests": ["s3://bucket/metadata/manifest-1.avro"] + } + ] + } + "#; + + let result = serde_json::from_str::(metadata); + assert!(result.is_err()); + } + + { + let metadata = r#" + { + "format-version": 1, + "table-uuid": "d20125c8-7284-442c-9aea-15fee620737c", + "location": "s3://bucket/test/location", + "last-updated-ms": 1700000000000, + "last-column-id": 1, + "schema": { + "type": "struct", + "fields": [ + {"id": 1, "name": "x", "required": true, "type": "long"} + ] + }, + "partition-spec": [], + "properties": {}, + "current-snapshot-id": 111111111, + "snapshots": [ + { + "snapshot-id": 111111111, + "timestamp-ms": 1600000000000, + "summary": {"operation": "append"}, + "manifests": ["s3://bucket/metadata/manifest-1.avro"] + } + ] + } + "#; + let result = serde_json::from_str::(metadata); + assert!(result.is_err()); + } + } + #[test] fn test_snapshot_v1_to_v2_with_missing_summary() { use crate::spec::snapshot::_serde::SnapshotV1; From e58d4e21d7368b8160c3ebfe281a73bb10ff6ed9 Mon Sep 17 00:00:00 2001 From: StandingMan Date: Thu, 25 Dec 2025 09:48:40 +0800 Subject: [PATCH 2/3] chore: use more accurate error types Signed-off-by: StandingMan --- crates/iceberg/src/spec/snapshot.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot.rs b/crates/iceberg/src/spec/snapshot.rs index 7d8e9e05d1..b727cf1786 100644 --- a/crates/iceberg/src/spec/snapshot.rs +++ b/crates/iceberg/src/spec/snapshot.rs @@ -410,14 +410,14 @@ pub(super) mod _serde { (Some(file), None) => file, (Some(_), Some(_)) => { return Err(Error::new( - ErrorKind::Unexpected, - "v1 snapshot invariant violated: manifest_list and manifests are both set", + ErrorKind::DataInvalid, + "Invalid v1 snapshot, when manifest list provided, manifest files should be omitted", )); } (None, _) => { return Err(Error::new( - ErrorKind::FeatureUnsupported, - "v1 snapshot invariant violated: manifest_list is missing", + ErrorKind::DataInvalid, + "Unsupported v1 snapshot, only manifest list is supported", )); } }, @@ -648,6 +648,10 @@ mod tests { let result = serde_json::from_str::(metadata); assert!(result.is_err()); + assert_eq!( + result.unwrap_err().to_string(), + "DataInvalid => Invalid v1 snapshot, when manifest list provided, manifest files should be omitted" + ) } { @@ -679,6 +683,10 @@ mod tests { "#; let result = serde_json::from_str::(metadata); assert!(result.is_err()); + assert_eq!( + result.unwrap_err().to_string(), + "DataInvalid => Unsupported v1 snapshot, only manifest list is supported" + ) } } From d8d14b6a2a70bb66ec7e007a11382d1c0263118f Mon Sep 17 00:00:00 2001 From: StandingMan Date: Thu, 25 Dec 2025 16:13:34 +0800 Subject: [PATCH 3/3] chore: rename variables for clarity Signed-off-by: StandingMan --- crates/iceberg/src/spec/snapshot.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot.rs b/crates/iceberg/src/spec/snapshot.rs index b727cf1786..270279988b 100644 --- a/crates/iceberg/src/spec/snapshot.rs +++ b/crates/iceberg/src/spec/snapshot.rs @@ -646,10 +646,13 @@ mod tests { } "#; - let result = serde_json::from_str::(metadata); - assert!(result.is_err()); + let result_both_manifest_list_and_manifest_set = + serde_json::from_str::(metadata); + assert!(result_both_manifest_list_and_manifest_set.is_err()); assert_eq!( - result.unwrap_err().to_string(), + result_both_manifest_list_and_manifest_set + .unwrap_err() + .to_string(), "DataInvalid => Invalid v1 snapshot, when manifest list provided, manifest files should be omitted" ) } @@ -681,10 +684,10 @@ mod tests { ] } "#; - let result = serde_json::from_str::(metadata); - assert!(result.is_err()); + let result_missing_manifest_list = serde_json::from_str::(metadata); + assert!(result_missing_manifest_list.is_err()); assert_eq!( - result.unwrap_err().to_string(), + result_missing_manifest_list.unwrap_err().to_string(), "DataInvalid => Unsupported v1 snapshot, only manifest list is supported" ) }