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: [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]
