DeviousCardi commented on code in PR #3451:
URL: https://github.com/apache/iceberg-rust/pull/3451#discussion_r4369713089
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -4276,6 +4293,39 @@ mod tests {
);
}
+ #[test]
+ fn test_deserialize_rejects_field_ids_in_reserved_range() {
+ let schema = Schema::builder()
+ .with_fields(vec![
+ NestedField::required(1, "id",
Type::Primitive(PrimitiveType::Long)).into(),
+ ])
+ .build()
+ .unwrap();
+
+ for format_version in [FormatVersion::V2, FormatVersion::V3] {
+ let metadata = TableMetadataBuilder::new(
+ schema.clone(),
+ PartitionSpec::unpartition_spec().into_unbound(),
+ SortOrder::unsorted_order(),
+ "s3://test/location".to_string(),
+ format_version,
+ HashMap::new(),
+ )
+ .unwrap()
+ .build()
+ .unwrap()
+ .metadata;
+ let mut json = serde_json::to_value(&metadata).unwrap();
+
+ json["schemas"][0]["fields"][0]["id"] =
serde_json::json!(2147483447);
+ serde_json::from_value::<TableMetadata>(json.clone()).unwrap();
+
+ json["schemas"][0]["fields"][0]["id"] =
serde_json::json!(2147483448);
+ let err =
serde_json::from_value::<TableMetadata>(json).unwrap_err();
+ assert!(err.to_string().contains("2147483447"), "{err}");
Review Comment:
Updated in 8b1ebc3e. The test now converts the parsed metadata with
`TableMetadata::try_from`, which runs the same normalization as deserialization
but returns an `iceberg::Error` rather than a `serde_json::Error`. It asserts
`ErrorKind::DataInvalid` and the full message: `Schema 0 uses field id
2147483448, but table field ids must not exceed 2147483447; higher ids are
reserved for metadata columns`.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -3795,6 +3796,63 @@ mod tests {
assert_eq!(field_ids, vec![1000, 1001, 1002]); // Reused 1000, 1001;
new 1002
}
+ #[test]
+ fn test_add_schema_accepts_max_table_field_id() {
+ let schema = Schema::builder()
+ .with_fields(vec![
+ NestedField::optional(2147483447, "a",
Type::Primitive(PrimitiveType::Long)).into(),
+ ])
+ .build()
+ .unwrap();
+
+ let metadata = builder_without_changes(FormatVersion::V2)
+ .add_schema(schema)
+ .unwrap()
+ .build()
+ .unwrap()
+ .metadata;
+
+ assert_eq!(metadata.last_column_id, 2147483447);
+ }
+
+ #[test]
+ fn test_add_schema_rejects_field_ids_in_reserved_range() {
+ let top_level = Schema::builder()
+ .with_fields(vec![
+ NestedField::optional(2147483448, "a",
Type::Primitive(PrimitiveType::Long)).into(),
+ ])
+ .build()
+ .unwrap();
+ let nested = Schema::builder()
+ .with_fields(vec![
+ NestedField::optional(
+ 4,
+ "s",
+ Type::Struct(StructType::new(vec![
+ NestedField::optional(
+ 2147483448,
+ "a",
+ Type::Primitive(PrimitiveType::Long),
+ )
+ .into(),
+ ])),
+ )
+ .into(),
+ ])
+ .build()
+ .unwrap();
+
+ for format_version in [FormatVersion::V1, FormatVersion::V2,
FormatVersion::V3] {
+ for schema in [top_level.clone(), nested.clone()] {
+ let err = builder_without_changes(format_version)
+ .add_schema(schema)
+ .unwrap_err();
+ assert_eq!(err.kind(), ErrorKind::DataInvalid);
+ assert!(err.message().contains("2147483447"), "{err}");
Review Comment:
Updated in 8b1ebc3e. The builder test now asserts `ErrorKind::DataInvalid`
and the exact message for both the top-level and nested schema cases across V1,
V2 and V3.
--
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]
