search

apache / iceberg-rust

Apache Iceberg
https://rust.iceberg.apache.org/
Apache License 2.0
642 stars 149 forks source link

No builder for TableMetadata and no public field #52

Open y0psolo opened 1 year ago

y0psolo commented 1 year ago

There is no way to create a new TableMetadata struct outside a deserialization operation. I don't know if it's intended but all other struct have either builder or public field.

By the way i dont see clearly a consistent way of building API struct. Sometimes there is a builder and no public field, sometimes public field and no builder and sometimes builder and public field.

To have a better visibility on this I could write, if you want, a short table summary if needed with all API struct and two column : pub field (yes/no), builder (yes/no)

liurenjie1024 commented 1 year ago

Hi, @y0psolo Thanks for the contribution. We had a discussion in #3 about the style of public api, in conclusion

  1. For complex structs such as TableMetadata, Sanpshot, Schema, etc, we prefer not to expose pub field, but only necessary methods
  2. I'm ok with builder pattern.

cc @Xuanwo @ZENOTME @JanKaul Any comments?

JanKaul commented 1 year ago

I would also be in favor of using the builder pattern for the pub structs.

If I'm correct all pub structs except for TableMetadata already have a builder. With the derive_builder crate it should be quite easy to implement the buider for TableMetadata.

y0psolo commented 1 year ago

i will try to start working on it next week. Not too much spare time till now.