zeroshade commented on code in PR #1823:
URL: https://github.com/apache/iceberg-go/pull/1823#discussion_r3832187952
##########
partitions.go:
##########
@@ -122,32 +122,41 @@ func (p *PartitionField) String() string {
}
func (p *PartitionField) UnmarshalJSON(b []byte) error {
- var raw map[string]json.RawMessage
- if err := json.Unmarshal(b, &raw); err != nil {
+ var aux struct {
+ SourceID json.RawMessage `json:"source-id"`
+ SourceIDs json.RawMessage `json:"source-ids"`
+ FieldID int `json:"field-id"`
+ Name string `json:"name"`
+ Transform json.RawMessage `json:"transform"`
+ }
+ if err := json.Unmarshal(b, &aux); err != nil {
Review Comment:
Decoding `FieldID` and `Name` in this first pass changes validation
precedence. For example, with both source fields plus `"field-id":"x"`, the
base decoder returns `ErrInvalidPartitionSpec`, but this now exits here with a
generic JSON type error, so `errors.Is(err, ErrInvalidPartitionSpec)` changes
from true to false. A null transform plus an invalid `field-id` similarly stops
matching `ErrInvalidTransform`. These are exported sentinels downstream callers
can inspect; please retain the previous semantic-check ordering.
`Direction`/`NullOrder` cause the equivalent change in `table/sorting.go`.
##########
partitions.go:
##########
@@ -122,32 +122,41 @@ func (p *PartitionField) String() string {
}
func (p *PartitionField) UnmarshalJSON(b []byte) error {
- var raw map[string]json.RawMessage
- if err := json.Unmarshal(b, &raw); err != nil {
+ var aux struct {
+ SourceID json.RawMessage `json:"source-id"`
Review Comment:
`encoding/json` matches struct tags case-insensitively, whereas the previous
`map[string]json.RawMessage` presence checks used exact keys. As a result, a
payload containing `"Source-Id"` (or uppercase `"TRANSFORM"`) was rejected by
the base decoder but is accepted here. Iceberg JSON member names are
case-sensitive, so please preserve exact-key behavior and add mixed-case
regression cases. The same issue exists in `table/sorting.go`.
##########
partitions.go:
##########
@@ -122,32 +122,41 @@ func (p *PartitionField) String() string {
}
func (p *PartitionField) UnmarshalJSON(b []byte) error {
- var raw map[string]json.RawMessage
- if err := json.Unmarshal(b, &raw); err != nil {
+ var aux struct {
+ SourceID json.RawMessage `json:"source-id"`
+ SourceIDs json.RawMessage `json:"source-ids"`
+ FieldID int `json:"field-id"`
+ Name string `json:"name"`
+ Transform json.RawMessage `json:"transform"`
+ }
+ if err := json.Unmarshal(b, &aux); err != nil {
return fmt.Errorf("%w: failed to unmarshal partition field",
err)
}
- if _, ok := raw["source-id"]; ok {
- if _, ok := raw["source-ids"]; ok {
- return fmt.Errorf("%w: partition field cannot contain
both source-id and source-ids", ErrInvalidPartitionSpec)
- }
+ hasSourceID := aux.SourceID != nil
+ hasSourceIDs := aux.SourceIDs != nil
+ if hasSourceID && hasSourceIDs {
+ return fmt.Errorf("%w: partition field cannot contain both
source-id and source-ids", ErrInvalidPartitionSpec)
}
- _, hasSourceID := raw["source-id"]
- _, hasSourceIDs := raw["source-ids"]
- if tf, ok := raw["transform"]; !ok || string(tf) == "null" {
+ if aux.Transform == nil || string(aux.Transform) == "null" {
return fmt.Errorf("%w: partition field requires a transform",
ErrInvalidTransform)
}
- aux := struct {
- SourceID int `json:"source-id"`
- SourceIDs []int `json:"source-ids,omitempty"`
- FieldID int `json:"field-id"`
- Name string `json:"name"`
- TransformString string `json:"transform"`
- }{}
-
- if err := json.Unmarshal(b, &aux); err != nil {
+ var sourceID int
+ if hasSourceID {
+ if err := json.Unmarshal(aux.SourceID, &sourceID); err != nil {
Review Comment:
Unmarshalling the `RawMessage` as a standalone value drops the field name
from type errors: malformed `source-id` used to identify `.source-id`, but now
only says it cannot unmarshal into a Go value of type `int`. Please wrap each
of these returns with the relevant JSON field name so malformed metadata
remains diagnosable; the same applies to the corresponding returns in
`table/sorting.go`.
--
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]
