zeroshade commented on code in PR #1514:
URL: https://github.com/apache/iceberg-go/pull/1514#discussion_r3647331336
##########
partitions.go:
##########
@@ -422,15 +422,47 @@ func (ps PartitionSpec) MarshalJSON() ([]byte, error) {
func (ps *PartitionSpec) UnmarshalJSON(b []byte) error {
aux := struct {
- ID int `json:"spec-id"`
- Fields []PartitionField `json:"fields"`
- }{ID: ps.id, Fields: ps.fields}
+ ID int `json:"spec-id"`
+ Fields []json.RawMessage `json:"fields"`
+ }{ID: ps.id}
if err := json.Unmarshal(b, &aux); err != nil {
return err
}
- ps.id, ps.fields = aux.ID, aux.Fields
+ fields := make([]PartitionField, len(aux.Fields))
+ fieldIDCount := 0
+ for i, rawField := range aux.Fields {
+ var keys map[string]json.RawMessage
+ if err := json.Unmarshal(rawField, &keys); err != nil {
+ return err
+ }
+ if rawFieldID, ok := keys["field-id"]; ok {
+ var fieldID *int
+ if err := json.Unmarshal(rawFieldID, &fieldID); err !=
nil {
+ return fmt.Errorf("%w: invalid partition field
ID: %w", ErrInvalidPartitionSpec, err)
+ }
+ if fieldID == nil {
+ return fmt.Errorf("%w: partition field ID
cannot be null", ErrInvalidPartitionSpec)
+ }
+ fieldIDCount++
+ }
+ if err := json.Unmarshal(rawField, &fields[i]); err != nil {
+ return err
+ }
+ }
+
+ if fieldIDCount != 0 && fieldIDCount != len(fields) {
+ return fmt.Errorf("%w: cannot parse spec with missing field
IDs: %d missing of %d fields",
+ ErrInvalidPartitionSpec, len(fields)-fieldIDCount,
len(fields))
+ }
+ if fieldIDCount == 0 {
Review Comment:
All-missing assigns `PartitionDataIDStart + i` per spec. With no table-level
`last-partition-id`/existing-id context, multiple specs parsed from the same
metadata each assign 1000, 1001, … and collide across the table-wide
partition-field-id space (also watch for collision with an existing field
already using 1000/1001).
##########
partitions.go:
##########
@@ -422,15 +422,47 @@ func (ps PartitionSpec) MarshalJSON() ([]byte, error) {
func (ps *PartitionSpec) UnmarshalJSON(b []byte) error {
aux := struct {
- ID int `json:"spec-id"`
- Fields []PartitionField `json:"fields"`
- }{ID: ps.id, Fields: ps.fields}
+ ID int `json:"spec-id"`
+ Fields []json.RawMessage `json:"fields"`
+ }{ID: ps.id}
if err := json.Unmarshal(b, &aux); err != nil {
return err
}
- ps.id, ps.fields = aux.ID, aux.Fields
+ fields := make([]PartitionField, len(aux.Fields))
+ fieldIDCount := 0
+ for i, rawField := range aux.Fields {
+ var keys map[string]json.RawMessage
+ if err := json.Unmarshal(rawField, &keys); err != nil {
+ return err
+ }
+ if rawFieldID, ok := keys["field-id"]; ok {
+ var fieldID *int
+ if err := json.Unmarshal(rawFieldID, &fieldID); err !=
nil {
+ return fmt.Errorf("%w: invalid partition field
ID: %w", ErrInvalidPartitionSpec, err)
+ }
+ if fieldID == nil {
+ return fmt.Errorf("%w: partition field ID
cannot be null", ErrInvalidPartitionSpec)
+ }
+ fieldIDCount++
+ }
+ if err := json.Unmarshal(rawField, &fields[i]); err != nil {
+ return err
+ }
+ }
+
+ if fieldIDCount != 0 && fieldIDCount != len(fields) {
Review Comment:
Mixed set/unset partition field ids are rejected here. Expected behavior is
to preserve existing ids and assign fresh ids only to the missing fields.
--
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]
