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/04 21:42:31 UTC
[PR] fix(table): preserve field state on failed JSON decoding [iceberg-go]
fallintoplace opened a new pull request, #1647:
URL: https://github.com/apache/iceberg-go/pull/1647
## Summary
Make partition and sort field JSON decoding transactional.
## Why
PartitionField and SortField currently write decoded values before transform, source ID, direction, and null-order validation completes. Invalid input can therefore leave a reused field partially updated.
## Testing
- `go test . ./table`
--
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(table): preserve field state on failed JSON decoding [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1647:
URL: https://github.com/apache/iceberg-go/pull/1647#discussion_r3732167790
##########
table/sorting_test.go:
##########
@@ -319,6 +319,43 @@ func TestUnmarshalInvalidSortTransform(t *testing.T) {
assert.ErrorIs(t, err, iceberg.ErrInvalidTransform)
}
+func TestSortFieldUnmarshalPreservesStateOnError(t *testing.T) {
+ initial := table.SortField{
+ SourceIDs: []int{7},
+ Transform: iceberg.IdentityTransform{},
+ Direction: table.SortDESC,
+ NullOrder: table.NullsLast,
+ }
+
+ for _, test := range []struct {
+ name string
+ data string
+ }{
+ {
+ name: "invalid transform",
+ data: `{"source-id":1,"transform":"not-a-transform","direction":"asc","null-order":"nulls-first"}`,
+ },
+ {
+ name: "non-positive source ID",
+ data: `{"source-id":0,"transform":"identity","direction":"asc","null-order":"nulls-first"}`,
+ },
+ {
+ name: "invalid direction",
+ data: `{"source-id":1,"transform":"identity","direction":"not-a-direction","null-order":"nulls-first"}`,
+ },
+ {
+ name: "invalid null order",
+ data: `{"source-id":1,"transform":"identity","direction":"asc","null-order":"not-a-null-order"}`,
+ },
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ field := initial
Review Comment:
Same as on `partitions_test.go:693`: `field := initial` shares `SourceIDs`' backing array with `initial`, so `assert.Equal(t, initial, field)` on line 354 cannot detect in-place mutation of that slice — a buggy implementation writing through `s.SourceIDs` would mutate the array both structs point at and the assertion would still pass.
Consider building `initial` inside the subtest, or comparing against an independently constructed value with its own slice.
##########
table/sorting.go:
##########
@@ -158,36 +158,40 @@ func (s *SortField) UnmarshalJSON(b []byte) error {
return err
}
- s.Direction = aux.Direction
- s.NullOrder = aux.NullOrder
+ next := SortField{
+ Direction: aux.Direction,
+ NullOrder: aux.NullOrder,
+ }
if hasSourceIDs {
- s.SourceIDs = aux.SourceIDs
+ next.SourceIDs = aux.SourceIDs
} else {
- s.SourceIDs = []int{aux.SourceID}
+ next.SourceIDs = []int{aux.SourceID}
}
- if err := validateSortSourceIDs(s.SourceIDs); err != nil {
+ if err := validateSortSourceIDs(next.SourceIDs); err != nil {
Review Comment:
Non-blocking, pre-existing, and noted only because the block was rewritten here: `validateSortSourceIDs` still runs before `iceberg.ParseTransform` on line 177, so a document with both a bad source ID and a bad transform reports only the former. Fine as-is — flagging in case the ordering was meant to change along with the restructure.
##########
partitions_test.go:
##########
@@ -660,6 +660,43 @@ func TestPartitionFieldUnmarshalJSON(t *testing.T) {
})
}
+func TestPartitionFieldUnmarshalPreservesStateOnError(t *testing.T) {
+ initial := iceberg.PartitionField{
+ SourceIDs: []int{7},
+ FieldID: 1007,
+ Name: "old",
+ Transform: iceberg.IdentityTransform{},
+ }
+
+ for _, test := range []struct {
+ name string
+ data string
+ }{
+ {
+ name: "invalid transform",
+ data: `{"source-id":1,"field-id":1000,"transform":"not-a-transform","name":"new"}`,
+ },
+ {
+ name: "non-positive source ID",
+ data: `{"source-id":0,"field-id":1000,"transform":"identity","name":"new"}`,
+ },
+ {
+ name: "missing source ID",
+ data: `{"field-id":1000,"transform":"identity","name":"new"}`,
+ },
+ {
+ name: "empty name",
+ data: `{"source-id":1,"field-id":1000,"transform":"identity","name":""}`,
+ },
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ field := initial
Review Comment:
`field := initial` copies the struct but shares `SourceIDs`' backing array with `initial`, so `assert.Equal(t, initial, field)` on line 695 cannot detect in-place mutation of that slice. An implementation that decoded into `p.SourceIDs` would overwrite element 0 of the array **both** structs point at, and the assertion would still pass — the test would go green on the class of bug it exists to catch.
Consider constructing `initial` inside the subtest, or comparing against an independently constructed expected value that owns its own slice.
##########
partitions.go:
##########
@@ -137,41 +137,45 @@ func (p *PartitionField) UnmarshalJSON(b []byte) error {
return err
}
- p.FieldID = aux.FieldID
- p.Name = aux.Name
+ next := PartitionField{
+ FieldID: aux.FieldID,
+ Name: aux.Name,
+ }
var err error
- if p.Transform, err = ParseTransform(aux.TransformString); err != nil {
+ if next.Transform, err = ParseTransform(aux.TransformString); err != nil {
return fmt.Errorf("%w: %w", ErrInvalidPartitionSpec, err)
}
- if err := validateTransform(p.Transform); err != nil {
+ if err := validateTransform(next.Transform); err != nil {
return fmt.Errorf("%w: %w", ErrInvalidPartitionSpec, err)
}
if hasSourceIDs && len(aux.SourceIDs) == 0 {
return fmt.Errorf("%w: partition source-ids cannot be empty", ErrInvalidPartitionSpec)
}
if !hasSourceID && !hasSourceIDs {
- if _, isVoid := p.Transform.(VoidTransform); !isVoid {
+ if _, isVoid := next.Transform.(VoidTransform); !isVoid {
return fmt.Errorf("%w: partition field requires source-id or source-ids", ErrInvalidPartitionSpec)
}
// Preserve compatibility with historical source-less void tombstones.
- p.SourceIDs = []int{0}
+ next.SourceIDs = []int{0}
} else if len(aux.SourceIDs) > 0 {
- p.SourceIDs = aux.SourceIDs
+ next.SourceIDs = aux.SourceIDs
} else {
- p.SourceIDs = []int{aux.SourceID}
+ next.SourceIDs = []int{aux.SourceID}
}
- for _, sourceID := range p.SourceIDs {
- _, isVoid := p.Transform.(VoidTransform)
+ for _, sourceID := range next.SourceIDs {
+ _, isVoid := next.Transform.(VoidTransform)
if sourceID <= 0 && (!isVoid || hasSourceID || hasSourceIDs) {
return fmt.Errorf("%w: partition source ID must be positive: %d", ErrInvalidPartitionSpec, sourceID)
}
}
- if p.Name == "" {
+ if next.Name == "" {
return fmt.Errorf("%w: partition name cannot be empty", ErrInvalidPartitionSpec)
}
+ *p = next
Review Comment:
This line silently fixes a second bug that neither the code nor the PR description claims, and it is worth claiming.
`escapedName` (`partitions.go:55-57`) is a cached `url.QueryEscape(Name)`. The old code assigned `p.Name` but never reset `escapedName`, so re-decoding into an already-populated `PartitionField` left `EscapedName()` returning the **previous** field's escaped name. `*p = next` zeroes it. That is a real correctness fix, not just a hygiene improvement.
Consider mentioning it in the description and pinning it with a test. As written, nothing in the code or the test suite records the dependency, so a future refactor back to in-place assignment would silently reintroduce it.
--
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(table): preserve field state on failed JSON decoding [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1647:
URL: https://github.com/apache/iceberg-go/pull/1647#discussion_r3737315832
##########
partitions_test.go:
##########
@@ -660,6 +677,63 @@ func TestPartitionFieldUnmarshalJSON(t *testing.T) {
})
}
+func TestPartitionFieldUnmarshalPreservesStateOnError(t *testing.T) {
+ for _, test := range []struct {
+ name string
+ data string
+ }{
+ {
+ name: "invalid transform",
+ data: `{"source-id":1,"field-id":1000,"transform":"not-a-transform","name":"new"}`,
+ },
+ {
+ name: "non-positive source ID",
+ data: `{"source-id":0,"field-id":1000,"transform":"identity","name":"new"}`,
+ },
+ {
+ name: "missing source ID",
+ data: `{"field-id":1000,"transform":"identity","name":"new"}`,
+ },
+ {
+ name: "empty name",
+ data: `{"source-id":1,"field-id":1000,"transform":"identity","name":""}`,
+ },
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ initial := iceberg.PartitionField{
+ SourceIDs: []int{7},
Review Comment:
Moving `initial` into the subtest fixed real cross-subtest contamination — previously all four subtests shared one `[]int{7}` backing array, so a mutation in the first would leak into the rest.
Within a single subtest, though, `field := initial` on line 709 is a shallow copy: `field.SourceIDs` and `initial.SourceIDs` still point at the same array. So if the code under test ever mutated the slice *elements* in place, both sides would change together and `assert.Equal(t, initial, field)` would still pass:
```
PROBE aliasing: initial.SourceIDs=[999] field.SourceIDs=[999] assert.Equal_would_pass=true
```
Not reachable in the current implementation — `next.SourceIDs` is only ever assigned a fresh slice (`partitions.go:159/161/163`), never indexed into. So this is a regression-detection gap rather than a live bug. `SourceIDs: slices.Clone([]int{7})`, or comparing `field` against a separately-constructed literal, would close it.
##########
partitions_test.go:
##########
@@ -639,6 +639,23 @@ func TestPartitionFieldUnmarshalJSON(t *testing.T) {
assert.ErrorContains(t, err, "partition field cannot contain both source-id and source-ids")
})
+ t.Run("unmarshal rejects empty source-ids", func(t *testing.T) {
+ var field iceberg.PartitionField
+ err := json.Unmarshal([]byte(`{
+ "source-ids": [],
+ "field-id": 1002,
+ "transform": "identity",
+ "name": "identity"
+ }`), &field)
+ require.ErrorIs(t, err, iceberg.ErrInvalidPartitionSpec)
+ assert.ErrorContains(t, err, "source-ids cannot be empty")
+ })
+
+ t.Run("unmarshal rejects malformed JSON", func(t *testing.T) {
+ var field iceberg.PartitionField
+ require.Error(t, json.Unmarshal([]byte(`{"source-id":`), &field))
Review Comment:
This case never executes any line of `PartitionField.UnmarshalJSON`. `encoding/json` scans the entire buffer for well-formedness *before* dispatching to a custom unmarshaler, so `{"source-id":` fails in the stdlib scanner:
```
err=unexpected end of JSON input type=*json.SyntaxError
```
Coverage over just this subtest confirms it — every block in the method is at hit count 0:
```
partitions.go:114.56,116.48 2 0
partitions.go:116.48,118.3 1 0 <- the raw-unmarshal error return
partitions.go:120.2,120.35 1 0
partitions.go:125.2,136.48 4 0
```
If the intent was to cover the `json.Unmarshal(b, &raw)` guard at `partitions.go:116-118`, that needs input which is **well-formed JSON but structurally wrong** for the target — e.g. `[1,2]` or `"a string"`, which parse fine but fail the `map[string]json.RawMessage` decode inside the method. That guard is currently unreached by any test. Otherwise this subtest is asserting that the standard library rejects truncated JSON, and is better dropped.
##########
table/sorting_test.go:
##########
@@ -337,6 +373,11 @@ func TestSortFieldMultiArgSourceIDs(t *testing.T) {
assert.Contains(t, err.Error(), "cannot contain both source-id and source-ids")
})
+ t.Run("unmarshal rejects malformed JSON", func(t *testing.T) {
+ var field table.SortField
+ require.Error(t, json.Unmarshal([]byte(`{"source-id":`), &field))
Review Comment:
Same issue as the `partitions_test.go` malformed-JSON case: this never reaches `SortField.UnmarshalJSON`. All 24 statement blocks in `table/sorting.go:134-196` report hit count 0 when only this subtest runs — `encoding/json` rejects the truncated buffer before the method is called.
Use well-formed-but-structurally-wrong input (`[1,2]`) if the goal is to cover the `raw` decode guard at `table/sorting.go:136-138`, which no test currently reaches.
Minor, while here: this subtest lives inside `TestSortFieldMultiArgSourceIDs`, which is about multi-arg source IDs. The partition-side equivalent went into `TestPartitionFieldUnmarshalJSON`. Worth aligning.
##########
partitions_test.go:
##########
@@ -660,6 +677,63 @@ func TestPartitionFieldUnmarshalJSON(t *testing.T) {
})
}
+func TestPartitionFieldUnmarshalPreservesStateOnError(t *testing.T) {
+ for _, test := range []struct {
+ name string
+ data string
+ }{
+ {
+ name: "invalid transform",
+ data: `{"source-id":1,"field-id":1000,"transform":"not-a-transform","name":"new"}`,
+ },
+ {
+ name: "non-positive source ID",
+ data: `{"source-id":0,"field-id":1000,"transform":"identity","name":"new"}`,
+ },
+ {
+ name: "missing source ID",
+ data: `{"field-id":1000,"transform":"identity","name":"new"}`,
+ },
+ {
+ name: "empty name",
+ data: `{"source-id":1,"field-id":1000,"transform":"identity","name":""}`,
+ },
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ initial := iceberg.PartitionField{
+ SourceIDs: []int{7},
+ FieldID: 1007,
+ Name: "old",
+ Transform: iceberg.IdentityTransform{},
+ }
+ field := initial
+ require.Error(t, json.Unmarshal([]byte(test.data), &field))
+ assert.Equal(t, initial, field)
+ })
+ }
+}
+
+func TestPartitionFieldUnmarshalReplacesStateOnSuccess(t *testing.T) {
+ spec := iceberg.NewPartitionSpecID(0, iceberg.PartitionField{
+ SourceIDs: []int{1},
+ FieldID: 1000,
+ Name: "old field",
+ Transform: iceberg.IdentityTransform{},
+ })
+ field := spec.Field(0)
+
+ require.NoError(t, json.Unmarshal([]byte(`{
+ "source-id": 2,
+ "field-id": 1001,
+ "transform": "identity",
+ "name": "new field"
+ }`), &field))
+
+ assert.Equal(t, []int{2}, field.SourceIDs)
+ assert.Equal(t, "new field", field.Name)
+ assert.Equal(t, "new+field", field.EscapedName())
Review Comment:
This is the assertion that makes the whole test load-bearing — on a build with the fix reverted, it's the *only* one that fails (`expected: "new+field", actual: "old+field"`). Lines 732 and 733 pass either way, because the old incremental code overwrote `SourceIDs` and `Name` too; the stale unexported `escapedName` cache was the only state that actually survived.
That's a lot of weight on one line, and on the incidental fact that `escapedName` rides through `clonePartitionField` (`partitions.go:571-573`) as part of a by-value copy. Worth making the test harder to accidentally defeat:
- `assert.Equal(t, 1001, field.FieldID)` for completeness.
- A multi-arg → single-arg shrink case: seed `SourceIDs: []int{1, 2}`, decode a document with a single `source-id`, and assert the result is exactly `[]int{2}`. That directly exercises full replacement over merge, independent of the cache.
##########
table/sorting_test.go:
##########
@@ -319,6 +319,42 @@ func TestUnmarshalInvalidSortTransform(t *testing.T) {
assert.ErrorIs(t, err, iceberg.ErrInvalidTransform)
}
+func TestSortFieldUnmarshalPreservesStateOnError(t *testing.T) {
+ for _, test := range []struct {
+ name string
+ data string
+ }{
+ {
+ name: "invalid transform",
+ data: `{"source-id":1,"transform":"not-a-transform","direction":"asc","null-order":"nulls-first"}`,
+ },
+ {
+ name: "non-positive source ID",
+ data: `{"source-id":0,"transform":"identity","direction":"asc","null-order":"nulls-first"}`,
+ },
+ {
+ name: "invalid direction",
+ data: `{"source-id":1,"transform":"identity","direction":"not-a-direction","null-order":"nulls-first"}`,
+ },
+ {
+ name: "invalid null order",
+ data: `{"source-id":1,"transform":"identity","direction":"asc","null-order":"not-a-null-order"}`,
+ },
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ initial := table.SortField{
+ SourceIDs: []int{7},
Review Comment:
Same latent aliasing as the partition-side preservation test: `field := initial` on line 351 shares the `SourceIDs` backing array with `initial`, so an in-place element mutation would be invisible to `assert.Equal` on line 353.
Not triggerable today — `next.SourceIDs` only ever receives a fresh slice (`table/sorting.go:167/169`). `slices.Clone` here keeps it that way if the implementation later changes.
--
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(table): preserve field state on failed JSON decoding [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1647:
URL: https://github.com/apache/iceberg-go/pull/1647
--
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(table): preserve field state on failed JSON decoding [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1647:
URL: https://github.com/apache/iceberg-go/pull/1647#discussion_r3740294219
##########
partitions_test.go:
##########
@@ -778,6 +677,63 @@ func TestPartitionFieldUnmarshalJSON(t *testing.T) {
})
}
+func TestPartitionFieldUnmarshalPreservesStateOnError(t *testing.T) {
+ for _, test := range []struct {
+ name string
+ data string
+ }{
+ {
+ name: "invalid transform",
+ data: `{"source-id":1,"field-id":1000,"transform":"not-a-transform","name":"new"}`,
+ },
+ {
+ name: "non-positive source ID",
+ data: `{"source-id":0,"field-id":1000,"transform":"identity","name":"new"}`,
+ },
+ {
+ name: "missing source ID",
+ data: `{"field-id":1000,"transform":"identity","name":"new"}`,
+ },
+ {
+ name: "empty name",
+ data: `{"source-id":1,"field-id":1000,"transform":"identity","name":""}`,
+ },
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ initial := iceberg.PartitionField{
+ SourceIDs: []int{7},
+ FieldID: 1007,
+ Name: "old",
+ Transform: iceberg.IdentityTransform{},
+ }
+ field := initial
+ require.Error(t, json.Unmarshal([]byte(test.data), &field))
+ assert.Equal(t, initial, field)
+ })
+ }
+}
+
+func TestPartitionFieldUnmarshalReplacesStateOnSuccess(t *testing.T) {
+ spec := iceberg.NewPartitionSpecID(0, iceberg.PartitionField{
+ SourceIDs: []int{1},
+ FieldID: 1000,
+ Name: "old field",
+ Transform: iceberg.IdentityTransform{},
+ })
+ field := spec.Field(0)
+
+ require.NoError(t, json.Unmarshal([]byte(`{
+ "source-id": 2,
+ "field-id": 1001,
+ "transform": "identity",
+ "name": "new field"
+ }`), &field))
+
+ assert.Equal(t, []int{2}, field.SourceIDs)
+ assert.Equal(t, "new field", field.Name)
+ assert.Equal(t, "new+field", field.EscapedName())
Review Comment:
building on zeroshade's point that this test really rests on `EscapedName()` — I'd also pin `FieldID` and `Transform` here. On the reverted build `SourceIDs`/`Name` both still pass, so a regression that cleared the cache but copied the wrong `FieldID` or left a stale `Transform` would slip through, and this is the one test covering `*p = next`.
```go
assert.Equal(t, 1001, field.FieldID)
assert.Equal(t, iceberg.IdentityTransform{}, field.Transform)
```
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: 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