liurenjie1024 commented on code in PR #645:
URL: https://github.com/apache/iceberg-rust/pull/645#discussion_r1825726996
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -235,6 +281,21 @@ impl UnboundPartitionSpec {
}
}
+fn has_sequential_ids(field_ids: &[i32]) -> bool {
Review Comment:
nit: We only need an iterator here, so we can make the argument type `impl
Iterator<item=i32>`, which could avoid collecting to a vec.
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -131,50 +205,22 @@ impl PartitionSpec {
/// * Field names
/// * Source column ids
/// * Transforms
- pub fn is_compatible_with(&self, other: &UnboundPartitionSpec) -> bool {
+ pub fn is_compatible_with_unbound(&self, other: &UnboundPartitionSpec) ->
bool {
Review Comment:
I'm not a big fan of having this method implemented in
`SchemalessPartitionSpec`. The semantics of compatible of two
`BoundPartitionSpec` is clear, but it's not the case for
`SchemalessPartitionSpec` and `UnboundPartitionSpec`. How about we still keep
`compatible_with` still implemented for `BoundPartitionSpec`, while we have a
method following:
```rust
impl SchemalessPartitionSpec {
fn is_compatible_with_unbound(&self, other: &UnboundPartitionSpec,
schema: SchemaRef) {
let p1 = self.bind_to(schema);
let p2 = other.bind_to(schema);
p1.is_compatiable_with(p2)
}
}
```
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -80,44 +110,88 @@ impl PartitionSpec {
&self.fields
}
+ /// The schema this partition spec is bound to
+ pub fn schema(&self) -> &Schema {
+ &self.schema
+ }
+
+ /// The schema ref this partition spec is bound to
+ pub fn schema_ref(&self) -> &SchemaRef {
+ &self.schema
+ }
+
/// Returns if the partition spec is unpartitioned.
///
/// A [`PartitionSpec`] is unpartitioned if it has no fields or all fields
are [`Transform::Void`] transform.
pub fn is_unpartitioned(&self) -> bool {
- self.fields.is_empty()
- || self
- .fields
- .iter()
- .all(|f| matches!(f.transform, Transform::Void))
+ self.fields.is_empty() || self.fields.iter().all(|f| f.transform ==
Transform::Void)
+ }
+
+ /// Turn this partition spec into an unbound partition spec.
+ ///
+ /// The `field_id` is retained as `partition_id` in the unbound partition
spec.
+ pub fn into_unbound(self) -> UnboundPartitionSpec {
+ self.into()
+ }
+
+ /// Turn this partition spec into a preserved partition spec.
+ pub fn into_schemaless(self) -> SchemalessPartitionSpec {
+ self.into()
+ }
+
+ /// Check if this partition spec has sequential partition ids.
+ /// Sequential ids start from 1000 and increment by 1 for each field.
+ /// This is required for spec version 1
+ pub fn has_sequential_ids(&self) -> bool {
+ has_sequential_ids(&self.fields.iter().map(|f|
f.field_id).collect::<Vec<_>>())
+ }
+
+ /// Get the highest field id in the partition spec.
+ /// If the partition spec is unpartitioned, it returns the last
unpartitioned last assigned id (999).
+ pub fn highest_field_id(&self) -> i32 {
Review Comment:
I think we should return `Option<i32>` here?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
