You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "liurenjie1024 (via GitHub)" <gi...@apache.org> on 2023/10/31 02:22:06 UTC
[I] Discussion: Replace usage of `Builder` with `TypedBuilder` [iceberg-rust]
liurenjie1024 opened a new issue, #88:
URL: https://github.com/apache/iceberg-rust/issues/88
Currently we used `Builder` to derive builder for simple struct. I just learned that there is another builder implementation called [TypedBuilder](https://github.com/idanarye/rust-typed-builder), which uses generics to ensure safety at compile. I would propose to replace `Builder` with `TypedBuilder`.
--
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.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: [I] Discussion: Replace usage of `Builder` with `TypedBuilder` [iceberg-rust]
Posted by "xiaoyang-sde (via GitHub)" <gi...@apache.org>.
xiaoyang-sde commented on issue #88:
URL: https://github.com/apache/iceberg-rust/issues/88#issuecomment-1829169593
Hi! I'm learning Apache Iceberg and I think this issue might be a good starting point to learn about the source code.
I noticed that `PartitionSpec`, `Snapshot`, and `SortOrder` are still using the `derive_build` macros, and I figured out a solution to migrate them to `TypedBuilder`.
- `Snapshot` is straightforward:
```diff
-#[derive(Debug, PartialEq, Eq, Clone, Builder, Serialize, Deserialize)]
+#[derive(Debug, PartialEq, Eq, Clone, TypedBuilder, Serialize, Deserialize)]
#[serde(from = "SnapshotV2", into = "SnapshotV2")]
-#[builder(setter(prefix = "with"))]
+#[builder(field_defaults(setter(prefix = "with_")))]
/// A snapshot represents the state of a table at some time and is used to access the complete set of data files in the table.
pub struct Snapshot {
/// A unique long ID
snapshot_id: i64,
/// The snapshot ID of the snapshot’s parent.
/// Omitted for any snapshot with no parent
- #[builder(default = "None")]
+ #[builder(default = None)]
parent_snapshot_id: Option<i64>,
/// A monotonically increasing long that tracks the order of
/// changes to a table.
@@ -82,7 +83,7 @@ pub struct Snapshot {
/// A string map that summarizes the snapshot changes, including operation.
summary: Summary,
/// ID of the table’s current schema when the snapshot was created.
- #[builder(setter(strip_option), default = "None")]
+ #[builder(setter(strip_option), default = None)]
schema_id: Option<i64>,
}
```
- `PartitionSpec` is a bit complex, because we need to explicitly define the `with_fields` and `with_partition_field` methods
```diff
-#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Default, Builder)]
+#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Default, TypedBuilder)]
#[serde(rename_all = "kebab-case")]
-#[builder(setter(prefix = "with"))]
+#[builder(field_defaults(setter(prefix = "with_")))]
/// Partition spec that defines how to produce a tuple of partition values from a record.
pub struct PartitionSpec {
/// Identifier for PartitionSpec
pub spec_id: i32,
/// Details of the partition spec
- #[builder(setter(each(name = "with_partition_field")))]
- pub fields: Vec<PartitionField>,
+ #[builder(mutators(
+ pub fn with_fields(self, fields: Vec<PartitionField>) {
+ self.fields.extend(fields)
+ }
+
+ pub fn with_partition_field(self, field: PartitionField) {
+ self.fields.push(field)
+ }
+ ), default, via_mutators)]
+ pub fields: Vec<PartitionField>,
}
```
- `SortOrder` is similar to `PartitionSpec`
--
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: [I] Discussion: Replace usage of `Builder` with `TypedBuilder` [iceberg-rust]
Posted by "xiaoyang-sde (via GitHub)" <gi...@apache.org>.
xiaoyang-sde commented on issue #88:
URL: https://github.com/apache/iceberg-rust/issues/88#issuecomment-1831394325
> Hi, @xiaoyang-sde Welcome to contribute! This seems reasonable to me.
Thanks! Could you please assign this issue to me?
--
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: [I] Discussion: Replace usage of `Builder` with `TypedBuilder` [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on issue #88:
URL: https://github.com/apache/iceberg-rust/issues/88#issuecomment-1831312906
Hi, @xiaoyang-sde Welcome to contribute! This seems reasonable to me.
--
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: [I] Discussion: Replace usage of `Builder` with `TypedBuilder` [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on issue #88:
URL: https://github.com/apache/iceberg-rust/issues/88#issuecomment-1786333662
cc @Fokko @ZENOTME @JanKaul @Xuanwo
--
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: [I] Discussion: Replace usage of `Builder` with `TypedBuilder` [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko closed issue #88: Discussion: Replace usage of `Builder` with `TypedBuilder`
URL: https://github.com/apache/iceberg-rust/issues/88
--
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: [I] Discussion: Replace usage of `Builder` with `TypedBuilder` [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on issue #88:
URL: https://github.com/apache/iceberg-rust/issues/88#issuecomment-1831404891
> > Hi, @xiaoyang-sde Welcome to contribute! This seems reasonable to me.
>
> Thanks! Could you please assign this issue to me?
Done!
--
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