tanmayrauth commented on issue #1760:
URL: https://github.com/apache/iceberg-go/issues/1760#issuecomment-5256420888
Thanks for the detailed writeup and the self-contained repro, this is a
real correctness bug and the analysis is right. Confirmed on main:
ReadManifestList (manifest.go:1008-1040) selects the reader schema purely from
the optional format-version metadata key and falls back to v1. When a v2/v3
list omits that key, the v1 reader schema is resolved against the v2/v3 writer
schema and Avro drops the writer fields the v1 schema doesn't declare — so
content reads as data (0), sequence_number/min_sequence_number read as 0, and
v3 first_row_id is lost, with no error. Agreed on severity: any
read-modify-rewrite path (compaction, snapshot expiry, manifest rewriting)
would persist that corruption and hide delete manifests.
Root cause is treating format-version as authoritative for manifest lists.
Per spec that key is only required on manifest files, so keying off it is
unsound, and a writer like DuckDB legitimately omits it.
For reference, Java's ManifestLists.read never looks at format-version —
it projects onto the full ManifestFile.schema() (where
content/sequence_number/first_row_id are all optional) and lets field-ID
resolution against the embedded writer schema decide what's present, defaulting
absent fields in code (content → DATA). That's why Java and PyIceberg read
these files correctly regardless of the key.
Your schema-inference approach is a good direction, and I'd anchor it on
the embedded writer schema (reachable in the reader callback) rather than the
metadata key, discriminating on field IDs — equence_number(515)/content(517) ⇒
v2+, first_row_id(520) ⇒ v, since IDs are the stable contract.
Two things worth settling before a PR:
- Do we want field-ID inference feeding the existing v1/v2+ paths, or to
move toward the Java model (single superset reader schema + in-code defaults,
no version branch)? Curious what the maintainers prefer.
- Field-ID inference assumes writers embed field-id props on the Avro
schema — do we need a name-based fallback for writers that don't? If you've
looked at what DuckDB emits, that'd be useful to know.
Either way, when the key is present it should still agree with the schema,
and we should never silently drop writer fields — an error there beats zeroed
values. We should also keep the existing behavior where a genuine v1 list
without the key still reads as v1.
A PR would be very welcome, ideally with the key-stripped v2/v3 round-trip
from your repro as regression coverage, plus a real DuckDB-written v3 fixture
if feasible. Happy to review.
--
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]
