You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/05 18:46:55 UTC
[PR] fix(partitions): require partition spec JSON fields [iceberg-go]
fallintoplace opened a new pull request, #1662:
URL: https://github.com/apache/iceberg-go/pull/1662
Partition spec decoding currently lets a reused receiver keep its old spec-id and treats a missing fields property as an empty spec.
The decoder now requires spec-id and fields, validates them locally, and replaces the receiver only after successful decoding. Legacy missing partition field-id normalization is unchanged.
Tests:
- go test .
--
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: [PR] fix(partitions): require fields in partition spec JSON [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1662:
URL: https://github.com/apache/iceberg-go/pull/1662
--
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: [PR] fix(partitions): require fields in partition spec JSON [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1662:
URL: https://github.com/apache/iceberg-go/pull/1662#discussion_r3732117422
##########
partitions_test.go:
##########
@@ -317,6 +317,27 @@ func TestSerializePartitionSpec(t *testing.T) {
assert.True(t, spec.Equals(outspec))
}
+func TestDeserializePartitionSpecRequiresTopLevelFields(t *testing.T) {
+ for _, tt := range []struct {
+ name string
+ data string
+ }{
+ {name: "missing spec id", data: `{"fields": []}`},
Review Comment:
This case encodes the behavior that needs to change. `{"fields": []}` is a **valid** REST `PartitionSpec` — the OpenAPI schema requires `fields` and nothing else — so asserting that it returns `ErrInvalidPartitionSpec` locks in the request-path regression described at `partitions.go:464`.
Suggested fix: move this case out of the error table and into a positive test asserting that it decodes successfully with `ID() == InitialPartitionSpecID` and zero fields. The other four rows here are all correct and should stay: `null` `spec-id` is genuinely invalid even where the field is optional, and the three `fields` cases match the schema. Their assertions on receiver preservation are a nice touch and worth keeping in the positive test too — starting from `NewPartitionSpecID(7)` and confirming the decoded ID is `0`, not `7`, is exactly the check that catches the receiver-inheritance trap.
##########
partitions.go:
##########
@@ -454,20 +455,34 @@ func (ps PartitionSpec) MarshalJSON() ([]byte, error) {
}
func (ps *PartitionSpec) UnmarshalJSON(b []byte) error {
- aux := struct {
- ID int `json:"spec-id"`
- Fields []json.RawMessage `json:"fields"`
- }{ID: ps.id}
-
- if err := json.Unmarshal(b, &aux); err != nil {
+ var raw map[string]json.RawMessage
+ if err := json.Unmarshal(b, &raw); err != nil {
return err
Review Comment:
Non-blocking, for consistency. Every other failure path added in this PR wraps its error with `ErrInvalidPartitionSpec`, so callers can use `errors.Is` uniformly. This one returns the bare `json.Unmarshal` error, which means a malformed top-level value — a string or an array where an object was expected — is the one shape error that does not match the sentinel.
Suggested fix: wrap it the same way as the others, e.g. `fmt.Errorf("%w: %w", ErrInvalidPartitionSpec, err)`. The same applies to the two bare `return err` paths inside the field loop, at `:488` and `:500`.
##########
partitions.go:
##########
@@ -454,20 +455,34 @@ func (ps PartitionSpec) MarshalJSON() ([]byte, error) {
}
func (ps *PartitionSpec) UnmarshalJSON(b []byte) error {
- aux := struct {
- ID int `json:"spec-id"`
- Fields []json.RawMessage `json:"fields"`
- }{ID: ps.id}
-
- if err := json.Unmarshal(b, &aux); err != nil {
+ var raw map[string]json.RawMessage
+ if err := json.Unmarshal(b, &raw); err != nil {
return err
}
- if aux.ID < 0 {
- return fmt.Errorf("%w: spec ID must be non-negative: %d", ErrInvalidPartitionSpec, aux.ID)
+
+ rawID, ok := raw["spec-id"]
+ if !ok || bytes.Equal(bytes.TrimSpace(rawID), []byte("null")) {
Review Comment:
This is the blocking issue. Requiring `spec-id` is correct for stored table metadata, but this decoder is not only used for stored table metadata.
The REST OpenAPI `PartitionSpec` schema requires only `fields`; `spec-id` is optional and marked read-only, deliberately, so that a client can send a spec and let the server assign the ID. `PartitionSpec.UnmarshalJSON` is the single entry point for both shapes, so applying the stored-metadata requirement here applies it to requests too.
The concrete regression is at `table/updates.go:242` — `addPartitionSpecUpdate.Spec` is an `*iceberg.PartitionSpec`, so it decodes through this function. A spec-valid update that worked before this change:
```json
{"action": "add-spec", "spec": {"fields": []}}
```
now fails with `partition spec is missing required spec-id`. Create-table requests whose spec omits the ID are exposed identically. A shared unmarshaller cannot impose the stored-metadata requirement globally without breaking the request path.
Suggested fix, in two parts:
1. **Here:** keep `fields` required — that part is right and matches both schemas — but allow `spec-id` to be absent, defaulting to `InitialPartitionSpecID`. Note that the default must be the constant, *not* the receiver's current `ps.id`: the old code seeded `aux` from `ps.id`, and reintroducing that would make decoding depend on whatever the receiver happened to hold, which is the inheritance problem the `decoded` local at `:507` otherwise fixes. Explicit `null` should still be rejected, as should a negative or non-integer value.
2. **In the metadata decoder:** enforce the requirement where it actually belongs, in the code that parses full table metadata and therefore knows both the format version and the context. That is the only place with enough information to distinguish "stored metadata missing a required field" from "a request legitimately omitting a read-only one."
--
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