This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3527-7b0222f1e87e29273bd7bd98306e5c99fe0dfea8 in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit 081a36dd311f4098a9a19847abedfa05f7af2e8f Author: Danny C Jones <[email protected]> AuthorDate: Mon Oct 26 23:50:52 2026 +0000 refactor(spec)!: accept impl Into<Type> in NestedField constructors (#3527) --- crates/iceberg/public-api.txt | 16 +-- crates/iceberg/src/avro/schema.rs | 178 ++++++++++++---------------------- crates/iceberg/src/expr/term.rs | 4 +- crates/iceberg/src/scan/mod.rs | 12 +-- crates/iceberg/src/spec/datatypes.rs | 28 ++++-- crates/iceberg/src/spec/partition.rs | 22 ++--- crates/iceberg/src/test_utils/scan.rs | 8 +- 7 files changed, 110 insertions(+), 158 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 5f87d9881..701cb7f95 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -2411,8 +2411,8 @@ pub iceberg::spec::MapType::key_field: iceberg::spec::NestedFieldRef pub iceberg::spec::MapType::value_field: iceberg::spec::NestedFieldRef impl iceberg::spec::MapType pub fn iceberg::spec::MapType::new(key_field: iceberg::spec::NestedFieldRef, value_field: iceberg::spec::NestedFieldRef) -> Self -pub fn iceberg::spec::MapType::optional(key_id: i32, key_type: iceberg::spec::Type, value_id: i32, value_type: iceberg::spec::Type) -> Self -pub fn iceberg::spec::MapType::required(key_id: i32, key_type: iceberg::spec::Type, value_id: i32, value_type: iceberg::spec::Type) -> Self +pub fn iceberg::spec::MapType::optional(key_id: i32, key_type: impl core::convert::Into<iceberg::spec::Type>, value_id: i32, value_type: impl core::convert::Into<iceberg::spec::Type>) -> Self +pub fn iceberg::spec::MapType::required(key_id: i32, key_type: impl core::convert::Into<iceberg::spec::Type>, value_id: i32, value_type: impl core::convert::Into<iceberg::spec::Type>) -> Self impl core::clone::Clone for iceberg::spec::MapType pub fn iceberg::spec::MapType::clone(&self) -> iceberg::spec::MapType impl core::cmp::Eq for iceberg::spec::MapType @@ -2484,12 +2484,12 @@ pub iceberg::spec::NestedField::name: alloc::string::String pub iceberg::spec::NestedField::required: bool pub iceberg::spec::NestedField::write_default: core::option::Option<iceberg::spec::Literal> impl iceberg::spec::NestedField -pub fn iceberg::spec::NestedField::list_element(id: i32, field_type: iceberg::spec::Type, required: bool) -> Self -pub fn iceberg::spec::NestedField::map_key_element(id: i32, field_type: iceberg::spec::Type) -> Self -pub fn iceberg::spec::NestedField::map_value_element(id: i32, field_type: iceberg::spec::Type, required: bool) -> Self -pub fn iceberg::spec::NestedField::new(id: i32, name: impl alloc::string::ToString, field_type: iceberg::spec::Type, required: bool) -> Self -pub fn iceberg::spec::NestedField::optional(id: i32, name: impl alloc::string::ToString, field_type: iceberg::spec::Type) -> Self -pub fn iceberg::spec::NestedField::required(id: i32, name: impl alloc::string::ToString, field_type: iceberg::spec::Type) -> Self +pub fn iceberg::spec::NestedField::list_element(id: i32, field_type: impl core::convert::Into<iceberg::spec::Type>, required: bool) -> Self +pub fn iceberg::spec::NestedField::map_key_element(id: i32, field_type: impl core::convert::Into<iceberg::spec::Type>) -> Self +pub fn iceberg::spec::NestedField::map_value_element(id: i32, field_type: impl core::convert::Into<iceberg::spec::Type>, required: bool) -> Self +pub fn iceberg::spec::NestedField::new(id: i32, name: impl alloc::string::ToString, field_type: impl core::convert::Into<iceberg::spec::Type>, required: bool) -> Self +pub fn iceberg::spec::NestedField::optional(id: i32, name: impl alloc::string::ToString, field_type: impl core::convert::Into<iceberg::spec::Type>) -> Self +pub fn iceberg::spec::NestedField::required(id: i32, name: impl alloc::string::ToString, field_type: impl core::convert::Into<iceberg::spec::Type>) -> Self pub fn iceberg::spec::NestedField::with_doc(self, doc: impl alloc::string::ToString) -> Self pub fn iceberg::spec::NestedField::with_initial_default(self, value: iceberg::spec::Literal) -> Self pub fn iceberg::spec::NestedField::with_write_default(self, value: iceberg::spec::Literal) -> Self diff --git a/crates/iceberg/src/avro/schema.rs b/crates/iceberg/src/avro/schema.rs index 8bdd67c50..9af68022a 100644 --- a/crates/iceberg/src/avro/schema.rs +++ b/crates/iceberg/src/avro/schema.rs @@ -695,12 +695,12 @@ mod tests { fn test_unknown_type_schema_conversion() { let schema = Schema::builder() .with_fields(vec![ - NestedField::optional(1, "empty", PrimitiveType::Unknown.into()).into(), + NestedField::optional(1, "empty", PrimitiveType::Unknown).into(), NestedField::required( 2, "unknowns", Type::List(ListType::new( - NestedField::list_element(3, PrimitiveType::Unknown.into(), false).into(), + NestedField::list_element(3, PrimitiveType::Unknown, false).into(), )), ) .into(), @@ -709,9 +709,9 @@ mod tests { "unknown_values", Type::Map(MapType::optional( 5, - PrimitiveType::String.into(), + PrimitiveType::String, 6, - PrimitiveType::Unknown.into(), + PrimitiveType::Unknown, )), ) .into(), @@ -740,71 +740,57 @@ mod tests { #[test] fn test_manifest_file_v1_schema() { let fields = vec![ - NestedField::required(500, "manifest_path", PrimitiveType::String.into()) + NestedField::required(500, "manifest_path", PrimitiveType::String) .with_doc("Location URI with FS scheme") .into(), - NestedField::required(501, "manifest_length", PrimitiveType::Long.into()) + NestedField::required(501, "manifest_length", PrimitiveType::Long) .with_doc("Total file size in bytes") .into(), - NestedField::required(502, "partition_spec_id", PrimitiveType::Int.into()) + NestedField::required(502, "partition_spec_id", PrimitiveType::Int) .with_doc("Spec ID used to write") .into(), - NestedField::optional(503, "added_snapshot_id", PrimitiveType::Long.into()) + NestedField::optional(503, "added_snapshot_id", PrimitiveType::Long) .with_doc("Snapshot ID that added the manifest") .into(), - NestedField::optional(504, "added_data_files_count", PrimitiveType::Int.into()) + NestedField::optional(504, "added_data_files_count", PrimitiveType::Int) .with_doc("Added entry count") .into(), - NestedField::optional(505, "existing_data_files_count", PrimitiveType::Int.into()) + NestedField::optional(505, "existing_data_files_count", PrimitiveType::Int) .with_doc("Existing entry count") .into(), - NestedField::optional(506, "deleted_data_files_count", PrimitiveType::Int.into()) + NestedField::optional(506, "deleted_data_files_count", PrimitiveType::Int) .with_doc("Deleted entry count") .into(), - NestedField::optional( - 507, - "partitions", - ListType { - element_field: NestedField::list_element( - 508, - StructType::new(vec![ - NestedField::required( - 509, - "contains_null", - PrimitiveType::Boolean.into(), - ) + NestedField::optional(507, "partitions", ListType { + element_field: NestedField::list_element( + 508, + StructType::new(vec![ + NestedField::required(509, "contains_null", PrimitiveType::Boolean) .with_doc("True if any file has a null partition value") .into(), - NestedField::optional( - 518, - "contains_nan", - PrimitiveType::Boolean.into(), - ) + NestedField::optional(518, "contains_nan", PrimitiveType::Boolean) .with_doc("True if any file has a nan partition value") .into(), - NestedField::optional(510, "lower_bound", PrimitiveType::Binary.into()) - .with_doc("Partition lower bound for all files") - .into(), - NestedField::optional(511, "upper_bound", PrimitiveType::Binary.into()) - .with_doc("Partition upper bound for all files") - .into(), - ]) - .into(), - true, - ) - .into(), - } + NestedField::optional(510, "lower_bound", PrimitiveType::Binary) + .with_doc("Partition lower bound for all files") + .into(), + NestedField::optional(511, "upper_bound", PrimitiveType::Binary) + .with_doc("Partition upper bound for all files") + .into(), + ]), + true, + ) .into(), - ) + }) .with_doc("Summary for each partition") .into(), - NestedField::optional(512, "added_rows_count", PrimitiveType::Long.into()) + NestedField::optional(512, "added_rows_count", PrimitiveType::Long) .with_doc("Added rows count") .into(), - NestedField::optional(513, "existing_rows_count", PrimitiveType::Long.into()) + NestedField::optional(513, "existing_rows_count", PrimitiveType::Long) .with_doc("Existing rows count") .into(), - NestedField::optional(514, "deleted_rows_count", PrimitiveType::Long.into()) + NestedField::optional(514, "deleted_rows_count", PrimitiveType::Long) .with_doc("Deleted rows count") .into(), ]; @@ -844,19 +830,10 @@ mod tests { let iceberg_schema = { Schema::builder() .with_fields(vec![ - NestedField::required( - 100, - "array_with_string", - ListType { - element_field: NestedField::list_element( - 101, - PrimitiveType::String.into(), - true, - ) + NestedField::required(100, "array_with_string", ListType { + element_field: NestedField::list_element(101, PrimitiveType::String, true) .into(), - } - .into(), - ) + }) .into(), ]) .build() @@ -895,19 +872,10 @@ mod tests { let iceberg_schema = { Schema::builder() .with_fields(vec![ - NestedField::required( - 100, - "array_with_string", - ListType { - element_field: NestedField::list_element( - 101, - PrimitiveType::String.into(), - true, - ) + NestedField::required(100, "array_with_string", ListType { + element_field: NestedField::list_element(101, PrimitiveType::String, true) .into(), - } - .into(), - ) + }) .into(), ]) .build() @@ -960,33 +928,19 @@ mod tests { let iceberg_schema = { Schema::builder() .with_fields(vec![ - NestedField::required( - 100, - "array_with_record", - ListType { - element_field: NestedField::list_element( - 101, - StructType::new(vec![ - NestedField::required( - 102, - "contains_null", - PrimitiveType::Boolean.into(), - ) + NestedField::required(100, "array_with_record", ListType { + element_field: NestedField::list_element( + 101, + StructType::new(vec![ + NestedField::required(102, "contains_null", PrimitiveType::Boolean) .into(), - NestedField::optional( - 103, - "contains_nan", - PrimitiveType::Boolean.into(), - ) + NestedField::optional(103, "contains_nan", PrimitiveType::Boolean) .into(), - ]) - .into(), - true, - ) - .into(), - } + ]), + true, + ) .into(), - ) + }) .into(), ]) .build() @@ -1017,8 +971,7 @@ mod tests { 4, "map", Type::Map(MapType { - key_field: NestedField::map_key_element(5, PrimitiveType::Int.into()) - .into(), + key_field: NestedField::map_key_element(5, PrimitiveType::Int).into(), value_field: NestedField::map_value_element(6, decimal, false).into(), }), ) @@ -1042,11 +995,10 @@ mod tests { // in code, which keeps `logicalType: map` in apache-avro 0.22. let map = |key_id, value_id, value_required| { Type::Map(MapType { - key_field: NestedField::map_key_element(key_id, PrimitiveType::Boolean.into()) - .into(), + key_field: NestedField::map_key_element(key_id, PrimitiveType::Boolean).into(), value_field: NestedField::map_value_element( value_id, - PrimitiveType::Boolean.into(), + PrimitiveType::Boolean, value_required, ) .into(), @@ -1151,14 +1103,11 @@ mod tests { 100, "optional", Type::Map(MapType { - key_field: NestedField::map_key_element( - 102, - PrimitiveType::Boolean.into(), - ) - .into(), + key_field: NestedField::map_key_element(102, PrimitiveType::Boolean) + .into(), value_field: NestedField::map_value_element( 103, - PrimitiveType::Boolean.into(), + PrimitiveType::Boolean, false, ) .into(), @@ -1168,14 +1117,11 @@ mod tests { 104, "required", Type::Map(MapType { - key_field: NestedField::map_key_element( - 105, - PrimitiveType::Boolean.into(), - ) - .into(), + key_field: NestedField::map_key_element(105, PrimitiveType::Boolean) + .into(), value_field: NestedField::map_value_element( 106, - PrimitiveType::Boolean.into(), + PrimitiveType::Boolean, true, ) .into(), @@ -1185,14 +1131,11 @@ mod tests { 107, "string_map", Type::Map(MapType { - key_field: NestedField::map_key_element( - 108, - PrimitiveType::String.into(), - ) - .into(), + key_field: NestedField::map_key_element(108, PrimitiveType::String) + .into(), value_field: NestedField::map_value_element( 109, - PrimitiveType::Long.into(), + PrimitiveType::Long, false, ) .into(), @@ -1258,9 +1201,8 @@ mod tests { let mut converter = AvroSchemaToSchema; let iceberg_type = Type::Map(MapType { - key_field: NestedField::map_key_element(101, PrimitiveType::String.into()).into(), - value_field: NestedField::map_value_element(102, PrimitiveType::Long.into(), false) - .into(), + key_field: NestedField::map_key_element(101, PrimitiveType::String).into(), + value_field: NestedField::map_value_element(102, PrimitiveType::Long, false).into(), }); assert_eq!( diff --git a/crates/iceberg/src/expr/term.rs b/crates/iceberg/src/expr/term.rs index 7c5cc0891..41970e1ef 100644 --- a/crates/iceberg/src/expr/term.rs +++ b/crates/iceberg/src/expr/term.rs @@ -433,8 +433,8 @@ mod tests { let schema = Arc::new( Schema::builder() .with_fields(vec![ - NestedField::optional(1, "id", PrimitiveType::Int.into()).into(), - NestedField::optional(2, "ID", PrimitiveType::Int.into()).into(), + NestedField::optional(1, "id", PrimitiveType::Int).into(), + NestedField::optional(2, "ID", PrimitiveType::Int).into(), ]) .build() .unwrap(), diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index cb77abeb7..636abb4d7 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -837,9 +837,9 @@ mod tests { fn test_resolve_field_id_case_insensitive_rejects_ambiguous_name() { let schema = Schema::builder() .with_fields(vec![ - NestedField::optional(1, "id", PrimitiveType::Int.into()).into(), - NestedField::optional(2, "ID", PrimitiveType::Int.into()).into(), - NestedField::optional(3, "data", PrimitiveType::Int.into()).into(), + NestedField::optional(1, "id", PrimitiveType::Int).into(), + NestedField::optional(2, "ID", PrimitiveType::Int).into(), + NestedField::optional(3, "data", PrimitiveType::Int).into(), ]) .build() .unwrap(); @@ -858,9 +858,9 @@ mod tests { let schema = Schema::builder() .with_schema_id(metadata.current_schema_id()) .with_fields(vec![ - NestedField::optional(1, "id", PrimitiveType::Int.into()).into(), - NestedField::optional(2, "ID", PrimitiveType::Int.into()).into(), - NestedField::optional(3, "data", PrimitiveType::Int.into()).into(), + NestedField::optional(1, "id", PrimitiveType::Int).into(), + NestedField::optional(2, "ID", PrimitiveType::Int).into(), + NestedField::optional(3, "data", PrimitiveType::Int).into(), ]) .build() .unwrap(); diff --git a/crates/iceberg/src/spec/datatypes.rs b/crates/iceberg/src/spec/datatypes.rs index a5ea6ee87..1345cac2a 100644 --- a/crates/iceberg/src/spec/datatypes.rs +++ b/crates/iceberg/src/spec/datatypes.rs @@ -696,12 +696,12 @@ pub type NestedFieldRef = Arc<NestedField>; impl NestedField { /// Construct a new field. - pub fn new(id: i32, name: impl ToString, field_type: Type, required: bool) -> Self { + pub fn new(id: i32, name: impl ToString, field_type: impl Into<Type>, required: bool) -> Self { Self { id, name: name.to_string(), required, - field_type: Box::new(field_type), + field_type: Box::new(field_type.into()), doc: None, initial_default: None, write_default: None, @@ -709,27 +709,27 @@ impl NestedField { } /// Construct a required field. - pub fn required(id: i32, name: impl ToString, field_type: Type) -> Self { + pub fn required(id: i32, name: impl ToString, field_type: impl Into<Type>) -> Self { Self::new(id, name, field_type, true) } /// Construct an optional field. - pub fn optional(id: i32, name: impl ToString, field_type: Type) -> Self { + pub fn optional(id: i32, name: impl ToString, field_type: impl Into<Type>) -> Self { Self::new(id, name, field_type, false) } /// Construct list type's element field. - pub fn list_element(id: i32, field_type: Type, required: bool) -> Self { + pub fn list_element(id: i32, field_type: impl Into<Type>, required: bool) -> Self { Self::new(id, LIST_FIELD_NAME, field_type, required) } /// Construct map type's key field. - pub fn map_key_element(id: i32, field_type: Type) -> Self { + pub fn map_key_element(id: i32, field_type: impl Into<Type>) -> Self { Self::required(id, MAP_KEY_FIELD_NAME, field_type) } /// Construct map type's value field. - pub fn map_value_element(id: i32, field_type: Type, required: bool) -> Self { + pub fn map_value_element(id: i32, field_type: impl Into<Type>, required: bool) -> Self { Self::new(id, MAP_VALUE_FIELD_NAME, field_type, required) } @@ -921,7 +921,12 @@ impl MapType { } /// Construct an optional map type with the given key and value fields. - pub fn optional(key_id: i32, key_type: Type, value_id: i32, value_type: Type) -> Self { + pub fn optional( + key_id: i32, + key_type: impl Into<Type>, + value_id: i32, + value_type: impl Into<Type>, + ) -> Self { Self { key_field: NestedField::map_key_element(key_id, key_type).into(), value_field: NestedField::map_value_element(value_id, value_type, false).into(), @@ -929,7 +934,12 @@ impl MapType { } /// Construct a required map type with the given key and value fields. - pub fn required(key_id: i32, key_type: Type, value_id: i32, value_type: Type) -> Self { + pub fn required( + key_id: i32, + key_type: impl Into<Type>, + value_id: i32, + value_type: impl Into<Type>, + ) -> Self { Self { key_field: NestedField::map_key_element(key_id, key_type).into(), value_field: NestedField::map_value_element(value_id, value_type, true).into(), diff --git a/crates/iceberg/src/spec/partition.rs b/crates/iceberg/src/spec/partition.rs index 3e639de00..e161802c4 100644 --- a/crates/iceberg/src/spec/partition.rs +++ b/crates/iceberg/src/spec/partition.rs @@ -1213,9 +1213,9 @@ mod tests { assert_eq!( partition_spec.partition_type(&schema).unwrap(), StructType::new(vec![ - NestedField::optional(1000, "ts_day", PrimitiveType::Date.into()).into(), - NestedField::optional(1001, "id_bucket", PrimitiveType::Int.into()).into(), - NestedField::optional(1002, "id_truncate", PrimitiveType::String.into()).into(), + NestedField::optional(1000, "ts_day", PrimitiveType::Date).into(), + NestedField::optional(1001, "id_bucket", PrimitiveType::Int).into(), + NestedField::optional(1002, "id_truncate", PrimitiveType::String).into(), ]) ); @@ -1224,16 +1224,16 @@ mod tests { assert_eq!( partition_spec.partition_type(&empty_schema).unwrap(), StructType::new(vec![ - NestedField::optional(1000, "ts_day", PrimitiveType::Date.into()).into(), - NestedField::optional(1001, "id_bucket", PrimitiveType::Int.into()).into(), - NestedField::optional(1002, "id_truncate", PrimitiveType::Unknown.into()).into(), + NestedField::optional(1000, "ts_day", PrimitiveType::Date).into(), + NestedField::optional(1001, "id_bucket", PrimitiveType::Int).into(), + NestedField::optional(1002, "id_truncate", PrimitiveType::Unknown).into(), ]) ); // Missing sources must not suppress validation of transforms on remaining sources. let incompatible_schema = Schema::builder() .with_fields(vec![ - NestedField::required(1, "id", PrimitiveType::Boolean.into()).into(), + NestedField::required(1, "id", PrimitiveType::Boolean).into(), ]) .build() .unwrap(); @@ -1273,7 +1273,7 @@ mod tests { assert_eq!( spec.partition_type(&schema).unwrap(), StructType::new(vec![ - NestedField::optional(1000, "partition", expected_type.into()).into(), + NestedField::optional(1000, "partition", expected_type).into(), ]) ); } @@ -2093,8 +2093,8 @@ mod tests { ] { let schema = Schema::builder() .with_fields(vec![ - NestedField::required(1, "kept", PrimitiveType::Int.into()).into(), - NestedField::required(2, "dropped", source_type.into()).into(), + NestedField::required(1, "kept", PrimitiveType::Int).into(), + NestedField::required(2, "dropped", source_type).into(), ]) .build() .unwrap(); @@ -2105,7 +2105,7 @@ mod tests { .unwrap(); let current_schema = Schema::builder() .with_fields(vec![ - NestedField::required(1, "kept", PrimitiveType::Int.into()).into(), + NestedField::required(1, "kept", PrimitiveType::Int).into(), ]) .build() .unwrap(); diff --git a/crates/iceberg/src/test_utils/scan.rs b/crates/iceberg/src/test_utils/scan.rs index 039fb6421..a6023a9bf 100644 --- a/crates/iceberg/src/test_utils/scan.rs +++ b/crates/iceberg/src/test_utils/scan.rs @@ -630,7 +630,7 @@ impl TableTestFixture { // The schema the data manifest was written under: the current schema with the // partition source `x` still `int`. let mut fields = current_schema.as_struct().fields().to_vec(); - fields[0] = NestedField::required(1, "x", PrimitiveType::Int.into()).into(); + fields[0] = NestedField::required(1, "x", PrimitiveType::Int).into(); let written_schema = Arc::new( Schema::builder() .with_schema_id(0) @@ -766,7 +766,7 @@ impl TableTestFixture { let current_partition_spec = self.table.metadata().default_partition_spec(); let mut fields = current_schema.as_struct().fields().to_vec(); - fields[0] = NestedField::required(1, "x", PrimitiveType::Int.into()).into(); + fields[0] = NestedField::required(1, "x", PrimitiveType::Int).into(); let written_schema = Arc::new( Schema::builder() .with_schema_id(0) @@ -875,7 +875,7 @@ impl TableTestFixture { .as_struct() .fields() .to_vec(); - fields.push(NestedField::optional(9, "d", PrimitiveType::Date.into()).into()); + fields.push(NestedField::optional(9, "d", PrimitiveType::Date).into()); let date_schema = Schema::builder().with_fields(fields).build().unwrap(); let date_spec = PartitionSpec::builder(date_schema.clone()) .add_partition_field("x", "x", Transform::Identity) @@ -904,7 +904,7 @@ impl TableTestFixture { // The schema the data manifest was written under: the partition source `x` still // `int`. let mut fields = current_schema.as_struct().fields().to_vec(); - fields[0] = NestedField::required(1, "x", PrimitiveType::Int.into()).into(); + fields[0] = NestedField::required(1, "x", PrimitiveType::Int).into(); let written_schema = Arc::new( Schema::builder() .with_schema_id(0)
