laskoviymishka commented on code in PR #2361:
URL: https://github.com/apache/iceberg-go/pull/2361#discussion_r4431324458
##########
types.go:
##########
@@ -431,13 +431,28 @@ func (s *StructType) Equals(other Type) bool {
func (s *StructType) Fields() []NestedField { return s.FieldList }
-func (s *StructType) MarshalJSON() ([]byte, error) {
- type Alias StructType
+// The JSON methods of the nested types name every serialized field instead of
+// encoding through an alias of the type, so the wire format does not depend on
+// which Go fields are exported.
+func (s *StructType) MarshalJSON() ([]byte, error) {
Review Comment:
Could we give `Schema` the same explicit JSON in this PR?
`Schema.MarshalJSON` and `UnmarshalJSON` still go through `type Alias Schema`
for `identifier-field-ids`
([schema.go#L357-L367](https://github.com/apache/iceberg-go/blob/e4170bb71586137d6dce85059b7eef46ae7be449/schema.go#L357-L367)).
#2062 plans to unexport `Schema.IdentifierFieldIDs`, but its prerequisite list
only names the three composite types. `TestSchemaRoundTrip` would catch the
dropped key, so this is about doing it in the same step rather than finding it
later.
##########
types_test.go:
##########
@@ -304,6 +304,107 @@ func TestMapTypeUnmarshalRequiresValue(t *testing.T) {
assert.ErrorContains(t, err, "missing required 'value' key")
}
+// nestedTypeWireFixture nests every composite type inside the others, with
+// required and optional children, docs and defaults on nested fields.
+func nestedTypeWireFixture() *iceberg.StructType {
+ return &iceberg.StructType{FieldList: []iceberg.NestedField{
+ {ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64,
Required: true},
+ {ID: 2, Name: "tags", Doc: "labels", Type: &iceberg.ListType{
+ ElementID: 3, Element: iceberg.PrimitiveTypes.String,
ElementRequired: false,
+ }},
+ {ID: 4, Name: "attrs", Required: true, Type: &iceberg.MapType{
+ KeyID: 5, KeyType: iceberg.PrimitiveTypes.String,
+ ValueID: 6, ValueType: &iceberg.StructType{FieldList:
[]iceberg.NestedField{
+ {ID: 7, Name: "score", Type:
iceberg.PrimitiveTypes.Float64, Required: true},
+ {ID: 8, Name: "note", Type:
iceberg.PrimitiveTypes.String, InitialDefault: "n/a", WriteDefault: "none"},
+ }},
+ ValueRequired: true,
+ }},
+ {ID: 9, Name: "points", Type: &iceberg.ListType{
+ ElementID: 10, ElementRequired: true, Element:
&iceberg.StructType{FieldList: []iceberg.NestedField{
+ {ID: 11, Name: "x", Type:
iceberg.PrimitiveTypes.Float32, Required: true},
+ {ID: 12, Name: "y", Type:
iceberg.PrimitiveTypes.Float32, Required: true},
+ }},
+ }},
+ {ID: 13, Name: "flags", Type: &iceberg.MapType{
+ KeyID: 14, KeyType: iceberg.PrimitiveTypes.Int32,
+ ValueID: 15, ValueType: &iceberg.ListType{
+ ElementID: 16, Element:
iceberg.PrimitiveTypes.Bool, ElementRequired: false,
+ },
+ ValueRequired: false,
+ }},
+ {ID: 17, Name: "active", Type: iceberg.PrimitiveTypes.Bool,
InitialDefault: true},
+ }}
+}
+
+const nestedTypeWireJSON = `{"type":"struct","fields":[` +
+ `{"type":"long","id":1,"name":"id","required":true},` +
+
`{"type":{"type":"list","element-id":3,"element-required":false,"element":"string"},`
+
+ `"id":2,"name":"tags","required":false,"doc":"labels"},` +
+
`{"type":{"type":"map","key-id":5,"value-id":6,"value-required":true,"key":"string",`
+
+ `"value":{"type":"struct","fields":[` +
+ `{"type":"double","id":7,"name":"score","required":true},` +
+
`{"type":"string","id":8,"name":"note","required":false,"initial-default":"n/a","write-default":"none"}]}},`
+
+ `"id":4,"name":"attrs","required":true},` +
+ `{"type":{"type":"list","element-id":10,"element-required":true,` +
+ `"element":{"type":"struct","fields":[` +
+ `{"type":"float","id":11,"name":"x","required":true},` +
+ `{"type":"float","id":12,"name":"y","required":true}]}},` +
+ `"id":9,"name":"points","required":false},` +
+
`{"type":{"type":"map","key-id":14,"value-id":15,"value-required":false,"key":"int",`
+
+
`"value":{"type":"list","element-id":16,"element-required":false,"element":"boolean"}},`
+
+ `"id":13,"name":"flags","required":false},` +
+
`{"type":"boolean","id":17,"name":"active","required":false,"initial-default":true}]}`
+
+// TestNestedTypeJSONWireFormat pins the exact bytes, key order included, that
+// nested types encode to, and that decoding them rebuilds an equal type.
+func TestNestedTypeJSONWireFormat(t *testing.T) {
Review Comment:
Could we add a guard that walks the fields of `StructType`, `ListType`,
`MapType` and `NestedField` and checks that each one survives a round trip? The
pinned bytes here only cover fields the fixture sets, so a field added later
goes unchecked. `TestMarshalSchemaIncludesExportedFields`
([schema_test.go#L683-L710](https://github.com/apache/iceberg-go/blob/e4170bb71586137d6dce85059b7eef46ae7be449/schema_test.go#L683-L710))
does this for `Schema`, but it skips unexported fields, so a copy would need
to be an internal test to keep working after #2062.
##########
types.go:
##########
@@ -431,13 +431,28 @@ func (s *StructType) Equals(other Type) bool {
func (s *StructType) Fields() []NestedField { return s.FieldList }
-func (s *StructType) MarshalJSON() ([]byte, error) {
- type Alias StructType
+// The JSON methods of the nested types name every serialized field instead of
+// encoding through an alias of the type, so the wire format does not depend on
+// which Go fields are exported.
+func (s *StructType) MarshalJSON() ([]byte, error) {
return json.Marshal(struct {
- Type string `json:"type"`
- *Alias
- }{Type: s.Type(), Alias: (*Alias)(s)})
+ Type string `json:"type"`
+ Fields []NestedField `json:"fields"`
+ }{Type: s.Type(), Fields: s.FieldList})
+}
+
+func (s *StructType) UnmarshalJSON(b []byte) error {
Review Comment:
Should `NestedField.UnmarshalJSON` move to the same aux-struct shape? It
still decodes `name`, `doc` and the defaults through `type Alias NestedField`
([types.go#L376-L415](https://github.com/apache/iceberg-go/blob/e4170bb71586137d6dce85059b7eef46ae7be449/types.go#L376-L415)),
while its `MarshalJSON` names every key. `main` has the same code and this PR
doesn't touch it, and zeroshade's [plan on
#2062](https://github.com/apache/iceberg-go/issues/2062#issuecomment-6476832377)
keeps those fields exported, so nothing breaks today. The question is only
whether we want both directions off the tags.
##########
types.go:
##########
@@ -431,13 +431,28 @@ func (s *StructType) Equals(other Type) bool {
func (s *StructType) Fields() []NestedField { return s.FieldList }
-func (s *StructType) MarshalJSON() ([]byte, error) {
- type Alias StructType
+// The JSON methods of the nested types name every serialized field instead of
+// encoding through an alias of the type, so the wire format does not depend on
+// which Go fields are exported.
+func (s *StructType) MarshalJSON() ([]byte, error) {
return json.Marshal(struct {
- Type string `json:"type"`
- *Alias
- }{Type: s.Type(), Alias: (*Alias)(s)})
+ Type string `json:"type"`
+ Fields []NestedField `json:"fields"`
+ }{Type: s.Type(), Fields: s.FieldList})
+}
+
+func (s *StructType) UnmarshalJSON(b []byte) error {
+ aux := struct {
+ Fields []NestedField `json:"fields"`
+ }{}
+ if err := json.Unmarshal(b, &aux); err != nil {
+ return err
+ }
+
+ s.FieldList = aux.Fields
Review Comment:
Is full replacement intended when `fields` is missing? Decoding
`{"type":"struct"}` into a populated `StructType` now sets `FieldList` to nil,
where `main` kept the existing list. Only direct callers can see this, since
`typeIFace` always decodes into a fresh `&StructType{}`
([types.go#L286-L292](https://github.com/apache/iceberg-go/blob/e4170bb71586137d6dce85059b7eef46ae7be449/types.go#L286-L292)).
`TestStructTypeUnmarshalMissingFieldsIsEmpty` decodes into a zero value, so it
passes either way. A case that decodes into a populated value would pin
whichever we pick.
--
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]
