hotcache commented on issue #1607:
URL: https://github.com/apache/iceberg-rust/issues/1607#issuecomment-5365364848
Hi @CTTY, @blackmwk, @dannycjones, @rexminnis,
I've been following the discussion across #1607, #2620, and #2367, and I'd
like to help move this forward. I have a working implementation that I'd
like
to propose as a PR.
## Approach
Based on the discussion, there were two open design questions:
1. **MergingSnapshotProducer as a new struct vs extending existing
actions** —
I went with a composition-based `MergingSnapshotProducer` struct
(aligning with @blackmwk's suggestion), but using Rust composition
instead of Java-style inheritance (aligning with @dannycjones's
preference
for idiomatic Rust).
2. **Cross-retry cache** — I intentionally deferred this to a follow-up PR.
As @rexminnis noted, "always-rewrite-per-attempt is correct, just not
optimal." This unblocks the feature without requiring the cache design
to be settled first.
## Architecture
The key design choice is that `MergingSnapshotProducer` is a **shared
engine
owned by each action via composition**, not a trait or base class:
MergingSnapshotProducer (struct — shared engine)
│ - add/delete file tracking
│ - ManifestFilterManager (filters deleted files from manifests)
│ - manifest writing + summary computation
│ - delegates snapshot creation to SnapshotProducer
│
│ Owned by each action:
│
├── RewriteFilesAction (this PR)
│ operation: Replace
│ validate: fail_missing_delete_paths
│
├── OverwriteFilesAction (future)
│ operation: Overwrite
│ validate: conflict_detection_filter
│
└── RowDeltaAction (future)
operation: Overwrite
validate: no_new_deletes_on_target_files
This structure keeps future operations simple. Adding a new operation
means writing a thin struct that:
1. Owns a `MergingSnapshotProducer`
2. Implements its own `validate()` rules
3. Calls `producer.commit_snapshot()`
No shared manifest filtering, caching, or summary logic needs to be
duplicated — it's all in `MergingSnapshotProducer`. For example,
`RewriteFilesAction` is ~50 lines (excluding tests), similar to how
Java's `BaseRewriteFiles` is trivially thin on top of
`MergingSnapshotProducer`.
The existing `FastAppendAction` and `SnapshotProducer` are completely
unchanged.
## Planned follow-up PRs
- **PR2**: Manifest cache for retry optimization
(`Mutex<Option<Vec<ManifestFile>>>` in `MergingSnapshotProducer`)
- **PR3**: `SnapshotValidator` trait for conflict detection
(applies to both `RewriteFiles` and `FastAppend`)
- **PR4**: `OverwriteFilesAction` — demonstrates extensibility of the
`MergingSnapshotProducer` pattern
I'd love to hear your thoughts before I open the PR. @CTTY, I want to
make sure this doesn't conflict with your plans — happy to coordinate.
--
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]
