laskoviymishka commented on code in PR #1313:
URL: https://github.com/apache/iceberg-go/pull/1313#discussion_r3490763459
##########
table/sorting.go:
##########
@@ -271,6 +303,9 @@ func NewSortOrder(orderID int, fields []SortField)
(SortOrder, error) {
fields = []SortField{}
}
for idx, field := range fields {
+ if err := validateSortSourceIDs(field.SourceIDs); err != nil {
Review Comment:
This changes the error a caller sees for `SortField{SourceIDs: nil,
Transform: nil}` from `ErrInvalidTransform` to `ErrInvalidSortSourceID`, since
the source-id check now runs first. `NewSortOrder` is public with named
sentinels, so anyone doing `errors.Is(err, ErrInvalidTransform)` on
programmatic construction quietly breaks — worth documenting the new validation
order in the godoc, or reordering so transform stays first.
While here: `fmt.Errorf("%w: sort field at index %d", err, idx)` wraps an
already-wrapped `err`, so the message reads "invalid sort source ID: source-ids
must not be empty: sort field at index 0" with the index trailing. The
transform line right below it puts the sentinel first in a self-contained
message — I'd flatten this to match.
##########
table/sorting.go:
##########
@@ -180,6 +190,28 @@ func (s *SortField) UnmarshalJSON(b []byte) error {
return nil
}
+func validateSortSourceID(id int) error {
+ if id <= 0 {
Review Comment:
I'd drop the `id <= 0` guard from the parse path — it's stricter than the
spec. The spec doesn't reserve source-id 0 for sort fields (only order-id 0,
for unsorted), and Java's `SortOrderParser`, iceberg-rust, and PyIceberg all
accept it with no range check.
Since this fires in `UnmarshalJSON`, a table another client wrote with
source-id 0 becomes unreadable here — we'd fail to load the metadata at all. If
we want to reject nonsensical *writes*, the place for it is
`CheckCompatibility` or `MarshalJSON`, not the reader. wdyt?
##########
table/sorting.go:
##########
@@ -133,10 +134,13 @@ func (s *SortField) UnmarshalJSON(b []byte) error {
return fmt.Errorf("%w: failed to unmarshal sort field", err)
}
- if _, ok := raw["source-id"]; ok {
- if _, ok := raw["source-ids"]; ok {
- return errors.New("sort field cannot contain both
source-id and source-ids")
- }
+ _, hasSourceID := raw["source-id"]
+ _, hasSourceIDs := raw["source-ids"]
+ if hasSourceID && hasSourceIDs {
+ return errors.New("sort field cannot contain both source-id and
source-ids")
+ }
+ if !hasSourceID && !hasSourceIDs {
+ return fmt.Errorf("%w: sort field must contain source-id or
source-ids", ErrInvalidSortSourceID)
Review Comment:
V3 marks both `source-id` and `source-ids` as `optional` in the sort field
JSON, so a reader hard-failing when both are absent isn't spec-compliant — the
"single-arg writes source-id, multi-arg writes source-ids" rule is a writer
obligation, not a reader parse-rejection rule.
Same interop risk as the zero-check: a V3 writer that omits both (a future
extension, or a quirk in another client) makes the whole table unreadable here.
I'd let `SourceIDs` stay empty through the parse and let `CheckCompatibility`
reject it downstream. Keep the `hasSourceID && hasSourceIDs` rejection though —
that one's genuinely malformed.
--
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]
