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