You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by at...@apache.org on 2026/08/13 10:57:58 UTC
(iceberg-go) branch main updated: fix(table): preserve field state on failed JSON decoding (#1647)
This is an automated email from the ASF dual-hosted git repository.
laskoviymishka pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 52fba8459 fix(table): preserve field state on failed JSON decoding (#1647)
52fba8459 is described below
commit 52fba845983c5d0690aa0257793d0f7dd742ab43
Author: Minh Vu <vu...@gmail.com>
AuthorDate: Thu Aug 13 12:57:52 2026 +0200
fix(table): preserve field state on failed JSON decoding (#1647)
* fix(table): preserve field state on failed JSON decoding
* test(table): cover field decode failure paths
* test(table): cover field state replacement
* test: strengthen JSON state replacement coverage
* test: use invalid parameterized transforms
* test: assert decoded partition transform
---
partitions.go | 26 +++++++++--------
partitions_test.go | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++
table/sorting.go | 20 +++++++------
table/sorting_test.go | 42 +++++++++++++++++++++++++++
4 files changed, 147 insertions(+), 19 deletions(-)
diff --git a/partitions.go b/partitions.go
index 728096c97..44082078e 100644
--- a/partitions.go
+++ b/partitions.go
@@ -151,14 +151,16 @@ 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)
}
@@ -166,26 +168,28 @@ func (p *PartitionField) UnmarshalJSON(b []byte) error {
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
+
return nil
}
diff --git a/partitions_test.go b/partitions_test.go
index bc0ba4559..c4d93cc08 100644
--- a/partitions_test.go
+++ b/partitions_test.go
@@ -757,6 +757,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 non-object JSON", func(t *testing.T) {
+ var field iceberg.PartitionField
+ require.Error(t, json.Unmarshal([]byte(`[1, 2]`), &field))
+ })
+
t.Run("unmarshal source-less void tombstone", func(t *testing.T) {
jsonData := `
{
@@ -767,6 +784,7 @@ func TestPartitionFieldUnmarshalJSON(t *testing.T) {
var field iceberg.PartitionField
require.NoError(t, json.Unmarshal([]byte(jsonData), &field))
assert.Equal(t, 0, field.SourceID())
+ assert.Equal(t, []int{0}, field.SourceIDs)
})
t.Run("unmarshal void with source id", func(t *testing.T) {
@@ -778,6 +796,66 @@ 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":"bucket[0]","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
+ field.SourceIDs = slices.Clone(initial.SourceIDs)
+ 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, 2},
+ 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, 1001, field.FieldID)
+ assert.Equal(t, "new field", field.Name)
+ assert.Equal(t, iceberg.IdentityTransform{}, field.Transform)
+ assert.Equal(t, "new+field", field.EscapedName())
+}
+
func TestPartitionSpecUnmarshalRejectsInvalidStructure(t *testing.T) {
tests := []struct {
name string
diff --git a/table/sorting.go b/table/sorting.go
index 0f25ffb57..dd076b140 100644
--- a/table/sorting.go
+++ b/table/sorting.go
@@ -162,36 +162,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 {
return fmt.Errorf("%w: %w", ErrInvalidSortSourceID, err)
}
var err error
- if s.Transform, err = iceberg.ParseTransform(aux.TransformString); err != nil {
+ if next.Transform, err = iceberg.ParseTransform(aux.TransformString); err != nil {
return err
}
- switch s.Direction {
+ switch next.Direction {
case SortASC, SortDESC:
default:
return ErrInvalidSortDirection
}
- switch s.NullOrder {
+ switch next.NullOrder {
case NullsFirst, NullsLast:
default:
return ErrInvalidNullOrder
}
+ *s = next
+
return nil
}
diff --git a/table/sorting_test.go b/table/sorting_test.go
index 09f3599e7..e27149ec6 100644
--- a/table/sorting_test.go
+++ b/table/sorting_test.go
@@ -357,6 +357,48 @@ 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":"bucket[0]","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},
+ Transform: iceberg.IdentityTransform{},
+ Direction: table.SortDESC,
+ NullOrder: table.NullsLast,
+ }
+ field := initial
+ field.SourceIDs = []int{7}
+ require.Error(t, json.Unmarshal([]byte(test.data), &field))
+ assert.Equal(t, initial, field)
+ })
+ }
+}
+
+func TestSortFieldUnmarshalJSON(t *testing.T) {
+ var field table.SortField
+ require.Error(t, json.Unmarshal([]byte(`[1, 2]`), &field))
+}
+
func TestSortFieldMultiArgSourceIDs(t *testing.T) {
t.Run("unmarshal with source-ids", func(t *testing.T) {
jsonData := `{"source-ids": [2, 3], "transform": "identity", "direction": "asc", "null-order": "nulls-first"}`