zeroshade commented on code in PR #1313:
URL: https://github.com/apache/iceberg-go/pull/1313#discussion_r3482957405
##########
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 validates that every entry in `SourceIDs` is present and positive -
good. The remaining gap is *existence in the schema*:
`SortOrder.CheckCompatibility` only looks up `field.SourceID()`, i.e. the first
source id (sorting.go:298), so a multi-arg sort field such as `SourceIDs: [1,
999]` still passes when `999` isn't in the schema. Worth iterating all of
`field.SourceIDs` there (looking each up via `FindFieldByID` and checking it's
primitive), with a regression case pairing a valid id and a nonexistent one.
--
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]
