laskoviymishka commented on code in PR #2069:
URL: https://github.com/apache/iceberg-go/pull/2069#discussion_r4145267197
##########
puffin/puffin_reader.go:
##########
@@ -510,6 +514,10 @@ func (r *Reader) readFooter() error {
// content deliberately, even though some other Iceberg implementations
// accept padding or additional values inside the footer payload.
if decoder.More() {
+ if compressedFooter != nil && compressedFooter.err != nil {
Review Comment:
Neither of these new branches is exercised by the current test.
`TestReaderPreservesLZ4ChecksumError` runs with the default 64MB max footer, so
the `limitedFooter.N == 0` branch at line 505 is never hit, and the assertion
is only on the `invalid frame checksum` substring, so it can't tell which site
actually surfaced the error (Site 3's pre-existing `Token()` guard would
satisfy it just as well). I'd add a case with
`WithMaxFooterSize(int64(len(payload)))` and a corrupted checksum, and pin the
full wrapped prefix, so we know these additions are what's covering the error
rather than the guard that was already there.
##########
puffin/puffin_reader.go:
##########
@@ -502,6 +502,10 @@ func (r *Reader) readFooter() error {
return fmt.Errorf("puffin: read buffered footer JSON:
%w", err)
}
if len(bytes.TrimSpace(buffered)) > 0 {
+ if compressedFooter != nil && compressedFooter.err !=
nil {
Review Comment:
The checksum error only survives if every trailing-content return remembers
to check `compressedFooter.err`, and we've now had to patch that in after the
fact twice. Add a fourth trailing-content return down the line and it silently
drops the error again, which is the exact regression this PR is closing. I'd
move the check to a single site right after `Decode` returns nil, before any of
the trailing-content logic, so it's structural instead of something each new
return has to remember. A small `checkCompressedFooterErr(compressedFooter)`
helper is a fine alternative if you'd rather keep the per-site shape.
Minor and moot if you collapse to the single check: inside this
`limitedFooter != nil && N == 0` branch `compressedFooter` is always non-nil
(both are only set past the compressed-footer gate), so the `!= nil` arm here
is dead; it's only load-bearing at the sites outside that branch.
--
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]
