mzzz-zzm commented on PR #982: URL: https://github.com/apache/iceberg-go/pull/982#issuecomment-4398306629
**Heads-up — proactive note** While addressing the six review comments I noticed a fragility in `addSnapshotUpdate.Apply` that I think is worth flagging. It isn't covered by your review and I couldn't find an existing issue for it, so surfacing it here. The rebuild closure (`rebuildManifestList`) and `ownManifests` are runtime-only fields on `*addSnapshotUpdate`. When `Apply` runs, it calls `builder.AddSnapshot(u.Snapshot)`, which internally constructs a **new** `*addSnapshotUpdate` via `NewAddSnapshotUpdate(snapshot)` and appends it to `builder.updates`. That new object has the closure fields zeroed. To make the fix work, `Apply` reaches back into `builder.updates[n-1]` and copies the closure onto that fresh object. This works today, but it depends on `MetadataBuilder.AddSnapshot` always appending exactly **one** `*addSnapshotUpdate` as the last element. If `AddSnapshot` is ever extended to append a helper update afterward (e.g. for v3 row-lineage tracking or schema validation), `builder.updates[n-1]` is no longer our snapshot update — the type assertion silently fails, the closure is dropped, and the entire OCC rebuild stops running with no error or log. Same "silent fix-disable" shape as the `WriteFileIO` cast you flagged in Comment 5. The cleaner shape would be to have `MetadataBuilder.AddSnapshot` take `*addSnapshotUpdate` directly and store it as-is, so the back-door goes away and `Apply` becomes a one-liner. I kept the current approach to minimize blast radius, but happy to make the change in this PR or a follow-up -- 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]
