tanmayrauth commented on code in PR #1762:
URL: https://github.com/apache/iceberg-go/pull/1762#discussion_r3763686860
##########
manifest.go:
##########
@@ -1023,6 +1072,10 @@ func ReadManifestList(in io.Reader) ([]ManifestFile,
error) {
}
if version == 1 {
Review Comment:
`if version == 1 { if inferred > 1 { ... } }` - This only half-satisfies the
"key present must agree with the schema, error beats zeroed values" rule from
the issue. It catches a key claiming v1 over a v2+ schema, but not a key
claiming a *lower v2+* version than the schema carries: a file with key="2" but
a v3 schema (first_row_id present, inferred 3) reads with version=2 and no
error. first_row_id is read into the struct, but Version() reports 2, so a
read-modify-rewrite through WriteManifestList(2, …) uses the v2 writer schema
and silently drops v3 row lineage — the same corruption this PR fixes, one
version up. No real writer emits a key that undershoots its schema today
(DuckDB omits the key; Java/Py write consistent keys), so it's adversarial
rather than a field bug — but it's cheap to close the whole class: generalize
the check to `if version < inferred { return error }`, and add a
key=2/schema=v3 case to TestReadManifestListKeyContradictsSchema.
--
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]
