rexminnis commented on PR #2620: URL: https://github.com/apache/iceberg-rust/pull/2620#issuecomment-5492365234
Strong direction — the lifetime table plus testable invariants is exactly the discussion #2620 needed, and the artifact-lifecycle asymmetry in section 6 is a real improvement over Java's unknown-outcome dead-end. Three things I'd tighten, two from live-testing the earlier revival branch against a REST catalog + object store: 1. Phase 4's summary test should assert the *chained* totals (`total-data-files`/`total-records`) after a rewrite — we hit a live over-count from added-only summaries, and totals chain, so the error is permanent. 2. Step 2 of the merging pipeline (section 5.2) should state which partition spec a rewritten manifest is written with — manifests are single-spec, and preserving the *source* manifest's spec (never re-encoding into the default) is the rule the "evolved-spec constraints" bullet is hinting at. 3. Stable `commit_uuid` means attempt N+1 regenerates the same object keys as attempt N — safe after a confirmed failure, but after an *unknown* outcome that actually landed, same-name regeneration overwrites referenced metadata. The resolve-on-refresh path in section 6.3 covers it only if resolution is mandatory before any retry write; I'd promote that ordering to an invariant. Also worth cross-linking blackmwk's staged branches — they track these phases closely and would make a good reference implementation for the plan. Happy to re-run the live REST-catalog verification against Phase 4 when it lands. -- 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]
