ZENOTME commented on PR #1490:
URL: https://github.com/apache/iceberg-rust/pull/1490#issuecomment-3043365597
Hi @CTTY, Thanks for your review!
> Since TransactionAction::commit already takes in a &Table, I don't think
having SnapshotProducer hold its own table reference adds much value
According to our current implementation, SnapshotProducer will create using
&Table from TransactionAction::commit, so I think there is no case that
SnapshotProducer has different table with TransactionAction::commit?
```
#[async_trait]
impl TransactionAction for FastAppendAction {
async fn commit(self: Arc<Self>, table: &Table) -> Result<ActionCommit> {
// validate added files
SnapshotProducer::validate_added_data_files(table,
&self.added_data_files)?;
// Checks duplicate files
if self.check_duplicate {
SnapshotProducer::validate_duplicate_files(table,
&self.added_data_files).await?;
}
let snapshot_producer = SnapshotProducer::new(
table,
self.commit_uuid.unwrap_or_else(Uuid::now_v7),
self.key_metadata.clone(),
self.snapshot_properties.clone(),
self.added_data_files.clone(),
);
snapshot_producer
.commit(table, FastAppendOperation, DefaultManifestProcess)
.await
}
}
```
> I believe this is related to how MergeAppendAction will be integrated
with SnapshotProducer though, do you think we can have MergeAppendAction reuse
the existing design?
Yes, we can change the interface like
```
pub(crate) trait ManifestProcess: Send + Sync {
fn process_manifests(
&self,
table: &Table,
snapshot_produce: &SnapshotProducer<'_>,
manifests: Vec<ManifestFile>,
) -> Vec<ManifestFile>;
}
```
But I think the interface can be more clear if we store Table in
snapshot_produce to avoid pass the Table to lots of function around.
--
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]
