rexminnis commented on PR #2620:
URL: https://github.com/apache/iceberg-rust/pull/2620#issuecomment-5420325212
+1 on moving mutable state to the Transaction level — the
`Mutex<MergingCache>`
placeholder in the revival branch was pointing at the same conclusion, and
per-action retry state owned by the transaction (blackmwk/iceberg-rust#19)
is a
cleaner home for it than a lock inside MSP. The staged, <1000-line plan looks
very reviewable.
Since the last update I've been exercising the revival branch against a live
REST catalog (Polaris) + S3, including a table that had evolved its partition
spec (`identity(ts)` → `day(ts)`) and needed entries removed from manifests
written under the *old* spec. That surfaced two findings I'd suggest baking
into the transaction-layer design as constraints, because both only show up
on
spec-evolved tables and neither is visible to single-spec tests:
1. **Filtered manifests must be written under their source spec.** A rewrite
that touches an old-spec manifest has to re-emit survivors with the spec
their partition values were shaped by — writing them under the table's
default spec mis-describes the partitioning (Java keeps filtered manifests
on their spec). Implemented here, replacing the earlier refusal:
https://github.com/rexminnis/iceberg-rust/commit/393915cf — the writer is
parameterized by spec id, and an id absent from table metadata is a
`DataInvalid` error rather than a blanket `FeatureUnsupported`.
2. **Snapshot-summary partition metrics must resolve each removed file's own
spec.** Pathing an old-spec partition struct through the default spec's
transforms panics in `Datum`'s `Display` (an `unreachable!` arm) — hit
live
with a timestamptz identity partition under a `day()` default spec. Fix:
https://github.com/rexminnis/iceberg-rust/commit/7462c615.
Design implication for the retry cache specifically: any cached
manifest-filtering result needs to be keyed (or at least tagged) per source
spec — two manifests with identical content but different spec ids are not
interchangeable artifacts.
On V4: the filter *semantics* (drop removed entries, re-emit survivors as
EXISTING with original snapshot/sequence, omit emptied manifests, fail on
missing delete paths) look format-stable to me — it's the writer plumbing
that
V4 changes. Keeping the filter behind an interface with the writer injected
would let the V4 migration swap the bottom layer without touching the
semantics or their tests.
Happy to rebase these (plus the earlier summary-subtraction fix) as small
stacked PRs wherever they fit best in the new plan, and I have a reusable
live-verification recipe (REST catalog + object store + independent Java
reader checking the result) that could serve as the epic's integration-test
story if useful.
--
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]
