laskoviymishka commented on code in PR #2610:
URL: https://github.com/apache/iceberg-rust/pull/2610#discussion_r3459402680
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -260,6 +261,7 @@ pub struct UnboundPartitionField {
#[serde(rename_all = "kebab-case")]
pub struct UnboundPartitionSpec {
/// Identifier for PartitionSpec
+ #[serde(skip_serializing_if = "Option::is_none")]
Review Comment:
One to flag for a follow-up rather than this PR: this skip is on the core
`iceberg` type, and `UnboundPartitionSpec` also serializes into
`TableUpdate::AddSpec` on the commit path — where a missing `spec-id`
hard-fails a Java server (`PartitionSpecParser.fromJson()` calls
`JsonUtil.getInt(SPEC_ID, json)`, which throws `IllegalArgumentException:
Cannot parse missing int: spec-id`). It's safe today because
`add_partition_spec()` always sets `spec_id` via `with_spec_id` before building
the `AddSpec`, so this is latent rather than live.
Happy to land as-is — but could we open a follow-up PR to localize this skip
to the REST layer (a `serialize_with` or small newtype on
`CreateTableRequest::partition_spec`) so the core spec type isn't coupled to
create-table wire behavior, and add a small test pinning that `AddSpec` always
emits `spec-id`? Same applies to `field_id` just above. wdyt?
--
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]
