CTTY commented on code in PR #1400:
URL: https://github.com/apache/iceberg-rust/pull/1400#discussion_r2131447271
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -72,16 +72,14 @@ pub(crate) struct SnapshotProduceAction<'a> {
manifest_counter: RangeFrom<u64>,
}
-impl<'a> SnapshotProduceAction<'a> {
+impl SnapshotProduceAction {
pub(crate) fn new(
- tx: Transaction<'a>,
Review Comment:
I think it would be cleaner to separate Transaction from TransactionAction
and SnapshotProduceAction. So rather than passing `tx` between
TransactionAction and SnapshotProduceAction a lot, we can define a mutable
Transaction and pass it into TransactionAction and SnapshotProduceAction only
when needed. e.g.:
```
let mut tx = Transaction::new(table);
let mut append_action = tx.fast_append()
append_action
.add_data_files(&tx, data_file); // pass tx ref in when needed
Arc::new(append_action).commit(&mut tx).await; // same here, pass the tx
ref in
let table = tx.commit(Arc::new(&rest_catalog)).await.unwrap();
```
This way we also don't have to deal with mutual reference: Tx contains a
Vec<TransactionAction>, and each TransactionAction also needs to hold a
reference to the associated Tx.
The downside would be the slightly more verbose API semantics.
Maybe there is a more elegant way to solve this? Would appreciate any input
here
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
