dannycjones commented on code in PR #3451:
URL: https://github.com/apache/iceberg-rust/pull/3451#discussion_r4367952544
##########
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:
nitpick: we should assert more of the error here, it's not clear to me how
what we're erroring on so I can't make sense of how this will be reflected to
callers
I would expect instead to see something that asserts on both error kind and
then error message individually. We shouldn't call `to_string`.
##########
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:
same feedback as
https://github.com/apache/iceberg-rust/pull/3451/changes#r4367952544
--
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]
