hotcache commented on PR #3046:
URL: https://github.com/apache/iceberg-rust/pull/3046#issuecomment-5870613587
Rebased on main and worked through the review.
@jopdorp did most of it in `hotcache/iceberg-rust#1` — one commit per point,
each
with a test that fails on the code before it. His commits are here with their
authorship; when this is squashed, please keep:
Co-authored-by: Jegor van Opdorp <[email protected]>
**@JanKaul** — both inline comments are fixed in `7fdc8fd`. The compaction
and
partial-rewrite tests now run against V2 (`6fcdb66`) and V1 (`69aa1b3`) as
well,
sharing the same read-back assertions.
**@rexminnis** — (1) survivors go through `add_existing_entry` and keep their
status, snapshot id and both sequence numbers (`83ac7a5`); (2)
`data_sequence_number()`, bounded by the new snapshot's (`6fc7bf1`); (4) both
producer validations run before a rewrite commits (`9d40b4c`); (5) the
counter
moved onto `MergingSnapshotProducer`, which the action owns across retries,
so
attempt 2 cannot overwrite attempt 1's manifest (`d466a48`); (6) all-deleted
manifests drop out (`ee3fbdd`). (3) is interim — see below.
Two notes. On (1), `add_entry` also left the entry's sequence number `None`,
so
`ManifestWriter` never updated `min_seq_num` and the manifest list stamped
the
rewritten manifest with the *new* snapshot's `min_sequence_number` —
delete-file
pruning was reading a wrong lower bound. On (5) and (6), `fast_append` has
the
same two shapes and is untouched here (#2545 for the latter).
### The interim conflict check
`4f26afc` refuses a rewrite when a delete manifest landed after its starting
snapshot. It was fail-*open* in two cases, both of which committed silently;
each now has a test that fails without the fix:
- the starting snapshot has since been expired, where the comparison fell
back
to sequence number 0 (`d22436b`);
- the rewrite was planned before the table had any snapshot, where the check
was
skipped although *everything* the table holds arrived after planning
(`f5818cc`).
`d4dbf46` documents the resulting behaviour: the check is table-wide and
always
on, so a delete committed anywhere while the compaction ran fails the commit.
That is the intended trade until the file-level check exists, but a caller
has to
be able to tell it from a bug.
Also `b8c4c70` — `validate_added_data_files` is shared with fast append, so
its
content-type error claimed "for fast append" and did not name the file.
### On the design
RFC 0003 landed in the meantime (#2620). §4.1 specifies independent concrete
producer types rather than an inheritance hierarchy, with the producer
retaining
work across retries — which is what this PR does and what (5) relies on. The
RFC
lists `RowDeltaAction` as the initial consumer of `MergingSnapshotProducer`;
RewriteFiles arriving first doesn't change the shape, and RowDelta is PR5
here.
`e7db3c8` also moves the new code onto the `invalid_data!` macro from #2928,
which landed on main while this was open.
### Not in this PR
- **File-level `validateNoNewDeletesForDataFiles`** — needs the snapshot
validation in #2243; PR6 in the series. The table-wide refusal stands in.
- **Partition-summary pruning** (Java's `canContainDeletedFiles`) — every
data
manifest is read today.
- **`set_commit_uuid` and snapshot properties on the action** — asymmetric
with
`fast_append`.
- **A second partition spec in the tests.** The spec lookup in the filter is
still only exercised by the default spec, and it is the part that matters
most
on an evolved table. @rexminnis — you offered an `identity(ts)` → `day(ts)`
fixture and an independent reader against a REST catalog once (1) and (2)
were
in; they are, so that would be very welcome, here or as a follow-up.
`cargo test -p iceberg`, `cargo fmt`, `clippy -D warnings` and
`make check-public-api` are clean; `transaction::rewrite` has 21 tests.
--
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]
