feat: support UnboundPartitionSpec #106
Conversation
There was a problem hiding this comment.
Thanks for the pr, it looks great. For now, the most important use case of UnboundPartitionSpec is in the catalog api. We should replace the PartitionSpec in TableCreation with UnboundPartitionSpec since the spec id and field id should not be specified by user. Same should be applied to AddSpec table update.
| pub transform: Transform, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Default, Builder)] |
There was a problem hiding this comment.
I don't find a neat way to add #[builder(setter(each(name = "with_partition_field")))] in TypedBuilder.
Accoding to this, it may need to use mutators?
Maybe we can try later?
crates/iceberg/src/spec/partition.rs
Outdated
| } | ||
|
|
||
| /// Bind unbound partition spec to a schema | ||
| pub fn bind(&self, schema: SchemaRef) -> Result<PartitionSpec> { |
There was a problem hiding this comment.
In fact, I'm hesitating to implement bind method for now since I don't know if it's really useful, maybe we should implement it later when necessary?
There was a problem hiding this comment.
I'm also a little confused about the process. If there's a need, I can try it then. Thanks @liurenjie1024.
my-vegetable-has-exploded
force-pushed
the
UnboundPartitionSpec
branch
from
0a4a7ed
to
b85eb16
Compare
|
cc @Fokko PTAL, I think this is ready to go |
my-vegetable-has-exploded
force-pushed
the
UnboundPartitionSpec
branch
from
48a4e63
to
4c044dc
Compare
| "fields": [ | ||
| { | ||
| "source-id": 4, | ||
| "field-id": 1000, |
There was a problem hiding this comment.
Why remove these? They are part of the PartitionField: https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.py#L131-L135
There was a problem hiding this comment.
I think this field_id in request is optional?
There was a problem hiding this comment.
The open-api spec is a bit unclear, but it isn't right now:
References:
References:
And there it is non-optional.
There was a problem hiding this comment.
On Java:
And there it is optional:
There was a problem hiding this comment.
It looks like it is for historical reasons. For old specs they can be missing, and they'll just auto-increment.
There was a problem hiding this comment.
BTW, cleaned up the spec a bit: apache/iceberg#9240
There was a problem hiding this comment.
Thanks for the reference, I saw the comments now. But I think in the rest api request, it's reasonable to make it optional, and ask the server to assign a meaningful field id to it?
|
Thanks @my-vegetable-has-exploded for working on this, and @liurenjie1024 and @Xuanwo for reviewing this 👍 |
close #98