zeodtr commented on PR #62: URL: https://github.com/apache/iceberg-rust/pull/62#issuecomment-1730738269
Hi, (I'm not sure this is the right place to comment. if not, sorry.) I'm unsure whether a generic builder is a right approach for building a `TableMetadata`. It seems that this approach somewhat violates the decision of issue #3. While `TableMetadata` hides fields and imposes some logic to set the fields, the generic builder allows users to set the fields to whatever value (even an invalid one) they want. For example, in the test case of this commit, `last_column_id` and `schema` are set separately, which can result in an invalid `TableMetadata`. (BTW, the `Schema` struct avoids this problem by manually implementing the `SchemaBuilder` struct.) I think that the main usage patterns of `TableMetadata` are as follows: * CREATE TABLE: build a fresh new TableMetadata with mandatory fields -> mutate it if necessary -> serialize * All other cases: deserialize -> read fields -> clone if necessary -> mutate -> serialize So, maybe a 'builder' is required only for the `CREATE TABLE: build a fresh new TableMetadata with mandatory fields` case. Since `feat: Add Catalog API` (pull request #54) introduced the `TableCreation` struct in `catalog.rs`, perhaps it would be good (and sufficient?) to have a public `new` function (named `new_table` or `for_a_new_table`?) that takes the `TableCreation` struct as an argument in `TableMatadata` struct. Thanks. -- 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]
