sevbanbayrak commented on PR #2071: URL: https://github.com/apache/iceberg-go/pull/2071#issuecomment-5913306379
Thanks for the careful review — all points addressed in 12b915c. **Blocking** - `ErrNotPuffinFile` is now magic-first: `NewReader` reads the header magic before the minimum-size guard and returns the sentinel only when the file is shorter than 4 bytes (cannot show the magic) or the leading bytes are not `PFA1`. A file that starts with the magic but is truncated keeps the plain `puffin: file too small` error, so a partial upload of a real Puffin file never takes the bare-blob path. The existing "file too small" case in `puffin_test.go` was rewritten to use a valid-magic short file, plus a new too-short-for-magic case. - New `TestReadDVTruncatedPuffinDoesNotFallBack`: magic-only, truncated-footer and corrupt-footer files fail in the Puffin reader and the error does not wrap `ErrNotPuffinFile` (and does not carry the "not a Puffin container" message). **Inline** - Double `Open`: `openDVReader` now returns the still-open `iceio.File` with a nil reader on the not-Puffin case; `readBareDVs` takes that handle. Callers keep the single `defer f.Close()`. - Warning flood / ordering: logged once per distinct file path (`sync.Map`), and only after the first blob of that file has been read and decoded. - Redundant `validateDVFile`: removed from `readBareDVs`; the function is documented as requiring pre-validated entries (both callers validate before dispatch). - Read order: `readBareDVs` sorts by `content_offset` and restores input order in the result; covered by a reversed-input subtest. - Missing test: added a subtest that flips the blob's CRC bytes and asserts `ErrInvalidDeletionVector` with the "blob at offset" wrap. **Description**: added the two notes you suggested (Go validates footer metadata Java never checks; PyIceberg/iceberg-rust have no bare-blob path yet). Re-verified after these changes against a fresh Databricks `IcebergCompatV3` table with two DVs through the UC REST catalog: 999,000 rows / 1,000 updated markers, matching SQL. `go test ./puffin/ ./table/dv/`, `go vet`, `make lint` clean. -- 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]
