tanmayrauth commented on code in PR #1762:
URL: https://github.com/apache/iceberg-go/pull/1762#discussion_r3763682824
##########
manifest.go:
##########
@@ -999,17 +999,66 @@ func ReadManifest(m ManifestFile, f io.Reader,
discardDeleted bool) ([]ManifestE
return results, nil
}
+// Manifest-list entry field IDs used to infer the format version from an
+// embedded writer schema. Per the Iceberg spec, content and sequence_number
+// are required fields for v2+ manifest lists and first_row_id is a v3 field;
+// none of them exist in v1.
+const (
+ fieldIDManifestSequenceNumber = 515
+ fieldIDManifestContent = 517
+ fieldIDManifestFirstRowID = 520
+)
+
+// schemaFieldID returns the Iceberg "field-id" property of an Avro schema
+// field, if present. Props hold decoded JSON, so numbers arrive as float64.
+func schemaFieldID(f avro.SchemaField) (int, bool) {
+ if id, ok := f.Props["field-id"].(float64); ok {
Review Comment:
The name fallback here answers the open question from the issue (writers
that don't annotate ids), and it's the right call to keep since DuckDB does
embed them, so it's just belt-and-suspenders. One robustness gap though: this
only accepts `float64`, while the existing field-id reader in this same file
(`manifest.go:476`, `switch v := field.Props["field-id"].(type)`) handles both
`int` and `float64`. For the read path here the writer schema is JSON-parsed
from the OCF header, so numbers come through as `float64` and the DuckDB
fixture proves it works — no live bug. But the two field-id readers now
disagree on accepted types, and that sibling `int` case exists because an
in-memory-built schema yields `int` props. If inference is ever handed such a
schema, `schemaFieldID` returns false and silently degrades to name-matching,
and a writer that annotates only field-ids (non-spec names) then misfires back
to the v1 fallback this PR removes. Worth mirroring the sibling type switch
to accept `int` too — ideally one shared helper both sites call so they can't
drift again.
##########
manifest.go:
##########
@@ -1023,6 +1072,10 @@ func ReadManifestList(in io.Reader) ([]ManifestFile,
error) {
}
if version == 1 {
Review Comment:
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]
