rexminnis commented on PR #2620: URL: https://github.com/apache/iceberg-rust/pull/2620#issuecomment-5676573223
The rewrite reads well. Trading the numbered invariants for the three-lifetimes table (section 2.1) plus the preserve/recompute table (section 5.1) keeps the same guarantees with much less surface to keep consistent, and section 6's write-once rule for generated metadata paths resolves the retry-overwrite concern from the previous draft more cleanly than an ordering invariant would have. Two items from the earlier draft that I think still need a home now that the phase-level test lists are gone: 1. **Chained totals (section 8.1 step 5 verification).** `total-data-files` / `total-records` chain from the parent, so a summary built from additions alone is a permanent over-count after any removal-bearing snapshot. Section 5.1 correctly says the summary is recomputed per attempt, but the step 5 verification list should include asserting the chained totals after a removal-bearing commit, not only the per-snapshot `added-*` / `deleted-*` counts. Cheap test, and the failure mode is silent. 2. **Partition spec of a filtered manifest (section 4.3 step 2).** Section 8.2 defers per-spec bookkeeping, which is fine for the cache side, but "applies removals through manifest filtering" carries one rule that isn't deferrable: `M1′` must be written with `M1`'s partition spec, never re-encoded into the table's current default. Manifests are single-spec, and on a spec-evolved table the default-spec writer cannot represent the old partition values. One sentence in section 4.3 would pin it without pulling the cache design into the RFC. Both are semantics the merging-producer and filter-manager steps will hit regardless of which operation consumes them first, so pinning them now saves a round-trip later. -- 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]
