ryanworl opened a new pull request, #1762: URL: https://github.com/apache/iceberg-go/pull/1762
Fixes #1760 ## What `ReadManifestList` selected its Avro reader schema solely from the `format-version` file-metadata key, assuming v1 when the key is absent. Per the spec that key is required only for [manifest files](https://iceberg.apache.org/spec/#manifests); for [manifest lists](https://iceberg.apache.org/spec/#manifest-lists) it is optional, and real writers (DuckDB's iceberg extension ≤ 1.5.4) omit it. Reading a keyless v2/v3 list through the v1 reader schema made Avro schema resolution silently drop every writer field the v1 schema doesn't declare: `content` (field-id 517) read as 0 so delete manifests became invisible as deletes, `sequence_number` (515) / `min_sequence_number` (516) read as 0, and v3 `first_row_id` (520) was lost — with no error. Any read-modify-rewrite path (compaction, snapshot expiry, manifest rewriting) would persist that corruption. ## How Field-ID inference from the embedded writer schema, feeding the existing v1/v2+ decode paths: - Inside the reader-schema callback, the writer schema's top-level fields are inspected: `content` (517) or `sequence_number` (515) imply v2+, `first_row_id` (520) implies v3, neither means v1. Fields are discriminated primarily by their `field-id` prop — the stable contract per the spec (DuckDB embeds them) — with the spec field names as a fallback for writers that don't annotate IDs. - Key absent: the inferred version is used. A genuine keyless v1 list still reads as v1 (existing behavior, now pinned by a test). - Key present and consistent: unchanged behavior. - Key present but claiming v1 for a schema that carries v2+ fields: an error is returned instead of silently dropping writer fields. This matches how Java's `ManifestLists.read` and PyIceberg handle these files: they never consult the key and let field-ID resolution against the embedded writer schema decide what is present. The alternative design discussed in the issue — a Java-style single superset reader schema (v2+/v3 fields optional, defaulted in code) with no version branch at all — would also work and is arguably closer to Java; I went with inference as the smaller diff that feeds the existing v1/v2+ code paths. Happy to rework toward the superset approach if maintainers prefer it. ## Tests - Key-stripped v2 and v3 round-trips: written with `WriteManifestList` containing a delete-content manifest with non-zero sequence numbers (and an assigned `first_row_id` for v3), `format-version` stripped from the OCF header (data blocks byte-identical), read back asserting `content`, `sequence_number`, `min_sequence_number`, and `first_row_id` survive. - Keyless v1 list still decodes as v1, identical to the same list with the key. - Key claiming v1 over a v2/v3 schema returns an error. - A real DuckDB-written fixture (`testdata/duckdb_v3_manifest_list.avro`, 1.7 KB): produced by DuckDB v1.5.4's iceberg extension against a REST catalog — `format-version = '3'` table, one INSERT, one DELETE — and checked in. Under the previous fallback its delete manifest read as a data manifest with zeroed sequence numbers. `go test ./...` and `golangci-lint run` pass; the fixture is listed in `dev/release/rat_exclude_files.txt` alongside the existing binary test files. Made with [Cursor](https://cursor.com) -- 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]
