zeroshade commented on code in PR #1381:
URL: https://github.com/apache/iceberg-go/pull/1381#discussion_r3523577661
##########
table/sorting.go:
##########
@@ -184,8 +184,8 @@ func (s *SortField) UnmarshalJSON(b []byte) error {
}
func validateSortSourceID(id int) error {
- if id < 0 {
- return fmt.Errorf("source ID must be non-negative: %d", id)
+ if id <= 0 {
Review Comment:
[MAJOR] This check now rejects zero only when validation is invoked, but the
JSON unmarshal path still bypasses it: SortField.UnmarshalJSON stores the
decoded source IDs without presence/positivity validation, and
SortOrder.UnmarshalJSON calls newSortOrder(..., false). A missing source-id can
still decode leniently and later marshal as source-id: 0, so the round-trip
safety issue remains. Please adopt #1367's unmarshal-time presence/positivity
checks, or explicitly document that unmarshal is intentionally lenient and
validation is deferred. Anchored on the nearest changed line; the affected code
is in SortField.UnmarshalJSON and SortOrder.UnmarshalJSON.
##########
table/sorting_test.go:
##########
@@ -185,7 +168,13 @@ func
TestSortOrderCheckCompatibilityRejectsInvalidSourceIDs(t *testing.T) {
{
name: "negative",
jsonData: `{"order-id": 1, "fields":
[{"source-id": -1, "transform": "identity", "direction": "asc", "null-order":
"nulls-first"}]}`,
- wantErr: "source ID must be non-negative:
-1",
+ wantErr: "source ID must be positive: -1",
+ wantInvalidSourceID: true,
+ },
+ {
+ name: "zero",
+ jsonData: `{"order-id": 1, "fields":
[{"source-id": 0, "transform": "identity", "direction": "asc", "null-order":
"nulls-first"}]}`,
Review Comment:
[MINOR] This test still encodes deferred validation: malformed sort source
IDs unmarshal successfully and are only rejected later by CheckCompatibility.
That directly contradicts #1367 and the stated goal to validate source IDs
during unmarshal. Please replace these with rejection tests like #1367, or
document that lenient unmarshal is intentional. Anchored on the nearest changed
line in the current diff.
--
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]
