laskoviymishka commented on code in PR #1315:
URL: https://github.com/apache/iceberg-go/pull/1315#discussion_r3490798679
##########
types.go:
##########
@@ -444,15 +444,22 @@ func (l *ListType) String() string { return
fmt.Sprintf("list<%s>", l.Element) }
func (l *ListType) UnmarshalJSON(b []byte) error {
aux := struct {
- ID int `json:"element-id"`
+ ID *int `json:"element-id"`
Elem typeIFace `json:"element"`
Req bool `json:"element-required"`
}{}
if err := json.Unmarshal(b, &aux); err != nil {
return err
}
- l.ElementID = aux.ID
+ if aux.ID == nil {
+ return fmt.Errorf("%w: field is missing required 'element-id'
key in JSON", ErrInvalidSchema)
+ }
+ if *aux.ID == 0 {
+ return fmt.Errorf("%w: field 'element-id' must not be 0",
ErrInvalidSchema)
+ }
Review Comment:
The nil check here is right — it mirrors Java's `JsonUtil.getInt` existence
check, so keep it. The `== 0` rejection is the blocker, though, and I'd drop it.
The spec only requires field IDs to be unique in the table schema — it
doesn't reserve 0 or require positivity, and Java, PyIceberg, and iceberg-rust
all parse 0 without complaint. So this makes iceberg-go permanently unable to
read otherwise-valid metadata any of them wrote with `element-id: 0`. It also
breaks our own round-trip: `ListType.ElementID` is a plain `int`, so
`json.Marshal(&iceberg.ListType{})` emits `element-id: 0` and this same method
then refuses it.
```suggestion
if aux.ID == nil {
return fmt.Errorf("%w: field is missing required 'element-id'
key in JSON", ErrInvalidSchema)
}
```
Reject 0 vs. accept it as the spec allows — wdyt?
##########
schema_test.go:
##########
@@ -442,6 +442,217 @@ func TestUnmarshalSchema(t *testing.T) {
assert.True(t, tableSchemaSimple.Equals(&schema))
}
+func TestUnmarshalSchemaRejectsDuplicateFieldIDs(t *testing.T) {
+ tests := []struct {
+ name string
+ schema string
+ contains string
+ }{
+ {
+ name: "top level",
+ schema: `{
+ "type": "struct",
+ "fields": [
+ {"id": 1, "name": "foo", "type":
"string", "required": false},
+ {"id": 1, "name": "bar", "type": "int",
"required": true}
+ ],
+ "schema-id": 1,
+ "identifier-field-ids": []
+ }`,
+ contains: "multiple fields for id 1: foo and bar",
+ },
+ {
+ name: "nested struct",
+ schema: `{
+ "type": "struct",
+ "fields": [
+ {"id": 1, "name": "id", "type": "long",
"required": true},
+ {
+ "id": 2,
+ "name": "payload",
+ "type": {
+ "type": "struct",
+ "fields": [
+ {"id": 3,
"name": "inner", "type": "string", "required": false},
+ {"id": 3,
"name": "inner_dup", "type": "int", "required": false}
+ ]
+ },
+ "required": false
+ }
+ ],
+ "schema-id": 1,
+ "identifier-field-ids": []
+ }`,
+ contains: "multiple fields for id 3: inner and
inner_dup",
+ },
+ {
+ name: "list element",
+ schema: `{
+ "type": "struct",
+ "fields": [
+ {"id": 1, "name": "id", "type": "long",
"required": true},
+ {
+ "id": 2,
+ "name": "items",
+ "type": {
+ "type": "list",
+ "element-id": 1,
+ "element": "int",
+ "element-required":
false
+ },
+ "required": false
+ }
+ ],
+ "schema-id": 1,
+ "identifier-field-ids": []
+ }`,
+ contains: "multiple fields for id 1: id and element",
+ },
+ {
+ name: "map key",
+ schema: `{
+ "type": "struct",
+ "fields": [
+ {"id": 1, "name": "id", "type": "long",
"required": true},
+ {
+ "id": 2,
+ "name": "props",
+ "type": {
+ "type": "map",
+ "key-id": 1,
+ "key": "string",
+ "value-id": 3,
+ "value": "int",
+ "value-required": false
+ },
+ "required": false
+ }
+ ],
+ "schema-id": 1,
+ "identifier-field-ids": []
+ }`,
+ contains: "multiple fields for id 1: id and key",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var schema iceberg.Schema
+ err := json.Unmarshal([]byte(tt.schema), &schema)
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ assert.ErrorContains(t, err, tt.contains)
+ })
+ }
+}
+
+func TestUnmarshalSchemaRejectsMissingOrZeroCollectionFieldIDs(t *testing.T) {
+ tests := []struct {
+ name string
+ schema string
+ contains string
+ }{
+ {
+ name: "missing list element id",
+ schema: `{
+ "type": "struct",
+ "fields": [
+ {
+ "id": 1,
+ "name": "items",
+ "type": {
+ "type": "list",
+ "element": "int",
+ "element-required":
false
+ },
+ "required": false
+ }
+ ],
+ "schema-id": 1,
+ "identifier-field-ids": []
+ }`,
+ contains: "field is missing required 'element-id' key
in JSON",
+ },
+ {
+ name: "zero list element id",
Review Comment:
Once the `== 0` checks come out, this row and "zero map value id" assert the
wrong outcome — they'd need to flip to `require.NoError`. While here: the table
never exercises zero map key-id or missing map value-id (both are real
branches), and there's no positive case proving a valid non-zero list/map
schema still unmarshals after the `*int` change — `tableSchemaNested` goes
through the constructor, not JSON decode, so a regression there wouldn't be
caught. I'd add a round-trip case asserting `require.NoError`.
##########
types.go:
##########
@@ -524,18 +531,31 @@ func (m *MapType) String() string {
func (m *MapType) UnmarshalJSON(b []byte) error {
aux := struct {
- KeyID int `json:"key-id"`
+ KeyID *int `json:"key-id"`
Key typeIFace `json:"key"`
- ValueID int `json:"value-id"`
+ ValueID *int `json:"value-id"`
Value typeIFace `json:"value"`
ValueReq *bool `json:"value-required"`
}{}
if err := json.Unmarshal(b, &aux); err != nil {
return err
}
- m.KeyID, m.KeyType = aux.KeyID, aux.Key.Type
- m.ValueID, m.ValueType = aux.ValueID, aux.Value.Type
+ if aux.KeyID == nil {
+ return fmt.Errorf("%w: field is missing required 'key-id' key
in JSON", ErrInvalidSchema)
+ }
+ if *aux.KeyID == 0 {
+ return fmt.Errorf("%w: field 'key-id' must not be 0",
ErrInvalidSchema)
+ }
+ if aux.ValueID == nil {
+ return fmt.Errorf("%w: field is missing required 'value-id' key
in JSON", ErrInvalidSchema)
+ }
+ if *aux.ValueID == 0 {
+ return fmt.Errorf("%w: field 'value-id' must not be 0",
ErrInvalidSchema)
+ }
Review Comment:
Same blocker on the map side — keep both nil checks, drop the two `== 0`
checks. Java's `mapFromJson` parses zero key-id/value-id fine, so rejecting
them breaks reading Java/Py/Rust-written maps the same way. The suggestion also
spaces the two guards out, which keeps nlreturn happy.
```suggestion
if aux.KeyID == nil {
return fmt.Errorf("%w: field is missing required 'key-id' key
in JSON", ErrInvalidSchema)
}
if aux.ValueID == nil {
return fmt.Errorf("%w: field is missing required 'value-id' key
in JSON", ErrInvalidSchema)
}
```
--
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]
