liurenjie1024 commented on code in PR #491:
URL: https://github.com/apache/iceberg-rust/pull/491#discussion_r1716954093
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -44,22 +44,27 @@ pub struct PartitionField {
pub transform: Transform,
}
+impl PartitionField {
+ /// To unbound partition field
+ pub fn into_unbound(self) -> UnboundPartitionField {
+ self.into()
+ }
+}
+
/// Partition spec that defines how to produce a tuple of partition values
from a record.
-#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Default,
Builder)]
+#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Default)]
Review Comment:
> We could only add schema_id as Option<i32> as TableMetadata does not store
the schema_id as part of its PartitionSpecs - thus we won't be able to
deserialize this information. I am not sure if it's worth adding it - it would
only be relevant while building in memory.
I think it would be safer if store a `schema_id` or even `SchemaRef` in
`PartitionSpec`, so that functions such as `partition_type` could be safe. As
deserialization, we may need to change the deserialization function to pass
schema to `PartitionSpec`. But I think we can postpone this to later prs.
> I fully agree with not having the attributes public (see also my initial
statement point 6 at the very top). I addressed it in the latest commit.
Yeah, what I'm thinking is even make it private, so that we can't build
wrong `PartitionSpec`.
It maybe out of scope of this pr, I'll raise a discussion later.
--
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]
