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.
##########
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:
`if id, ok := f.Props["field-id"].(float64); ok {` — 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.
--
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]
