You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "DeviousCardi (via GitHub)" <gi...@apache.org> on 2026/10/14 12:05:08 UTC
[PR] fix(spec): reject field ids in the reserved metadata range [iceberg-rust]
DeviousCardi opened a new pull request, #3451:
URL: https://github.com/apache/iceberg-rust/pull/3451
## Which issue does this PR close?
- Closes #3403.
## What changes are included in this PR?
The spec reserves field ids above 2147483447 (`Integer.MAX_VALUE - 200`) for metadata columns such as `_file`. Table metadata accepted schemas that used them, and the reader then resolved such a user column as the metadata column with the same id.
`TableMetadataBuilder::add_schema` now rejects a schema whose highest field id is above the limit, and `try_normalize` (run after JSON deserialization and in `build()`) applies the same check to every schema in the metadata. The error quotes the limit.
Things that may not be obvious from the diff:
- The check is at the table metadata level, not in `SchemaBuilder::build`, because the crate builds internal schemas with reserved ids (position delete schema, scan projections with `_file`, `_pos`, etc.). Those never go through table metadata, so they are unaffected.
- `Schema::highest_field_id()` already covers nested struct, list and map fields, so no new traversal is needed.
- On load, all schemas are checked, not just the current one. Metadata written elsewhere with a reserved id in a historical schema will now fail to load.
- Partition field ids are not checked. They are allocated from 1000 upward and live in the partition tuple, not the table schema; the issue scopes this to schemas.
## Are these changes tested?
Unit tests: boundary id 2147483447 accepted and 2147483448 rejected through `add_schema` (top-level and nested field, v1-v3) and through JSON deserialization (v2, v3). Existing `_file`/`_pos` scan projection tests still pass.
## AI Disclosure
This change was written with Claude Code and reviewed by the author. Areas a reviewer should focus on:
- Whether rejecting reserved ids in historical (non-current) schemas on load is too strict for metadata written by other implementations, versus checking only the current schema.
- Whether partition field ids should also be bounded.
- The constant is private to `table_metadata.rs`; it could instead live next to the `RESERVED_FIELD_ID_*` constants in `metadata_columns.rs`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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(spec): reject field ids in the reserved metadata range [iceberg-rust]
Posted by "DeviousCardi (via GitHub)" <gi...@apache.org>.
DeviousCardi commented on PR #3451:
URL: https://github.com/apache/iceberg-rust/pull/3451#issuecomment-6376952211
Merged current main and resolved the test-module conflict while keeping both the reserved field ID coverage and upstream partition field ID tests. The PR now changes only the two intended metadata files relative to main. Verified with the pinned nightly formatter (`cargo fmt --check -p iceberg`) and `cargo test -p iceberg --lib` (2051 passed, 1 ignored).
--
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(spec): reject field ids in the reserved metadata range [iceberg-rust]
Posted by "DeviousCardi (via GitHub)" <gi...@apache.org>.
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: 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(spec): reject field ids in the reserved metadata range [iceberg-rust]
Posted by "DeviousCardi (via GitHub)" <gi...@apache.org>.
DeviousCardi commented on PR #3451:
URL: https://github.com/apache/iceberg-rust/pull/3451#issuecomment-6377069926
The new `security_audit` failure is from the lockfile inherited from current main: `ctutils` 0.4.2 is present in both this PR and `origin/main`, and a new 2026-10-23 advisory (RUSTSEC-2026-0385) requires 0.4.4 or newer. The reserved field ID diff does not add or change this dependency. This needs an upstream lockfile refresh; I kept that unrelated security update out of this PR.
--
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(spec): reject field ids in the reserved metadata range [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
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: 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