Revanth14 commented on PR #1492: URL: https://github.com/apache/iceberg-go/pull/1492#issuecomment-5054231331
> Did a thorough pass over the decoder, the codec extension, and the tests — this is in good shape. > > * Validation is careful and matches the existing manifest/Avro contract: `record-count`/`file-size` are rejected when `<= 0` (same as `NewDataFileBuilder`), and bounds are stored as raw `map[int][]byte` with typed decode deferred to `LiteralFromBytes` at evaluation time — exactly how manifest reads behave. > * Partition decode uses `field.Transform.ResultType(sourceType)`, consistent with `table/internal/utils.go`, and covers every primitive wire type (incl. hex for fixed/binary). > * The residual codec extension is genuinely backward/forward-compatible: a byte-stable Avro envelope + magic/uvarint trailer, an empty trailer decodes to a nil residual, and the `fileScanTaskShape` compile-time drift guard is a nice touch. > * Malformed-input coverage is excellent (20+ negative cases, plus codec error-marker/negative-range tests). > > A few non-blocking nits and two questions inline — nothing that must change, mostly parity confirmations. Happy to approve once you've had a look at the two questions (key-metadata encoding + unreferenced-delete handling). Thanks for the thorough review! I've addressed the two questions and the additional bounds-test suggestion: - Confirmed Java encodes `key-metadata` using base16 and added a focused fixture matching `TestContentFileParser`. - Unreferenced delete files are now rejected with a field-specific error, with test coverage intentionally stricter than Java, which silently ignores them at parse time (details in the inline thread). - Added `LiteralFromBytes` assertions for int, decimal, and timestamp bounds to pin endianness and type dispatch. -- 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]
