Conversation
0415221 to
15ecc3b
Compare
laskoviymishka
left a comment
There was a problem hiding this comment.
The fix is correct and valid files aren't affected. My hesitation is that it patches the shape rather than the root: the checksum error only survives if every trailing-content return remembers to check compressedFooter.err, and we've had to add that after the fact twice now. The next trailing-content return someone adds drops the error again, which is the exact regression this closes. I'd move it to a single check right after Decode returns nil, before the trailing-content logic, so it's structural instead of per-return (a small helper's a fine alternative if you'd rather keep the per-site shape).
Before merge I'd also want coverage for it: the current test uses the default 64MB max footer, so the limitedFooter.N == 0 branch these changes guard is never reached, and it only asserts the invalid frame checksum substring so it can't tell which site fired. A WithMaxFooterSize(len(payload)) case with a corrupted checksum, pinning the wrapped prefix, would actually exercise the new branches. Details inline.
| return fmt.Errorf("puffin: read buffered footer JSON: %w", err) | ||
| } | ||
| if len(bytes.TrimSpace(buffered)) > 0 { | ||
| if compressedFooter != nil && compressedFooter.err != nil { |
There was a problem hiding this 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.
| // 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 { |
There was a problem hiding this 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.
|
#2077 has this fix and is part of bumping up to 1.26. Closing this |
Rationale for the change
puffin/TestReaderPreservesLZ4ChecksumErrorfails on Go 1.27.1 (passes on Go 1.25.9):In
Reader.readFooter, three paths return "unexpected content after footer JSON", butonly one first checked
compressedFooter.err(the LZ4 read error, e.g.invalid frame checksum). Which path runs depends on how far Go'sencoding/jsondecoder readsahead - which changed between Go versions - so on Go 1.27.1 a real checksum failure
was reported as "unexpected content" instead.
Change
Check
compressedFooter.errat all three return sites, not just one. Valid files areunaffected (the error is
nil, so trailing content still reports "unexpected contentafter footer JSON").
Test
gofmt/go vetclean;go test ./puffin/ok on both go1.25.9 and go1.27.1 (was FAILon 1.27.1 before this change).