lgingerich opened a new issue, #1936: URL: https://github.com/apache/iceberg-rust/issues/1936
### Is your feature request related to a problem or challenge? TypedBuilder introduces a pattern that feels uncommon in Rust and makes initialization harder to follow than necessary. - Standard Rust patterns (new() constructors, Default, and struct literals) are simpler, explicit, and more familiar than macro-based builders. - Because this pattern is relatively uncommon in Rust projects and feels less natural than typical Rust initialization, I think it would be beneficial to consider removing TypedBuilder in favor of more idiomatic, explicit construction. - Any compile-time guarantees we currently get from TypedBuilder (e.g. “these fields are required”) can be expressed just as well with normal constructors and methods. For pub structs with pub fields, the builder doesn’t add guarantees at all, since callers can always use struct literals. ### Describe the solution you'd like The code below demonstrates our current usage of TypedBuilder and the proposed solution. Current: ```rust // https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/spec/manifest/entry.rs#L39 pub struct ManifestEntry { pub status: ManifestStatus, #[builder(default, setter(strip_option(fallback = snapshot_id_opt)))] pub snapshot_id: Option<i64>, #[builder(default, setter(strip_option(fallback = sequence_number_opt)))] pub sequence_number: Option<i64>, #[builder(default, setter(strip_option(fallback = file_sequence_number_opt)))] pub file_sequence_number: Option<i64>, pub data_file: DataFile, } // impl ManifestEntry{} does not have any new() or default() methods. // We then build ManifestEntry as follows: // https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/spec/manifest/writer.rs#L284 let entry = ManifestEntry { status: ManifestStatus::Added, snapshot_id: self.snapshot_id, sequence_number: (sequence_number >= 0).then_some(sequence_number), file_sequence_number: None, data_file, }; ``` Proposed Design: ```rust pub struct ManifestEntry { pub status: ManifestStatus, pub snapshot_id: Option<i64>, pub sequence_number: Option<i64>, pub file_sequence_number: Option<i64>, pub data_file: DataFile, } impl ManifestEntry { pub fn new(status: ManifestStatus, data_file: DataFile) -> Self { Self { status, snapshot_id: None, sequence_number: None, file_sequence_number: None, data_file, } } // all the existing methods stay the same: // is_alive, status, content_type, file_format, file_path, record_count, // inherit_data, snapshot_id, sequence_number, file_size_in_bytes, data_file } // Two ways in which we can then build ManifestEntry are: let mut entry = ManifestEntry::new(ManifestStatus::Added, data_file); entry.inherit_data(&snapshot_entry); // or set fields directly if needed: let entry = ManifestEntry { status: ManifestStatus::Added, snapshot_id: Some(1), sequence_number: Some(2), file_sequence_number: Some(3), data_file, }; ``` ### Willingness to contribute I can contribute to this feature independently -- 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]
