CTTY commented on code in PR #1960:
URL: https://github.com/apache/iceberg-rust/pull/1960#discussion_r2646329181
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -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",
Review Comment:
```suggestion
"Invalid v1 snapshot, when manifest list
provided, manifest files should be omitted",
```
I think the original error message is more specific
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -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::<TableMetadata>(metadata);
+ assert!(result.is_err());
Review Comment:
We should also check if the error has the correct error type/message. So
other developers will know what we are testing here
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -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,
Review Comment:
```suggestion
ErrorKind::DataInvalid,
```
Same here
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -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::<TableMetadata>(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::<TableMetadata>(metadata);
+ assert!(result.is_err());
Review Comment:
Same here, we should also check if the error has the correct error
type/message.
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -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,
Review Comment:
```suggestion
ErrorKind::DataInvalid,
```
Referring to the v3 parsing and error handling logic
[here](https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/spec/snapshot.rs#L349),
`DataInvlid` seems more apporpriate
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -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",
Review Comment:
```suggestion
"Unsupported v1 snapshot, only manifest list is
supported",
```
Same here, the original message is more specific
--
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]
