rexminnis commented on PR #3046:
URL: https://github.com/apache/iceberg-rust/pull/3046#issuecomment-5711613343
Thanks for pushing this forward — the filter manager resolving each
manifest's own partition spec and the summary subtracting removed files (with
the `total-*` assertions in the tests) are both things this operation has to
get right, and it's good to see them here. A few findings from reading the
committed manifests back rather than only the summary:
**1. Surviving entries are written as `Added`, not `Existing` (blocking).**
`filter_manifests` builds each survivor as `Existing` with the original
snapshot id and sequence numbers, then passes it to
`ManifestWriter::add_entry`. Per that method's doc it resets status to `Added`,
snapshot id to the writer's, and `file_sequence_number` to `None`. A read-back
test on this branch (append two files, rewrite one, then read the manifests of
the replace snapshot) shows the survivor as `status=Added`,
`snapshot_id=<replace snapshot>`, `sequence_number=1`,
`file_sequence_number=2`, and the filtered `ManifestFile` reports
`added_files_count=1, existing_files_count=0`. Incremental readers will re-emit
the survivor as new data on every compaction that touches its manifest.
`ManifestWriter::add_existing_entry` (currently `#[allow(dead_code)]`)
preserves all three fields; with that one-line swap the same test passes with
`status=Existing`, the append snapshot's id, `sequence_number=1`,
`file_sequence_number=1`, and `added=0, existing=1`, and the existing seven
tests sti
ll pass. Worth adding that read-back assertion to `test_rewrite_files_partial`
— `is_alive()` alone can't distinguish the two.
**2. Added files need a data sequence number option.**
Added entries inherit the new snapshot's sequence number. On a table with
equality deletes, compacting files at sequence 3 and 4 into a file at sequence
9 means a delete at sequence 5 no longer applies to the new file, so rows it
removed come back unless the rewriter applied it. Java exposes
`RewriteFiles.dataSequenceNumber(long)` for this (Spark's rewrite sets it to
the starting snapshot's sequence by default).
`ManifestWriter::add_file(data_file, sequence_number)` already exists, so this
is mostly threading a value through `write_added_manifest`. Without it the
action should probably document that all delete files must have been applied.
**3. Conflict validation / `starting_snapshot_id`.**
Understood this is the follow-up PR. One note on the interim:
`Transaction::do_commit` re-runs `commit` against the refreshed table on retry,
so the filter silently re-bases onto whatever is current, and a position or
equality delete committed concurrently against a replaced file is dropped.
Until `validateNoNewDeletesForDataFiles` lands, refusing when
`starting_snapshot_id` is set and any delete manifest was added after it would
keep this fail-closed. (The missing-source-file case is already fail-closed:
`DataInvalid` isn't retryable.)
**4. Producer validations are bypassed.**
`commit_with_manifests` skips `validate_added_data_files` (the
`partition_spec_id == default` and `validate_partition_value` checks — only the
content-type half is repeated here) and `validate_duplicate_files`. Adding a
file whose partition struct doesn't match the default spec writes a malformed
manifest, and adding an already-live path creates a duplicate entry. Running
both in `commit_snapshot` closes it.
**5. Manifest paths across retries.**
`commit_uuid` is fixed per action and `manifest_counter` restarts at 0 on
every `commit_snapshot` call, so attempt 2 writes `{uuid}-m0.avro` over attempt
1's. That's fine when attempt 1 provably failed, but after an ambiguous catalog
outcome it overwrites a manifest the live snapshot may reference — and here
that manifest holds surviving entries, not only the new file. Keeping the
counter on the producer (Java never resets `manifestCount`) or including the
attempt in the path avoids it. `fast_append` has the same shape, so this may be
worth fixing in `SnapshotProducer` instead.
**6. All-deleted manifests are kept forever.**
The manifest load keeps entries with `has_deleted_files()`. Java's filter
keeps `hasAddedFiles() || hasExistingFiles() || snapshotId() == current`, so
manifests that went all-deleted in an earlier commit drop out; here they ride
along in every future manifest list.
Follow-ups, not blocking: no partition-summary pruning before reading every
data manifest (Java's `canContainDeletedFiles`); no snapshot properties /
`set_commit_uuid` on the action, unlike `fast_append`; and none of the tests
exercise a second partition spec, so the spec lookup in the filter (the part
that matters most on evolved tables) is untested. Happy to contribute a
spec-evolution fixture (`identity(ts)` → `day(ts)`) and to run this against a
REST catalog with an independent reader once 1 and 2 are in.
--
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]
