laskoviymishka commented on code in PR #1613:
URL: https://github.com/apache/iceberg-go/pull/1613#discussion_r3719183445
##########
catalog/rest/scan_task_decoder.go:
##########
@@ -343,6 +343,9 @@ func decodeRESTDeleteFile(
if len(wire.EqualityIDs) != 0 {
return nil, errors.New("position-deletes file must not
carry equality-ids")
}
+ if format != iceberg.PuffinFile && (wire.ContentOffset != nil
|| wire.ContentSizeInBytes != nil) {
Review Comment:
I think this guard is over-strict and I'd drop it. The REST OpenAPI schema
puts `content-offset`/`content-size-in-bytes` on `PositionDeleteFile` with no
`file-format: puffin` constraint, so a conformant server can legally send them
on a non-Puffin position delete, and we'd now reject that response.
Both reference impls already accept it. Java's `ContentFileParser.fromJson`
reads both fields off any delete file with no PUFFIN check, and PyIceberg's
`test_scan_planning_models` fixture builds a `parquet` position-delete with
both fields set and validates it fine. So we'd be the only client hard-failing
on a wire response Java can produce.
The existing code just below already enforces the invariant we actually care
about: the `format == PuffinFile` block requires these fields and fills
`ReferencedDataFile`, and the non-Puffin path accepts them optionally with the
value checks (size required when offset present, non-negative). Dropping the
guard restores parity without losing any of that, and the three new test cases
go with it.
Am I missing a spec reading that justifies the stricter rule here? wdyt?
--
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]
